Closed Bug 1151077 Opened 5 years ago Closed 5 years ago

Desktop reading list sync module should batch its POST /batch requests

Categories

(Firefox Graveyard :: Reading List, defect, P1)

defect

Tracking

(firefox38 fixed, firefox39 fixed, firefox40 fixed)

RESOLVED FIXED
Firefox 40
Tracking Status
firefox38 --- fixed
firefox39 --- fixed
firefox40 --- fixed

People

(Reporter: adw, Assigned: adw)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

Almost forgot about this, but the list of requests in POST /batch is "limited to 25 by default", which means we have to batch our batches.  I asked Richard about it a week or two ago and he said indeed that's right but they aren't limiting batches on Android yet.

http://readinglist.readthedocs.org/en/latest/api/batch.html
It sounds a little like this might break sync for users who add more than 25 items before initially syncing, so bumping priority and "affected" flags accordingly.
Points: --- → 3
Flags: qe-verify-
Flags: firefox-backlog+
Priority: -- → P1
yodawg.png

(In reply to Drew Willcoxon :adw from comment #0)

> I asked Richard about it a week or two ago and he said indeed that's right but
> they aren't limiting batches on Android yet.

Actually, we aren't using batching *at all*. We make one HTTP request per item.

("If you're not embarrassed by your v1, you're not shipping soon enough")
Assignee: nobody → adw
Status: NEW → ASSIGNED
Blocks: 1132074
Iteration: --- → 40.1 - 13 Apr
Attached patch patchSplinter Review
I manually verified that this works by adding a hundred new items and syncing.  I modified the scheduler to not add its reading list listener so that it didn't sync when I added the items so that I could fire off one sync with all the new items at once.  I also tested without that scheduler modification, and that worked fine too.
Attachment #8589271 - Flags: review?(mhammond)
Attachment #8589271 - Flags: review?(mhammond) → review+
https://hg.mozilla.org/mozilla-central/rev/41a0c9bc40df
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 40
Drew, just a reminder we need to uplift this.
Flags: needinfo?(adw)
Comment on attachment 8589271 [details] [diff] [review]
patch

Approval Request Comment
[Feature/regressing bug #]: Desktop reading list sync, bug 1131416
[User impact if declined]: Users with more than 25 items to sync at one time likely will not be able to properly sync
[Describe test coverage new/current, TreeHerder]: Manual testing with production reading list server
[Risks and why]: Low risk, fairly simple patch
[String/UUID change made/needed]: None
Flags: needinfo?(adw)
Attachment #8589271 - Flags: approval-mozilla-beta?
Attachment #8589271 - Flags: approval-mozilla-aurora?
Comment on attachment 8589271 [details] [diff] [review]
patch

Approving for uplift to aurora.
Attachment #8589271 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment on attachment 8589271 [details] [diff] [review]
patch

We want this feature to work as best as possible, taking it for 38.

Should be in 38 beta 4
Attachment #8589271 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.