resolve comments

googleapis · Apr 22, 2021 · 3e8736e · 3e8736e
1 parent 7676e06
commit 3e8736e
Show file tree

Hide file tree

Showing 2 changed files with 14 additions and 10 deletions.
diff --git a/google-cloud-firestore/src/main/java/com/google/cloud/firestore/BulkWriter.java b/google-cloud-firestore/src/main/java/com/google/cloud/firestore/BulkWriter.java
@@ -867,7 +867,7 @@ private void scheduleCurrentBatchLocked(final boolean flush) {
         highestBackoffDuration = op.getBackoffDuration();
       }
     }
-    int backoffMsWithJitter = applyJitter(highestBackoffDuration);
+    final int backoffMsWithJitter = applyJitter(highestBackoffDuration);
 
     bulkWriterExecutor.schedule(
         new Runnable() {

diff --git a/google-cloud-firestore/src/test/java/com/google/cloud/firestore/BulkWriterTest.java b/google-cloud-firestore/src/test/java/com/google/cloud/firestore/BulkWriterTest.java
@@ -1092,6 +1092,7 @@ public void retriesWritesWhenBatchWriteFailsWithRetryableError() throws Exceptio
   @Test
   public void failsWritesAfterAllRetryAttemptsFail() throws Exception {
     final int[] retryAttempts = {0};
+    final int[] scheduleWithDelayCount = {0};
     final ScheduledExecutorService timeoutExecutor =
         new ScheduledThreadPoolExecutor(1) {
           @Override
@@ -1105,6 +1106,7 @@ public ScheduledFuture<?> schedule(Runnable command, long delay, TimeUnit unit)
 
               assertTrue(delay >= (1 - BulkWriter.DEFAULT_JITTER_FACTOR) * expected);
               assertTrue(delay <= (1 + BulkWriter.DEFAULT_JITTER_FACTOR) * expected);
+              scheduleWithDelayCount[0]++;
             }
             return super.schedule(command, 0, TimeUnit.MILLISECONDS);
           }
@@ -1132,22 +1134,21 @@ public ApiFuture<GeneratedMessageV3> answer(InvocationOnMock mock) {
       Assert.fail("Expected set() operation to fail");
     } catch (Exception e) {
       assertTrue(e.getMessage().contains("Mock batchWrite failed in test"));
-      assertEquals(retryAttempts[0], BulkWriter.MAX_RETRY_ATTEMPTS + 1);
+      assertEquals(BulkWriter.MAX_RETRY_ATTEMPTS + 1, retryAttempts[0]);
+      // The first attempt should not have a delay.
+      assertEquals(BulkWriter.MAX_RETRY_ATTEMPTS, scheduleWithDelayCount[0]);
     }
   }
 
   @Test
   public void appliesMaxBackoffOnRetriesForResourceExhausted() throws Exception {
     final int[] retryAttempts = {0};
+    final int[] scheduleWithDelayCount = {0};
     final ScheduledExecutorService timeoutExecutor =
         new ScheduledThreadPoolExecutor(1) {
           @Override
           @Nonnull
           public ScheduledFuture<?> schedule(Runnable command, long delay, TimeUnit unit) {
-            int expected =
-                (int)
-                    (BulkWriterOperation.DEFAULT_BACKOFF_INITIAL_DELAY_MS
-                        * Math.pow(1.5, retryAttempts[0]));
             if (delay > 0) {
               assertTrue(
                   delay
@@ -1157,6 +1158,7 @@ public ScheduledFuture<?> schedule(Runnable command, long delay, TimeUnit unit)
                   delay
                       <= (1 + BulkWriter.DEFAULT_JITTER_FACTOR)
                           * BulkWriterOperation.DEFAULT_BACKOFF_MAX_DELAY_MS);
+              scheduleWithDelayCount[0]++;
             }
             return super.schedule(command, 0, TimeUnit.MILLISECONDS);
           }
@@ -1191,7 +1193,9 @@ public boolean onError(BulkWriterException error) {
       Assert.fail("Expected create() operation to fail");
     } catch (Exception e) {
       assertTrue(e.getMessage().contains("Mock batchWrite failed in test"));
-      assertEquals(retryAttempts[0], 5);
+      assertEquals(5, retryAttempts[0]);
+      // The first attempt should not have a delay.
+      assertEquals(4, scheduleWithDelayCount[0]);
     }
   }
 
@@ -1256,7 +1260,7 @@ public boolean onError(BulkWriterException error) {
     bulkWriter.create(doc1, LocalFirestoreHelper.SINGLE_FIELD_MAP);
     bulkWriter.set(doc2, LocalFirestoreHelper.SINGLE_FIELD_MAP);
     bulkWriter.close();
-    assertEquals(retryAttempts[0], 2);
+    assertEquals(2, retryAttempts[0]);
   }
 
   @Test
@@ -1268,7 +1272,7 @@ public void sendsBackoffBatchAfterOtherEnqueuedBatches() throws Exception {
                 batchWrite(create(LocalFirestoreHelper.SINGLE_FIELD_PROTO, "coll/doc1")),
                 failedResponse(Code.RESOURCE_EXHAUSTED_VALUE));
             put(
-                batchWrite(set(LocalFirestoreHelper.SINGLE_FIELD_PROTO, "coll/doc1")),
+                batchWrite(set(LocalFirestoreHelper.SINGLE_FIELD_PROTO, "coll/doc2")),
                 successResponse(0));
             put(
                 batchWrite(create(LocalFirestoreHelper.SINGLE_FIELD_PROTO, "coll/doc1")),
@@ -1285,7 +1289,7 @@ public boolean onError(BulkWriterException error) {
         });
     bulkWriter.create(doc1, LocalFirestoreHelper.SINGLE_FIELD_MAP);
     bulkWriter.flush();
-    bulkWriter.set(doc1, LocalFirestoreHelper.SINGLE_FIELD_MAP);
+    bulkWriter.set(doc2, LocalFirestoreHelper.SINGLE_FIELD_MAP);
     bulkWriter.close();
   }