1152698 - scheduler doesn't see sync engine server error responses as errors.

Status: RESOLVED FIXED

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

Description

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

_handleUnexpectedResponse should probably throw for non 200 errors - or it's got to do something else special, else we end up with:

> 1428570983146   readinglist.sync        ERROR   Unexpected response downloading modified items: {"status":401,...
> 1428570983149   readinglist.sync        INFO    Sync done
> 1428570983150   readinglist.scheduler   INFO    Sync completed successfully
1428570983155   readinglist.scheduler   INFO    next scheduled sync is in [2 hours]

Bug 1148980 will cause it to be an error log (assuming it lands) but it still leaves us with the "2 hours" thing - we need to enter an "error" schedule.

Maybe we could be conservative and have _handleUnexpectedResponse set a flag and throw at the very end - or maybe there's a flag on the engine the scheduler could check?

Either way, I think this is important - it seems like something that might cause a perception of "it never syncs"

Comment 1

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

Attachment: 0005-Bug-1152698-certain-readinglist-server-response-code.patch

This patch throws on 401 or 5XX, which I think is a reasonable first start.  However, there is a risk we'll strike a server bug causing a 50X on just a single item which we could ignore and still get a "mostly successful" sync, but I'm not sure that's a good reason to try and force our way forward in these error states.

Comment 2

[Justin Dolske [:Dolske]]

P2 based on "I think this is important - it seems like something that might cause a perception of "it never syncs", but it also sorta sounds like this is just a logging fix?

Comment 3

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

(In reply to Justin Dolske [:Dolske] from comment #2)
> P2 based on "I think this is important - it seems like something that might
> cause a perception of "it never syncs", but it also sorta sounds like this
> is just a logging fix?

Actually, it also involves the scheduler - without this fix we wait 2 hours after an error before we retry. With this fix we exponentially back off to do 2mins, 4mins, 8mins retry until either success or 2 hours.

Comment 4

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

Attachment: 0002-Bug-1152698-certain-readinglist-server-response-code.patch

On reviewing your "todo" patch, I thought that maybe the first version of this patch was too aggressive - that aborting on those response codes in "sub" responses could cause issues.  I think we only need to abort when the "top-level" response is 401 or 5XX (eg, I could imagine a server bug might cause one single item to always return 5XX even though the response itself worked as did every other item in the body.

So this version adds a bool to _handleUnexpectedResponse(), which indicates if the response is top-level. We then only throw when it is true.

Drew, do you think this is an improvement on the earlier one?

Comment 5

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

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

Comment 6

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/cf459cec404b

Comment 7

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

Comment on attachment 8592572 [details] [diff] [review]
0002-Bug-1152698-certain-readinglist-server-response-code.patch

Approval Request Comment
[Feature/regressing bug #]: readinglist
[User impact if declined]: The readinglist sync engine will only retry every 2 hours on error instead of by the normal error schedule which is more frequent.
[Describe test coverage new/current, TreeHerder]: Existing tests pass
[Risks and why]: Risk limited to Readinglist syncing.
[String/UUID change made/needed]: None

Comment 8

[Liz Henry (:lizzard) (relman/hg->git project)]

Mark, do we still need to uplift this to 38 and 39 if we're aiming for a future release or would it make more sense to keep this on 40 for now?

Comment 9

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

Comment on attachment 8592572 [details] [diff] [review]
0002-Bug-1152698-certain-readinglist-server-response-code.patch

No need to uplift this