Bug 1373897 - Clear the succeeded records GUIDs list after each POST in non-batched mode. r=Grisha
authorEdouard Oger <eoger@fastmail.com>
Wed, 30 Aug 2017 16:46:41 -0400
changeset 660360 8d4b1890f7cf73a8ca194e2852cff9c5916b30b6
parent 660359 5edfd440c97f26c484add1826c985d05c4cdb5cc
child 660361 eb99a243bb9d34d9bda0f7f5154c31907b6801a2
push id78390
push userbmo:emilio@crisal.io
push dateWed, 06 Sep 2017 23:04:15 +0000
reviewersGrisha
bugs1373897
milestone57.0a1
Bug 1373897 - Clear the succeeded records GUIDs list after each POST in non-batched mode. r=Grisha MozReview-Commit-ID: 4VnYZc6gMkc
mobile/android/services/src/main/java/org/mozilla/gecko/sync/repositories/uploaders/BatchMeta.java
mobile/android/services/src/main/java/org/mozilla/gecko/sync/repositories/uploaders/PayloadDispatcher.java
mobile/android/services/src/main/java/org/mozilla/gecko/sync/repositories/uploaders/PayloadUploadDelegate.java
mobile/android/tests/background/junit4/src/org/mozilla/gecko/sync/repositories/uploaders/PayloadUploadDelegateTest.java
--- a/mobile/android/services/src/main/java/org/mozilla/gecko/sync/repositories/uploaders/BatchMeta.java
+++ b/mobile/android/services/src/main/java/org/mozilla/gecko/sync/repositories/uploaders/BatchMeta.java
@@ -49,16 +49,20 @@ public class BatchMeta {
         // Sanity check.
         if (recordGuid == null) {
             throw new IllegalStateException("Record guid is unexpectedly null");
         }
 
         successRecordGuids.add(recordGuid);
     }
 
+    void clearSuccessRecordGuids() {
+        successRecordGuids.clear();
+    }
+
     /* package-local */ void setInBatchingMode(boolean inBatchingMode) {
         this.inBatchingMode = inBatchingMode;
     }
 
     /* package-local */ Boolean getInBatchingMode() {
         return inBatchingMode;
     }
 
--- a/mobile/android/services/src/main/java/org/mozilla/gecko/sync/repositories/uploaders/PayloadDispatcher.java
+++ b/mobile/android/services/src/main/java/org/mozilla/gecko/sync/repositories/uploaders/PayloadDispatcher.java
@@ -67,38 +67,39 @@ class PayloadDispatcher {
         uploader.setUnlimitedMode(!inBatchingMode);
     }
 
     /**
      * We've been told by our upload delegate that a payload succeeded.
      * Depending on the type of payload and batch mode status, inform our delegate of progress.
      *
      * @param response success response to our commit post
-     * @param guids list of successfully posted record guids
      * @param isCommit was this a commit upload?
      * @param isLastPayload was this a very last payload we'll upload?
      */
-    void payloadSucceeded(final SyncStorageResponse response, final String[] guids, final boolean isCommit, final boolean isLastPayload) {
+    void payloadSucceeded(final SyncStorageResponse response, final boolean isCommit, final boolean isLastPayload) {
         // Sanity check.
         if (batchWhiteboard.getInBatchingMode() == null) {
             throw new IllegalStateException("Can't process payload success until we know if we're in a batching mode");
         }
 
+        final String[] guids = batchWhiteboard.getSuccessRecordGuids();
         // We consider records to have been committed if we're not in a batching mode or this was a commit.
         // If records have been committed, notify our store delegate.
         if (!batchWhiteboard.getInBatchingMode() || isCommit) {
             for (String guid : guids) {
                 uploader.sessionStoreDelegate.onRecordStoreSucceeded(guid);
             }
 
             // If we're not in a batching mode, or just committed a batch, uploaded records have
             // been applied to the server storage and are now visible to other clients.
             // Therefore, we bump our local "last store" timestamp.
             bumpTimestampTo(uploadTimestamp, response.normalizedTimestampForHeader(SyncResponse.X_LAST_MODIFIED));
             uploader.setLastStoreTimestamp(uploadTimestamp);
+            batchWhiteboard.clearSuccessRecordGuids();
         }
 
         // If this was our very last commit, we're done storing records.
         // Get Last-Modified timestamp from the response, and pass it upstream.
         if (isLastPayload) {
             uploader.finished();
         }
     }
--- a/mobile/android/services/src/main/java/org/mozilla/gecko/sync/repositories/uploaders/PayloadUploadDelegate.java
+++ b/mobile/android/services/src/main/java/org/mozilla/gecko/sync/repositories/uploaders/PayloadUploadDelegate.java
@@ -167,17 +167,16 @@ class PayloadUploadDelegate implements S
             }
         }
         // GC
         failed = null;
 
         // And we're done! Let uploader finish up.
         dispatcher.payloadSucceeded(
                 response,
-                dispatcher.batchWhiteboard.getSuccessRecordGuids(),
                 isCommit,
                 isLastPayload
         );
 
         if (isCommit && !isLastPayload) {
             dispatcher.prepareForNextBatch();
         }
     }
