Closed
Bug 1151077
Opened 6 years ago
Closed 6 years ago
Desktop reading list sync module should batch its POST /batch requests
Categories
(Firefox Graveyard :: Reading List, defect, P1)
Firefox Graveyard
Reading List
Tracking
(firefox38 fixed, firefox39 fixed, firefox40 fixed)
RESOLVED
FIXED
Firefox 40
People
(Reporter: adw, Assigned: adw)
References
(Blocks 1 open bug)
Details
Attachments
(1 file)
6.12 KB,
patch
|
markh
:
review+
lizzard
:
approval-mozilla-aurora+
Sylvestre
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
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
Comment 1•6 years ago
|
||
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
status-firefox38:
--- → affected
status-firefox39:
--- → affected
Flags: qe-verify-
Flags: firefox-backlog+
Priority: -- → P1
Comment 2•6 years ago
|
||
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 | ||
Updated•6 years ago
|
Assignee: nobody → adw
Status: NEW → ASSIGNED
Assignee | ||
Comment 3•6 years ago
|
||
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)
Updated•6 years ago
|
Attachment #8589271 -
Flags: review?(mhammond) → review+
Assignee | ||
Comment 4•6 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/41a0c9bc40df
Comment 5•6 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/41a0c9bc40df
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 40
Assignee | ||
Comment 7•6 years ago
|
||
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 10•6 years ago
|
||
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+
Updated•5 years ago
|
Product: Firefox → Firefox Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•