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

Status: RESOLVED FIXED

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

Description

[Drew Willcoxon :adw]

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

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

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.

Comment 2

[Richard Newman [:rnewman]]

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")

Comment 3

[Drew Willcoxon :adw]

Attachment: patch

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.

Comment 4

[Drew Willcoxon :adw]

https://hg.mozilla.org/integration/fx-team/rev/41a0c9bc40df

Comment 5

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/41a0c9bc40df

Comment 6

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

Drew, just a reminder we need to uplift this.

Comment 7

[Drew Willcoxon :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

Comment 8

[Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8589271 [details] [diff] [review]
patch

Approving for uplift to aurora.

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/b852e95d2f6c

Comment 10

[Sylvestre Ledru [:Sylvestre]]

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

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/1b6ba1cb52f6