--- a/mobile/android/tests/background/junit4/src/org/mozilla/gecko/sync/repositories/uploaders/PayloadUploadDelegateTest.java
+++ b/mobile/android/tests/background/junit4/src/org/mozilla/gecko/sync/repositories/uploaders/PayloadUploadDelegateTest.java
@@ -52,25 +52,29 @@ public class PayloadUploadDelegateTest {
 
         public int committedGuids = 0;
 
         public MockPayloadDispatcher(final Executor workQueue, final BatchingUploader uploader) {
             super(workQueue, uploader, null);
         }
 
         @Override
-        public void payloadSucceeded(final SyncStorageResponse response, String[] guids, final boolean isCommit, final boolean isLastPayload) {
+        public void payloadSucceeded(final SyncStorageResponse response, final boolean isCommit, final boolean isLastPayload) {
+            final String[] guids = batchWhiteboard.getSuccessRecordGuids();
             successResponses.add(response);
+            if (!batchWhiteboard.getInBatchingMode() || isCommit) {
+                committedGuids += guids.length;
+            }
             if (isCommit) {
                 ++commitPayloadsSucceeded;
-                committedGuids += guids.length;
             }
             if (isLastPayload) {
                 ++lastPayloadsSucceeded;
             }
+            super.payloadSucceeded(response, isCommit, isLastPayload);
         }
 
         @Override
         public void recordFailed(final String recordGuid) {
             recordFailed(new Exception(), recordGuid);
         }
 
         @Override
@@ -109,17 +113,17 @@ public class PayloadUploadDelegateTest {
         }
 
         @Override
         public void onRecordStoreReconciled(String guid, String oldGuid, Integer newVersion) {
         }
 
         @Override
         public RepositorySessionStoreDelegate deferredStoreDelegate(ExecutorService executor) {
-            return null;
+            return this;
         }
     }
 
     @Before
     public void setUp() throws Exception {
         sessionStoreDelegate = new MockRepositorySessionStoreDelegate();
 
         payloadDispatcher = new MockPayloadDispatcher(
@@ -226,31 +230,29 @@ public class PayloadUploadDelegateTest {
         postedGuids.add("guid2");
         postedGuids.add("guid3");
         PayloadUploadDelegate payloadUploadDelegate = new PayloadUploadDelegate(
                 authHeaderProvider, payloadDispatcher, postedGuids, false, false);
 
         payloadUploadDelegate.handleRequestSuccess(
                 makeSyncStorageResponse(200, "{\"success\": [\"guid1\", \"guid2\", \"guid3\"]}", "123"));
         assertEquals(0, ((MockPayloadDispatcher) payloadDispatcher).failedRecords.size());
-        assertEquals(3, payloadDispatcher.batchWhiteboard.getSuccessRecordGuids().length);
         assertFalse(((MockPayloadDispatcher) payloadDispatcher).didLastPayloadFail);
         assertEquals(1, ((MockPayloadDispatcher) payloadDispatcher).successResponses.size());
         assertEquals(0, ((MockPayloadDispatcher) payloadDispatcher).commitPayloadsSucceeded);
         assertEquals(0, ((MockPayloadDispatcher) payloadDispatcher).lastPayloadsSucceeded);
 
         // These should fail, because we're returning a non-changed L-M in a non-batching mode
         postedGuids.add("guid4");
         postedGuids.add("guid6");
         payloadUploadDelegate = new PayloadUploadDelegate(
                 authHeaderProvider, payloadDispatcher, postedGuids, false, false);
         payloadUploadDelegate.handleRequestSuccess(
                 makeSyncStorageResponse(200, "{\"success\": [\"guid4\", 5, \"guid6\"]}", "123"));
         assertEquals(5, ((MockPayloadDispatcher) payloadDispatcher).failedRecords.size());
-        assertEquals(3, payloadDispatcher.batchWhiteboard.getSuccessRecordGuids().length);
         assertFalse(((MockPayloadDispatcher) payloadDispatcher).didLastPayloadFail);
         assertEquals(1, ((MockPayloadDispatcher) payloadDispatcher).successResponses.size());
         assertEquals(0, ((MockPayloadDispatcher) payloadDispatcher).commitPayloadsSucceeded);
         assertEquals(0, ((MockPayloadDispatcher) payloadDispatcher).lastPayloadsSucceeded);
         assertEquals(BatchingUploader.LastModifiedDidNotChange.class,
                 ((MockPayloadDispatcher) payloadDispatcher).failedRecords.get("guid4").getClass());
     }
 
@@ -260,49 +262,53 @@ public class PayloadUploadDelegateTest {
         postedGuids.add("guid1");
         postedGuids.add("guid2");
         postedGuids.add("guid3");
         PayloadUploadDelegate payloadUploadDelegate = new PayloadUploadDelegate(
                 authHeaderProvider, payloadDispatcher, postedGuids, false, false);
         payloadUploadDelegate.handleRequestSuccess(
                 makeSyncStorageResponse(200, "{\"success\": [\"guid1\", \"guid2\", \"guid3\"], \"failed\": {}}", "123"));
 
+        assertEquals(3, ((MockPayloadDispatcher) payloadDispatcher).committedGuids);
+
         postedGuids = new ArrayList<>();
         postedGuids.add("guid4");
         postedGuids.add("guid5");
         payloadUploadDelegate = new PayloadUploadDelegate(
                 authHeaderProvider, payloadDispatcher, postedGuids, false, false);
         payloadUploadDelegate.handleRequestSuccess(
                 makeSyncStorageResponse(200, "{\"success\": [\"guid4\", \"guid5\"], \"failed\": {}}", "333"));
 
+        assertEquals(5, ((MockPayloadDispatcher) payloadDispatcher).committedGuids);
+
         postedGuids = new ArrayList<>();
         postedGuids.add("guid6");
         payloadUploadDelegate = new PayloadUploadDelegate(
                 authHeaderProvider, payloadDispatcher, postedGuids, false, true);
         payloadUploadDelegate.handleRequestSuccess(
                 makeSyncStorageResponse(200, "{\"success\": [\"guid6\"], \"failed\": {}}", "444"));
 
         assertEquals(0, ((MockPayloadDispatcher) payloadDispatcher).failedRecords.size());
-        assertEquals(6, payloadDispatcher.batchWhiteboard.getSuccessRecordGuids().length);
+        assertEquals(6, ((MockPayloadDispatcher) payloadDispatcher).committedGuids);
         assertFalse(((MockPayloadDispatcher) payloadDispatcher).didLastPayloadFail);
         assertEquals(3, ((MockPayloadDispatcher) payloadDispatcher).successResponses.size());
         assertEquals(0, ((MockPayloadDispatcher) payloadDispatcher).commitPayloadsSucceeded);
         assertEquals(1, ((MockPayloadDispatcher) payloadDispatcher).lastPayloadsSucceeded);
         assertFalse(payloadDispatcher.batchWhiteboard.getInBatchingMode());
 
         postedGuids = new ArrayList<>();
         postedGuids.add("guid7");
         postedGuids.add("guid8");
         payloadUploadDelegate = new PayloadUploadDelegate(
                 authHeaderProvider, payloadDispatcher, postedGuids, false, true);
         payloadUploadDelegate.handleRequestSuccess(
                 makeSyncStorageResponse(200, "{\"success\": [\"guid8\"], \"failed\": {\"guid7\": \"reason\"}}", "555"));
+        assertEquals(7, ((MockPayloadDispatcher) payloadDispatcher).committedGuids);
         assertEquals(1, ((MockPayloadDispatcher) payloadDispatcher).failedRecords.size());
         assertTrue(((MockPayloadDispatcher) payloadDispatcher).failedRecords.containsKey("guid7"));
-        assertEquals(7, payloadDispatcher.batchWhiteboard.getSuccessRecordGuids().length);
         assertFalse(((MockPayloadDispatcher) payloadDispatcher).didLastPayloadFail);
         assertEquals(4, ((MockPayloadDispatcher) payloadDispatcher).successResponses.size());
         assertEquals(0, ((MockPayloadDispatcher) payloadDispatcher).commitPayloadsSucceeded);
         assertEquals(2, ((MockPayloadDispatcher) payloadDispatcher).lastPayloadsSucceeded);
         assertFalse(payloadDispatcher.batchWhiteboard.getInBatchingMode());
     }
 
     @Test