From af6bbefa0191a940058f5170fdba148b425fa78d Mon Sep 17 00:00:00 2001 From: Ajay Kannan Date: Fri, 25 Sep 2015 10:43:36 -0700 Subject: [PATCH 1/2] Add third RetryResult (instead of using null) to denote 'proceed' --- .../com/google/gcloud/ExceptionHandler.java | 34 +++++-------- .../google/gcloud/ExceptionHandlerTest.java | 51 ++++++++++++++++++- .../gcloud/datastore/DatastoreImpl.java | 4 +- 3 files changed, 65 insertions(+), 24 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 index 412462ae156e..9e472965cb6e 100644 --- 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,18 +47,7 @@ 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; - } + ABORT, RETRY, PROCEED; } /** @@ -68,7 +56,7 @@ boolean booleanValue() { * @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}). + * should proceed ({@link RetryResult#PROCEED}). */ RetryResult beforeEval(Exception exception); @@ -79,7 +67,7 @@ boolean booleanValue() { * @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}). + * should proceed ({@link RetryResult#PROCEED}). */ RetryResult afterEval(Exception exception, RetryResult retryResult); } @@ -157,7 +145,7 @@ static final class RetryInfo implements Serializable { RetryInfo(Class exception, Interceptor.RetryResult retry) { this.exception = checkNotNull(exception); - this.retry = retry; + this.retry = checkNotNull(retry); } @Override @@ -253,18 +241,22 @@ public Set> 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.PROCEED) { + return interceptor.beforeEval(ex) == Interceptor.RetryResult.RETRY; } } RetryInfo retryInfo = findMostSpecificRetryInfo(this.retryInfo, ex.getClass()); Interceptor.RetryResult retryResult = retryInfo == null ? Interceptor.RetryResult.ABORT : 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.PROCEED) { + 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 index 3844f9de36d7..999cd06fac58 100644 --- a/gcloud-java-core/src/test/java/com/google/gcloud/ExceptionHandlerTest.java +++ b/gcloud-java-core/src/test/java/com/google/gcloud/ExceptionHandlerTest.java @@ -158,11 +158,60 @@ public RetryResult beforeEval(Exception exception) { assertTrue(handler.shouldRetry(new RuntimeException())); assertTrue(handler.shouldRetry(new NullPointerException())); - before.set(null); + before.set(RetryResult.PROCEED); 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 testNullRetryResult() { + @SuppressWarnings("serial") + Interceptor interceptor1 = new Interceptor() { + + @Override + public RetryResult beforeEval(Exception exception) { + return null; + } + + @Override + public RetryResult afterEval(Exception exception, RetryResult retryResult) { + return RetryResult.PROCEED; + } + + }; + + @SuppressWarnings("serial") + Interceptor interceptor2 = new Interceptor() { + + @Override + public RetryResult beforeEval(Exception exception) { + return RetryResult.PROCEED; + } + + @Override + public RetryResult afterEval(Exception exception, RetryResult retryResult) { + return null; + } + + }; + + ExceptionHandler handler1 = ExceptionHandler.builder().interceptor(interceptor1).build(); + try { + handler1.shouldRetry(new Exception()); + fail("Expected null pointer exception due to null RetryResult from beforeEval"); + } catch (NullPointerException e) { + // expected + } + + ExceptionHandler handler2 = ExceptionHandler.builder().interceptor(interceptor2).build(); + try { + handler2.shouldRetry(new Exception()); + fail("Expected null pointer exception due to null RetryResult from afterEval"); + } catch (NullPointerException e) { + // expected + } + } } 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 index 6f2454c62167..d330c31e706e 100644 --- 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,7 +53,7 @@ final class DatastoreImpl extends BaseService @Override public RetryResult afterEval(Exception exception, RetryResult retryResult) { - return null; + return Interceptor.RetryResult.PROCEED; } @Override @@ -62,7 +62,7 @@ public RetryResult beforeEval(Exception exception) { boolean retryable = ((DatastoreRpcException) exception).retryable(); return retryable ? Interceptor.RetryResult.RETRY : Interceptor.RetryResult.ABORT; } - return null; + return Interceptor.RetryResult.PROCEED; } }; private static final ExceptionHandler EXCEPTION_HANDLER = ExceptionHandler.builder() From 800b7090409203833b61cfaf4ec227eb26e1d82a Mon Sep 17 00:00:00 2001 From: Ajay Kannan Date: Fri, 25 Sep 2015 14:06:17 -0700 Subject: [PATCH 2/2] Rename RetryResult enum values, and update datastore and storage interceptor. --- .../com/google/gcloud/ExceptionHandler.java | 20 ++++---- .../google/gcloud/ExceptionHandlerTest.java | 46 +++++++++---------- .../gcloud/datastore/DatastoreImpl.java | 6 +-- .../google/gcloud/storage/StorageImpl.java | 6 +-- 4 files changed, 39 insertions(+), 39 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 index 9e472965cb6e..a0fab3dca566 100644 --- a/gcloud-java-core/src/main/java/com/google/gcloud/ExceptionHandler.java +++ b/gcloud-java-core/src/main/java/com/google/gcloud/ExceptionHandler.java @@ -47,7 +47,7 @@ public final class ExceptionHandler implements Serializable { public interface Interceptor extends Serializable { enum RetryResult { - ABORT, RETRY, PROCEED; + NO_RETRY, RETRY, CONTINUE_EVALUATION; } /** @@ -55,8 +55,8 @@ enum RetryResult { * * @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 ({@link RetryResult#PROCEED}). + * {@link RetryResult#RETRY}), propagated ({@link RetryResult#NO_RETRY}), or evaluation + * should proceed ({@link RetryResult#CONTINUE_EVALUATION}). */ RetryResult beforeEval(Exception exception); @@ -66,8 +66,8 @@ enum RetryResult { * @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 ({@link RetryResult#PROCEED}). + * {@link RetryResult#RETRY}), propagated ({@link RetryResult#NO_RETRY}), or evaluation + * should proceed ({@link RetryResult#CONTINUE_EVALUATION}). */ RetryResult afterEval(Exception exception, RetryResult retryResult); } @@ -177,7 +177,7 @@ private ExceptionHandler(Builder builder) { addRetryInfo(new RetryInfo(exception, Interceptor.RetryResult.RETRY), retryInfo); } for (Class exception : nonRetriableExceptions) { - addRetryInfo(new RetryInfo(exception, Interceptor.RetryResult.ABORT), retryInfo); + addRetryInfo(new RetryInfo(exception, Interceptor.RetryResult.NO_RETRY), retryInfo); } } @@ -242,17 +242,17 @@ public Set> getNonRetriableExceptions() { boolean shouldRetry(Exception ex) { for (Interceptor interceptor : interceptors) { Interceptor.RetryResult retryResult = checkNotNull(interceptor.beforeEval(ex)); - if (retryResult != Interceptor.RetryResult.PROCEED) { - return interceptor.beforeEval(ex) == Interceptor.RetryResult.RETRY; + 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) { Interceptor.RetryResult interceptorRetry = checkNotNull(interceptor.afterEval(ex, retryResult)); - if (interceptorRetry != Interceptor.RetryResult.PROCEED) { + if (interceptorRetry != Interceptor.RetryResult.CONTINUE_EVALUATION) { retryResult = interceptorRetry; } } 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 index 999cd06fac58..5ce05ad900a8 100644 --- 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 { @@ -128,13 +133,13 @@ public void testShouldTry() { assertFalse(handler.shouldRetry(new RuntimeException())); assertTrue(handler.shouldRetry(new NullPointerException())); - final AtomicReference before = new AtomicReference<>(RetryResult.ABORT); + final AtomicReference 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,7 +163,7 @@ public RetryResult beforeEval(Exception exception) { assertTrue(handler.shouldRetry(new RuntimeException())); assertTrue(handler.shouldRetry(new NullPointerException())); - before.set(RetryResult.PROCEED); + before.set(RetryResult.CONTINUE_EVALUATION); assertFalse(handler.shouldRetry(new IOException())); assertTrue(handler.shouldRetry(new ClosedByInterruptException())); assertTrue(handler.shouldRetry(new InterruptedException())); @@ -167,9 +172,9 @@ public RetryResult beforeEval(Exception exception) { } @Test - public void testNullRetryResult() { + public void testNullRetryResultFromBeforeEval() { @SuppressWarnings("serial") - Interceptor interceptor1 = new Interceptor() { + Interceptor interceptor = new Interceptor() { @Override public RetryResult beforeEval(Exception exception) { @@ -178,17 +183,24 @@ public RetryResult beforeEval(Exception exception) { @Override public RetryResult afterEval(Exception exception, RetryResult retryResult) { - return RetryResult.PROCEED; + 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 interceptor2 = new Interceptor() { + Interceptor interceptor = new Interceptor() { @Override public RetryResult beforeEval(Exception exception) { - return RetryResult.PROCEED; + return RetryResult.CONTINUE_EVALUATION; } @Override @@ -198,20 +210,8 @@ public RetryResult afterEval(Exception exception, RetryResult retryResult) { }; - ExceptionHandler handler1 = ExceptionHandler.builder().interceptor(interceptor1).build(); - try { - handler1.shouldRetry(new Exception()); - fail("Expected null pointer exception due to null RetryResult from beforeEval"); - } catch (NullPointerException e) { - // expected - } - - ExceptionHandler handler2 = ExceptionHandler.builder().interceptor(interceptor2).build(); - try { - handler2.shouldRetry(new Exception()); - fail("Expected null pointer exception due to null RetryResult from afterEval"); - } catch (NullPointerException e) { - // expected - } + 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 index d330c31e706e..c12ffc8a032d 100644 --- 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 @Override public RetryResult afterEval(Exception exception, RetryResult retryResult) { - return Interceptor.RetryResult.PROCEED; + 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 Interceptor.RetryResult.PROCEED; + 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 index f59c6c670969..403f8a7ab78a 100644 --- 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 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()