JAVA-1548: Retry idempotent statements on READ_TIMEOUT and UNAVAILABLE

apache · Jul 24, 2017 · 293035b · 293035b
1 parent 06b37b3
commit 293035b
Show file tree

Hide file tree

Showing 3 changed files with 31 additions and 19 deletions.
diff --git a/changelog/README.md b/changelog/README.md
@@ -4,6 +4,7 @@
 
 ### 4.0.0-alpha1 (in progress)
 
+- [bug] JAVA-1548: Retry idempotent statements on READ_TIMEOUT and UNAVAILABLE
 - [bug] JAVA-1562: Fix various issues around heart beats
 - [improvement] JAVA-1546: Make all statement implementations immutable
 - [bug] JAVA-1554: Include VIEW and CDC in WriteType

diff --git a/core/src/main/java/com/datastax/oss/driver/internal/core/cql/CqlRequestHandler.java b/core/src/main/java/com/datastax/oss/driver/internal/core/cql/CqlRequestHandler.java
@@ -423,15 +423,13 @@ private void processErrorResponse(Error errorMessage) {
         if (error instanceof ReadTimeoutException) {
           ReadTimeoutException readTimeout = (ReadTimeoutException) error;
           decision =
-              isIdempotent
-                  ? retryPolicy.onReadTimeout(
-                      request,
-                      readTimeout.getConsistencyLevel(),
-                      readTimeout.getBlockFor(),
-                      readTimeout.getReceived(),
-                      readTimeout.wasDataPresent(),
-                      retryCount)
-                  : RetryDecision.RETHROW;
+              retryPolicy.onReadTimeout(
+                  request,
+                  readTimeout.getConsistencyLevel(),
+                  readTimeout.getBlockFor(),
+                  readTimeout.getReceived(),
+                  readTimeout.wasDataPresent(),
+                  retryCount);
         } else if (error instanceof WriteTimeoutException) {
           WriteTimeoutException writeTimeout = (WriteTimeoutException) error;
           decision =
@@ -447,14 +445,12 @@ private void processErrorResponse(Error errorMessage) {
         } else if (error instanceof UnavailableException) {
           UnavailableException unavailable = (UnavailableException) error;
           decision =
-              isIdempotent
-                  ? retryPolicy.onUnavailable(
-                      request,
-                      unavailable.getConsistencyLevel(),
-                      unavailable.getRequired(),
-                      unavailable.getAlive(),
-                      retryCount)
-                  : RetryDecision.RETHROW;
+              retryPolicy.onUnavailable(
+                  request,
+                  unavailable.getConsistencyLevel(),
+                  unavailable.getRequired(),
+                  unavailable.getAlive(),
+                  retryCount);
         } else {
           decision =
               isIdempotent

diff --git a/core/src/test/java/com/datastax/oss/driver/internal/core/cql/CqlRequestHandlerRetryTest.java b/core/src/test/java/com/datastax/oss/driver/internal/core/cql/CqlRequestHandlerRetryTest.java
@@ -237,14 +237,27 @@ public void should_rethrow_error_if_idempotent_and_retry_policy_decides_so(
 
   @Test
   @UseDataProvider("failureAndNotIdempotent")
-  public void should_rethrow_error_if_not_idempotent(
+  public void should_rethrow_error_if_not_idempotent_and_error_unsafe_or_policy_rethrows(
       FailureScenario failureScenario, boolean defaultIdempotence, SimpleStatement statement) {
+
+    // For two of the possible exceptions, the retry policy is called even if the statement is not
+    // idempotent
+    boolean shouldCallRetryPolicy =
+        (failureScenario.expectedExceptionClass.equals(UnavailableException.class)
+            || failureScenario.expectedExceptionClass.equals(ReadTimeoutException.class));
+
     RequestHandlerTestHarness.Builder harnessBuilder =
         RequestHandlerTestHarness.builder().withDefaultIdempotence(defaultIdempotence);
     failureScenario.mockRequestError(harnessBuilder, node1);
     harnessBuilder.withResponse(node2, defaultFrameOf(singleRow()));
 
     try (RequestHandlerTestHarness harness = harnessBuilder.build()) {
+
+      if (shouldCallRetryPolicy) {
+        failureScenario.mockRetryPolicyDecision(
+            harness.getContext().retryPolicy(), RetryDecision.RETHROW);
+      }
+
       CompletionStage<AsyncResultSet> resultSetFuture =
           new CqlRequestHandler(statement, harness.getSession(), harness.getContext(), "test")
               .asyncResult();
@@ -254,7 +267,9 @@ public void should_rethrow_error_if_not_idempotent(
               error -> {
                 assertThat(error).isInstanceOf(failureScenario.expectedExceptionClass);
                 // When non idempotent, the policy is bypassed completely:
-                Mockito.verifyNoMoreInteractions(harness.getContext().retryPolicy());
+                if (!shouldCallRetryPolicy) {
+                  Mockito.verifyNoMoreInteractions(harness.getContext().retryPolicy());
+                }
               });
     }
   }