Repository: GoogleCloudPlatform/cloud-bigtable-client
Pull request #1994 (2018-11-09 19:47:12)

Fix backoff NPE when batch mutator runs out of retry attempts 
When we migrated to gax's RetryAlgorithm, we forgot to migrate the all call sites. The bulk mutation handler was still expecting a -1 to signal end of retries, but it should've been expecting a null.


Patch:
diff --git a/bigtable-client-core-parent/bigtable-client-core/src/main/java/com/google/cloud/bigtable/grpc/async/RetryingMutateRowsOperation.java b/bigtable-client-core-parent/bigtable-client-core/src/main/java/com/google/cloud/bigtable/grpc/async/RetryingMutateRowsOperation.java
index 5cf4ab5bc..861c85094 100644
--- a/bigtable-client-core-parent/bigtable-client-core/src/main/java/com/google/cloud/bigtable/grpc/async/RetryingMutateRowsOperation.java
+++ b/bigtable-client-core-parent/bigtable-client-core/src/main/java/com/google/cloud/bigtable/grpc/async/RetryingMutateRowsOperation.java
@@ -84,8 +84,8 @@ public class RetryingMutateRowsOperation extends
     }
 
     // Perform a partial retry, if the backoff policy allows it.
-    long nextBackOff = getNextBackoff();
-    if (nextBackOff == BackOff.STOP) {
+    Long nextBackOff = getNextBackoff();
+    if (nextBackOff == null) {
       // Return the response as is, and don't retry;
       rpc.getRpcMetrics().markRetriesExhasted();
       completionFuture.set(Arrays.asList(requestManager.buildResponse()));


Test patch:
diff --git a/bigtable-client-core-parent/bigtable-client-core/src/test/java/com/google/cloud/bigtable/grpc/async/OperationClock.java b/bigtable-client-core-parent/bigtable-client-core/src/test/java/com/google/cloud/bigtable/grpc/async/OperationClock.java
index fb153dde8..09bd8f0a6 100644
--- a/bigtable-client-core-parent/bigtable-client-core/src/test/java/com/google/cloud/bigtable/grpc/async/OperationClock.java
+++ b/bigtable-client-core-parent/bigtable-client-core/src/test/java/com/google/cloud/bigtable/grpc/async/OperationClock.java
@@ -15,10 +15,9 @@
  */
 package com.google.cloud.bigtable.grpc.async;
 
-import com.google.api.client.util.ExponentialBackOff;
 import com.google.api.client.util.NanoClock;
 import com.google.api.core.ApiClock;
-import com.google.cloud.bigtable.config.RetryOptions;
+import com.google.common.annotations.VisibleForTesting;
 import org.junit.Assert;
 import org.mockito.invocation.InvocationOnMock;
 import org.mockito.stubbing.Answer;
@@ -44,6 +43,11 @@ public class OperationClock implements ApiClock {
     this.timeNs = System.nanoTime();
   }
 
+  @VisibleForTesting
+  void setTime(long time, TimeUnit timeUnit) {
+    this.timeNs = timeUnit.toNanos(time);
+  }
+
   @Override
   public synchronized long nanoTime() {
     return timeNs + totalSleepTimeNs;
diff --git a/bigtable-client-core-parent/bigtable-client-core/src/test/java/com/google/cloud/bigtable/grpc/async/TestRetryingMutateRowsOperation.java b/bigtable-client-core-parent/bigtable-client-core/src/test/java/com/google/cloud/bigtable/grpc/async/TestRetryingMutateRowsOperation.java
index 6a7ca9b79..d82629414 100644
--- a/bigtable-client-core-parent/bigtable-client-core/src/test/java/com/google/cloud/bigtable/grpc/async/TestRetryingMutateRowsOperation.java
+++ b/bigtable-client-core-parent/bigtable-client-core/src/test/java/com/google/cloud/bigtable/grpc/async/TestRetryingMutateRowsOperation.java
@@ -161,6 +161,27 @@ public class TestRetryingMutateRowsOperation {
     checkResponse(future, createResponse(OK, OK));
   }
 
+  @Test
+  public void testRetryExhausted() throws Exception {
+    MutateRowsRequest request = createRequest(2);
+    RetryingMutateRowsOperation underTest = createOperation(request);
+    ListenableFuture<?> future = underTest.getAsyncResult();
+    MutateRowsRequest retryRequest = createRequest(request.getEntries(1));
+
+    send(underTest, OK, DEADLINE_EXCEEDED);
+    checkExecutor(1);
+    Assert.assertEquals(retryRequest, underTest.getRetryRequest());
+
+    // Fast forward until retry limit has been reached
+    clock.setTime(clock.millisTime() + RETRY_OPTIONS.getMaxElapsedBackoffMillis() + 1, TimeUnit.MILLISECONDS);
+
+    send(underTest, DEADLINE_EXCEEDED);
+    checkExecutor(1);
+    Assert.assertEquals(retryRequest, underTest.getRetryRequest());
+
+    checkResponse(future, createResponse(OK, DEADLINE_EXCEEDED));
+  }
+
   @Test
   public void testCompleteFailure() throws InterruptedException, TimeoutException {
     MutateRowsRequest request = createRequest(2);