Closed Bug 1151077 Opened 6 years ago Closed 6 years ago

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


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



(firefox38 fixed, firefox39 fixed, firefox40 fixed)

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


(Reporter: adw, Assigned: adw)


(Blocks 1 open bug)



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

(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
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+
Closed: 6 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]

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]

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

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.