Merge pull request #181 from ajkannan/add-retry-result-value

Add third RetryResult (instead of using null) to denote 'proceed'
googleapis · Sep 26, 2015 · 48458d3 · 48458d3
2 parents 0a71721 + 800b709
commit 48458d3
Show file tree

Hide file tree

Showing 4 changed files with 75 additions and 34 deletions.
diff --git a/gcloud-java-core/src/main/java/com/google/gcloud/ExceptionHandler.java b/gcloud-java-core/src/main/java/com/google/gcloud/ExceptionHandler.java
@@ -16,7 +16,6 @@
 
 package com.google.gcloud;
 
-import static com.google.common.base.MoreObjects.firstNonNull;
 import static com.google.common.base.Preconditions.checkNotNull;
 
 import com.google.common.annotations.VisibleForTesting;
@@ -48,27 +47,16 @@ public final class ExceptionHandler implements Serializable {
   public interface Interceptor extends Serializable {
 
     enum RetryResult {
-
-      RETRY(true), ABORT(false);
-
-      private final boolean booleanValue;
-
-      RetryResult(boolean booleanValue) {
-        this.booleanValue = booleanValue;
-      }
-
-      boolean booleanValue() {
-        return booleanValue;
-      }
+      NO_RETRY, RETRY, CONTINUE_EVALUATION;
     }
 
     /**
      * This method is called before exception evaluation and could short-circuit the process.
      *
      * @param exception the exception that is being evaluated
      * @return {@link RetryResult} to indicate if the exception should be ignored (
-     *         {@link RetryResult#RETRY}), propagated ({@link RetryResult#ABORT}), or evaluation
-     *         should proceed ({@code null}).
+     *         {@link RetryResult#RETRY}), propagated ({@link RetryResult#NO_RETRY}), or evaluation
+     *         should proceed ({@link RetryResult#CONTINUE_EVALUATION}).
      */
     RetryResult beforeEval(Exception exception);
 
@@ -78,8 +66,8 @@ boolean booleanValue() {
      * @param exception the exception that is being evaluated
      * @param retryResult the result of the evaluation so far.
      * @return {@link RetryResult} to indicate if the exception should be ignored (
-     *         {@link RetryResult#RETRY}), propagated ({@link RetryResult#ABORT}), or evaluation
-     *         should proceed ({@code null}).
+     *         {@link RetryResult#RETRY}), propagated ({@link RetryResult#NO_RETRY}), or evaluation
+     *         should proceed ({@link RetryResult#CONTINUE_EVALUATION}).
      */
     RetryResult afterEval(Exception exception, RetryResult retryResult);
   }
@@ -157,7 +145,7 @@ static final class RetryInfo implements Serializable {
 
     RetryInfo(Class<? extends Exception> exception, Interceptor.RetryResult retry) {
       this.exception = checkNotNull(exception);
-      this.retry = retry;
+      this.retry = checkNotNull(retry);
     }
 
     @Override
@@ -189,7 +177,7 @@ private ExceptionHandler(Builder builder) {
       addRetryInfo(new RetryInfo(exception, Interceptor.RetryResult.RETRY), retryInfo);
     }
     for (Class<? extends Exception> exception : nonRetriableExceptions) {
-      addRetryInfo(new RetryInfo(exception, Interceptor.RetryResult.ABORT), retryInfo);
+      addRetryInfo(new RetryInfo(exception, Interceptor.RetryResult.NO_RETRY), retryInfo);
     }
   }
 
@@ -253,18 +241,22 @@ public Set<Class<? extends Exception>> getNonRetriableExceptions() {
 
   boolean shouldRetry(Exception ex) {
     for (Interceptor interceptor : interceptors) {
-      Interceptor.RetryResult retryResult = interceptor.beforeEval(ex);
-      if (retryResult != null) {
-        return retryResult.booleanValue();
+      Interceptor.RetryResult retryResult = checkNotNull(interceptor.beforeEval(ex));
+      if (retryResult != Interceptor.RetryResult.CONTINUE_EVALUATION) {
+        return retryResult == Interceptor.RetryResult.RETRY;
       }
     }
     RetryInfo retryInfo = findMostSpecificRetryInfo(this.retryInfo, ex.getClass());
     Interceptor.RetryResult retryResult =
-        retryInfo == null ? Interceptor.RetryResult.ABORT : retryInfo.retry;
+        retryInfo == null ? Interceptor.RetryResult.NO_RETRY : retryInfo.retry;
     for (Interceptor interceptor : interceptors) {
-      retryResult = firstNonNull(interceptor.afterEval(ex, retryResult), retryResult);
+      Interceptor.RetryResult interceptorRetry = 
+          checkNotNull(interceptor.afterEval(ex, retryResult));
+      if (interceptorRetry != Interceptor.RetryResult.CONTINUE_EVALUATION) {
+        retryResult = interceptorRetry;
+      }
     }
-    return retryResult.booleanValue();
+    return retryResult == Interceptor.RetryResult.RETRY;
   }
 
   /**

diff --git a/gcloud-java-core/src/test/java/com/google/gcloud/ExceptionHandlerTest.java b/gcloud-java-core/src/test/java/com/google/gcloud/ExceptionHandlerTest.java
@@ -23,6 +23,8 @@
 import com.google.gcloud.ExceptionHandler.Interceptor;
 import com.google.gcloud.ExceptionHandler.Interceptor.RetryResult;
 
+import org.junit.rules.ExpectedException;
+import org.junit.Rule;
 import org.junit.Test;
 
 import java.io.FileNotFoundException;
@@ -36,6 +38,9 @@
  */
 public class ExceptionHandlerTest {
 
+  @Rule
+  public ExpectedException thrown = ExpectedException.none();
+
   @Test
   public void testVerifyCaller() {
     class A implements Callable<Object> {
@@ -128,13 +133,13 @@ public void testShouldTry() {
     assertFalse(handler.shouldRetry(new RuntimeException()));
     assertTrue(handler.shouldRetry(new NullPointerException()));
 
-    final AtomicReference<RetryResult> before = new AtomicReference<>(RetryResult.ABORT);
+    final AtomicReference<RetryResult> before = new AtomicReference<>(RetryResult.NO_RETRY);
     @SuppressWarnings("serial")
     Interceptor interceptor = new Interceptor() {
 
       @Override
       public RetryResult afterEval(Exception exception, RetryResult retryResult) {
-        return retryResult == RetryResult.ABORT ? RetryResult.RETRY : RetryResult.ABORT;
+        return retryResult == RetryResult.NO_RETRY ? RetryResult.RETRY : RetryResult.NO_RETRY;
       }
 
       @Override
@@ -158,11 +163,55 @@ public RetryResult beforeEval(Exception exception) {
     assertTrue(handler.shouldRetry(new RuntimeException()));
     assertTrue(handler.shouldRetry(new NullPointerException()));
 
-    before.set(null);
+    before.set(RetryResult.CONTINUE_EVALUATION);
     assertFalse(handler.shouldRetry(new IOException()));
     assertTrue(handler.shouldRetry(new ClosedByInterruptException()));
     assertTrue(handler.shouldRetry(new InterruptedException()));
     assertTrue(handler.shouldRetry(new RuntimeException()));
     assertFalse(handler.shouldRetry(new NullPointerException()));
   }
+
+  @Test
+  public void testNullRetryResultFromBeforeEval() {
+    @SuppressWarnings("serial")
+    Interceptor interceptor = new Interceptor() {
+
+      @Override
+      public RetryResult beforeEval(Exception exception) {
+        return null;
+      }
+
+      @Override
+      public RetryResult afterEval(Exception exception, RetryResult retryResult) {
+        return RetryResult.CONTINUE_EVALUATION;
+      }
+
+    };
+
+    ExceptionHandler handler = ExceptionHandler.builder().interceptor(interceptor).build();
+    thrown.expect(NullPointerException.class);
+    handler.shouldRetry(new Exception());
+  }
+
+  @Test
+  public void testNullRetryResultFromAfterEval() {
+    @SuppressWarnings("serial")
+    Interceptor interceptor = new Interceptor() {
+
+      @Override
+      public RetryResult beforeEval(Exception exception) {
+        return RetryResult.CONTINUE_EVALUATION;
+      }
+
+      @Override
+      public RetryResult afterEval(Exception exception, RetryResult retryResult) {
+        return null;
+      }
+
+    };
+
+    ExceptionHandler handler = ExceptionHandler.builder().interceptor(interceptor).build();
+    thrown.expect(NullPointerException.class);
+    handler.shouldRetry(new Exception());
+  }
 }
diff --git a/gcloud-java-datastore/src/main/java/com/google/gcloud/datastore/DatastoreImpl.java b/gcloud-java-datastore/src/main/java/com/google/gcloud/datastore/DatastoreImpl.java
@@ -53,16 +53,16 @@ final class DatastoreImpl extends BaseService<DatastoreOptions>
 
         @Override
         public RetryResult afterEval(Exception exception, RetryResult retryResult) {
-          return null;
+          return Interceptor.RetryResult.CONTINUE_EVALUATION;
         }
 
         @Override
         public RetryResult beforeEval(Exception exception) {
           if (exception instanceof DatastoreRpcException) {
             boolean retryable = ((DatastoreRpcException) exception).retryable();
-            return retryable ? Interceptor.RetryResult.RETRY : Interceptor.RetryResult.ABORT;
+            return retryable ? Interceptor.RetryResult.RETRY : Interceptor.RetryResult.NO_RETRY;
           }
-          return null;
+          return Interceptor.RetryResult.CONTINUE_EVALUATION;
         }
       };
   private static final ExceptionHandler EXCEPTION_HANDLER = ExceptionHandler.builder()

diff --git a/gcloud-java-storage/src/main/java/com/google/gcloud/storage/StorageImpl.java b/gcloud-java-storage/src/main/java/com/google/gcloud/storage/StorageImpl.java
@@ -72,16 +72,16 @@ final class StorageImpl extends BaseService<StorageOptions> implements Storage {
 
     @Override
     public RetryResult afterEval(Exception exception, RetryResult retryResult) {
-      return null;
+      return Interceptor.RetryResult.CONTINUE_EVALUATION;
     }
 
     @Override
     public RetryResult beforeEval(Exception exception) {
       if (exception instanceof StorageException) {
         boolean retriable = ((StorageException) exception).retryable();
-        return retriable ? Interceptor.RetryResult.RETRY : Interceptor.RetryResult.ABORT;
+        return retriable ? Interceptor.RetryResult.RETRY : Interceptor.RetryResult.NO_RETRY;
       }
-      return null;
+      return Interceptor.RetryResult.CONTINUE_EVALUATION;
     }
   };
   static final ExceptionHandler EXCEPTION_HANDLER = ExceptionHandler.builder()