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 428703 8d4b1890f7cf73a8ca194e2852cff9c5916b30b6
parent 428702 5edfd440c97f26c484add1826c985d05c4cdb5cc
child 428704 eb99a243bb9d34d9bda0f7f5154c31907b6801a2
push id7761
push userjlund@mozilla.com
push dateFri, 15 Sep 2017 00:19:52 +0000
treeherdermozilla-beta@c38455951db4 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersGrisha
bugs1373897
milestone57.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
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