Closed Bug 1403052 Opened 3 years ago Closed 3 years ago
tabs collection may exceed memcached server limits
59 bytes, text/x-review-board-request
[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.
I suggest 512kb in order to leave enough room for JSON serialization and other overheads added by the server.
(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?)
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 firstname.lastname@example.org: https://hg.mozilla.org/integration/autoland/rev/881dc00d8264 Limit tab sync max_record_payload_size to 512k. r=markh
Please request Beta approval on this when you get a chance.
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
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.
You need to log in before you can comment on or make changes to this bug.