1409472 - Pass whether or not we should abort on first failure through to BatchingUploader

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Android Sync, enhancement, P1)

Description

[Thom Chiovoloni [:tcsc] (ex-moco)]

It turns out that bug 1362206 doesn't quite cut it outside of the tests, since in practice the records are all CryptoRecords. Instead, we should tell the BatchingUploader up-front that it should abort on failure. One of the earlier revisions of the changeset for that bug did this, so it shouldn't be very hard.

Comment 1

[Thom Chiovoloni [:tcsc] (ex-moco)]

Attachment: Bug 1409472 - Replace always-wrong instanceof check with an explicit boolean in BatchingUploader.

Review commit: https://reviewboard.mozilla.org/r/190284/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/190284/

Comment 2

[:Grisha Kruglov]

Comment on attachment 8919443 [details]
Bug 1409472 - Replace always-wrong instanceof check with an explicit boolean in BatchingUploader.

https://reviewboard.mozilla.org/r/190284/#review196000

Looks all right. Make sure it's green on 'try', doesn't look like your push actually ran many of the tests.

::: mobile/android/services/src/main/java/org/mozilla/gecko/sync/repositories/ConfigurableServer15Repository.java:45
(Diff revision 1)
>            String sort,
>            ServerSyncStage.MultipleBatches multipleBatches,
>            ServerSyncStage.HighWaterMark highWaterMark,
>            RepositoryStateProvider stateProvider,
> -          boolean forceFullFetch) throws URISyntaxException {
> +          boolean forceFullFetch,
> +          boolean abortOnStoreFailure) throws URISyntaxException {

"If your function has ten parameters, you're probably missing a few".

Hoping we'll get to clean this up at some point.

::: mobile/android/services/src/main/java/org/mozilla/gecko/sync/stage/BookmarksServerSyncStage.java:86
(Diff revision 1)
>              BOOKMARKS_SORT,
>              getAllowedMultipleBatches(),
>              getAllowedToUseHighWaterMark(),
>              getRepositoryStateProvider(),
> -            false
> +            false,
> +            true

This is starting to look like an example out of a refactoring text book :)

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/00fd9db200d6
Replace always-wrong instanceof check with an explicit boolean in BatchingUploader. r=Grisha

Comment 4

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/00fd9db200d6