tabs collection may exceed memcached server limits

RESOLVED FIXED in Firefox 57

Status

()

defect
P1
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: markh, Assigned: tcsc)

Tracking

Trunk
Firefox 58
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +
qe-verify -

Firefox Tracking Flags

(firefox56 unaffected, firefox57+ fixed, firefox58 fixed)

Details

Attachments

(1 attachment)

[Tracking Requested - why for this release]:

In bug 1393659 we changed how we calculate how many tabs can fit in a payload to use data returned from info/configuration. However, in bug 1300451 comment 40 and later, we see that the "tabs" collection is stored in memcached so in-practice can not use the advertised server limits.

Therefore we should change http://searchfox.org/mozilla-central/rev/f6dc0e40b51a37c34e1683865395e72e7fca592c/services/sync/modules/engines/tabs.js#221 to use a hard-coded limit.

Ryan, is 1MB an appropriate limit? Seeing we had 256k before and no complaints, I'd be fine with 512kb or even all the way back to 256k if there was a reason to do so.
Flags: needinfo?(rfkelly)
I suggest 512kb in order to leave enough room for JSON serialization and other overheads added by the server.
Flags: needinfo?(rfkelly)
(also, shouldn'at http://searchfox.org/mozilla-central/rev/f6dc0e40b51a37c34e1683865395e72e7fca592c/services/sync/modules/engines/tabs.js#221,230, shouldn't those lines both be doing the same size calculation, utf-8 encoder and all?)
Tracked since it's a recent regression.
Priority: -- → P1
Assignee: nobody → tchiovoloni
Comment on attachment 8914542 [details]
Bug 1403052 - Limit tab sync max_record_payload_size to 512k.

https://reviewboard.mozilla.org/r/185858/#review190924

Thanks!
Attachment #8914542 - Flags: review?(markh) → review+
Pushed by tchiovoloni@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/881dc00d8264
Limit tab sync max_record_payload_size to 512k. r=markh
https://hg.mozilla.org/mozilla-central/rev/881dc00d8264
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 58
Please request Beta approval on this when you get a chance.
Flags: needinfo?(tchiovoloni)
Comment on attachment 8914542 [details]
Bug 1403052 - Limit tab sync max_record_payload_size to 512k.

Approval Request Comment
[Feature/Bug causing the regression]: 1393659
[User impact if declined]: Tab sync might fail for users with large number of tabs.
[Is this code covered by automated tests?]: Yes.
[Has the fix been verified in Nightly?]: Yes.
[Needs manual test from QE? If yes, steps to reproduce]: No.
[List of other uplifts needed for the feature/fix]: N/A
[Is the change risky?]: No.
[Why is the change risky/not risky?]: Small change to code that causes less data to be synced.
[String changes made/needed]: N/A
Flags: needinfo?(tchiovoloni)
Attachment #8914542 - Flags: approval-mozilla-beta?
Comment on attachment 8914542 [details]
Bug 1403052 - Limit tab sync max_record_payload_size to 512k.

recent regression in syncing with large number of tabs, beta57+
Attachment #8914542 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
(In reply to Thom Chiovoloni [:tcsc] from comment #10)
> [Is this code covered by automated tests?]: Yes.
> [Has the fix been verified in Nightly?]: Yes.
> [Needs manual test from QE? If yes, steps to reproduce]: No.

Setting qe-verify- based on Thom's assessment on manual testing needs and the fact this fix has automated coverage.
Flags: qe-verify-
Duplicate of this bug: 1411794
You need to log in before you can comment on or make changes to this bug.