1403052 - tabs collection may exceed memcached server limits

Status: RESOLVED FIXED

(Firefox :: Sync, defect, P1)

Description

[Mark Hammond [:markh] [:mhammond]]

[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.

Comment 1

[Ryan Kelly [:rfkelly]]

I suggest 512kb in order to leave enough room for JSON serialization and other overheads added by the server.

Comment 2

[Mark Hammond [:markh] [:mhammond]]

(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 3

[Mark Hammond [:markh] [:mhammond]]

doh - make that http://searchfox.org/mozilla-central/rev/f6dc0e40b51a37c34e1683865395e72e7fca592c/services/sync/modules/engines/tabs.js#219,230

Comment 4

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Tracked since it's a recent regression.

Comment 5

[Thom Chiovoloni [:tcsc] (ex-moco)]

Attachment: Bug 1403052 - Limit tab sync max_record_payload_size to 512k.

Also fixes an issue where we wouldn't encode to utf8 when comparing the actual
size to the limit after the first time.

Review commit: https://reviewboard.mozilla.org/r/185858/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/185858/

Comment 6

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 8914542 [details]
Bug 1403052 - Limit tab sync max_record_payload_size to 512k.

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

Thanks!

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/881dc00d8264
Limit tab sync max_record_payload_size to 512k. r=markh

Comment 8

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/881dc00d8264

Comment 9

[Ryan VanderMeulen [:RyanVM]]

Please request Beta approval on this when you get a chance.

Comment 10

[Thom Chiovoloni [:tcsc] (ex-moco)]

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

Comment 11

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

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+

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/68551d5aa88b

Comment 13

[Andrei Vaida [:avaida]]

(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.