Skip to content

Commit

Permalink
Adressed PR comments.
Browse files Browse the repository at this point in the history
  • Loading branch information
vam-google committed Mar 20, 2017
1 parent f64baa1 commit f9a0b1f
Show file tree
Hide file tree
Showing 7 changed files with 84 additions and 78 deletions.
49 changes: 25 additions & 24 deletions google-cloud-core/src/main/java/com/google/cloud/BaseService.java
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@

package com.google.cloud;

import com.google.cloud.ExceptionHandler.Interceptor;
import com.google.cloud.HttpExceptionRetryAlgorithm.Interceptor;

/**
* Base class for service objects.
Expand All @@ -26,36 +26,37 @@
public abstract class BaseService<OptionsT extends ServiceOptions<?, OptionsT>>
implements Service<OptionsT> {

public static final Interceptor EXCEPTION_HANDLER_INTERCEPTOR = new Interceptor() {

private static final long serialVersionUID = -8429573486870467828L;

@Override
public RetryResult afterEval(Exception exception, RetryResult retryResult) {
return Interceptor.RetryResult.CONTINUE_EVALUATION;
}

@Override
public RetryResult beforeEval(Exception exception) {
if (exception instanceof BaseServiceException) {
boolean retriable = ((BaseServiceException) exception).isRetryable();
return retriable ? Interceptor.RetryResult.RETRY : Interceptor.RetryResult.NO_RETRY;
}
return Interceptor.RetryResult.CONTINUE_EVALUATION;
}
};
public static final ExceptionHandler EXCEPTION_HANDLER = ExceptionHandler.newBuilder()
.abortOn(RuntimeException.class)
.addInterceptors(EXCEPTION_HANDLER_INTERCEPTOR)
.build();
public static final Interceptor EXCEPTION_HANDLER_INTERCEPTOR =
new Interceptor() {

private static final long serialVersionUID = -8429573486870467828L;

@Override
public RetryResult afterEval(Exception exception, RetryResult retryResult) {
return Interceptor.RetryResult.CONTINUE_EVALUATION;
}

@Override
public RetryResult beforeEval(Exception exception) {
if (exception instanceof BaseServiceException) {
boolean retriable = ((BaseServiceException) exception).isRetryable();
return retriable ? Interceptor.RetryResult.RETRY : Interceptor.RetryResult.NO_RETRY;
}
return Interceptor.RetryResult.CONTINUE_EVALUATION;
}
};
public static final HttpExceptionRetryAlgorithm EXCEPTION_HANDLER =
HttpExceptionRetryAlgorithm.newBuilder()
.abortOn(RuntimeException.class)
.addInterceptors(EXCEPTION_HANDLER_INTERCEPTOR)
.build();

private final OptionsT options;

protected BaseService(OptionsT options) {
this.options = options;
}


@Override
public OptionsT getOptions() {
return options;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -33,13 +33,13 @@
import java.util.concurrent.Callable;

/**
* Exception handling used by {@link RetryHelper}.
* Exception retry algorithm implementation used by {@link RetryHelper}.
*/
public final class ExceptionHandler implements ExceptionRetryAlgorithm, Serializable {
public final class HttpExceptionRetryAlgorithm implements ExceptionRetryAlgorithm, Serializable {

private static final long serialVersionUID = -2460707015779532919L;

private static final ExceptionHandler DEFAULT_INSTANCE =
private static final HttpExceptionRetryAlgorithm DEFAULT_INSTANCE =
newBuilder().retryOn(Exception.class).abortOn(RuntimeException.class).build();

private final ImmutableList<Interceptor> interceptors;
Expand Down Expand Up @@ -76,7 +76,7 @@ enum RetryResult {
}

/**
* ExceptionHandler builder.
* HttpExceptionRetryAlgorithm builder.
*/
public static class Builder {

Expand Down Expand Up @@ -132,10 +132,10 @@ public final Builder abortOn(Class<? extends Exception>... exceptions) {
}

/**
* Returns a new ExceptionHandler instance.
* Returns a new HttpExceptionRetryAlgorithm instance.
*/
public ExceptionHandler build() {
return new ExceptionHandler(this);
public HttpExceptionRetryAlgorithm build() {
return new HttpExceptionRetryAlgorithm(this);
}
}

Expand Down Expand Up @@ -170,7 +170,7 @@ public boolean equals(Object obj) {
}
}

private ExceptionHandler(Builder builder) {
private HttpExceptionRetryAlgorithm(Builder builder) {
interceptors = builder.interceptors.build();
retriableExceptions = builder.retriableExceptions.build();
nonRetriableExceptions = builder.nonRetriableExceptions.build();
Expand Down Expand Up @@ -263,6 +263,8 @@ public boolean accept(Throwable prevThrowable) {
@Override
public TimedAttemptSettings createNextAttempt(Throwable prevThrowable,
TimedAttemptSettings prevSettings) {
// Return null to indicate that this implementaiton does not provide any specific attempt
// settings, so by default the TimedRetryAlgorithm options can be used instead.
return null;
}

Expand All @@ -276,10 +278,10 @@ public boolean equals(Object obj) {
if (obj == this) {
return true;
}
if (!(obj instanceof ExceptionHandler)) {
if (!(obj instanceof HttpExceptionRetryAlgorithm)) {
return false;
}
ExceptionHandler other = (ExceptionHandler) obj;
HttpExceptionRetryAlgorithm other = (HttpExceptionRetryAlgorithm) obj;
return Objects.equals(interceptors, other.interceptors)
&& Objects.equals(retriableExceptions, other.retriableExceptions)
&& Objects.equals(nonRetriableExceptions, other.nonRetriableExceptions)
Expand All @@ -290,7 +292,7 @@ public boolean equals(Object obj) {
/**
* Returns an instance which retry any checked exception and abort on any runtime exception.
*/
public static ExceptionHandler getDefaultInstance() {
public static HttpExceptionRetryAlgorithm getDefaultInstance() {
return DEFAULT_INSTANCE;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,18 +28,19 @@

/**
* Utility class for retrying operations. For more details about the parameters, see {@link
* RetrySettings}.
* RetrySettings}. In case if retrying is unsuccessful, {@link RetryHelperException} will be
* thrown.
*/
public class RetryHelper {
public static <V> V runWithRetries(
Callable<V> callable,
RetrySettings retrySettings,
ExceptionHandler exceptionHandler,
HttpExceptionRetryAlgorithm exceptionRetryAlgorithm,
ApiClock clock)
throws RetryHelperException {
try {
RetryAlgorithm retryAlgorithm =
new RetryAlgorithm(exceptionHandler, new ExponentialRetryAlgorithm(retrySettings, clock));
new RetryAlgorithm(exceptionRetryAlgorithm, new ExponentialRetryAlgorithm(retrySettings, clock));
RetryingExecutor<V> executor = new DirectRetryingExecutor<>(retryAlgorithm);

RetryingFuture<V> retryingFuture = executor.createFuture(callable);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,8 @@
import static org.junit.Assert.assertTrue;
import static org.junit.Assert.fail;

import com.google.cloud.ExceptionHandler.Interceptor;
import com.google.cloud.ExceptionHandler.Interceptor.RetryResult;
import com.google.cloud.HttpExceptionRetryAlgorithm.Interceptor;
import com.google.cloud.HttpExceptionRetryAlgorithm.Interceptor.RetryResult;

import org.junit.Rule;
import org.junit.Test;
Expand All @@ -34,9 +34,9 @@
import java.util.concurrent.atomic.AtomicReference;

/**
* Tests for {@link ExceptionHandler}.
* Tests for {@link HttpExceptionRetryAlgorithm}.
*/
public class ExceptionHandlerTest {
public class HttpExceptionRetryAlgorithmTest {

@Rule
public ExpectedException thrown = ExpectedException.none();
Expand Down Expand Up @@ -82,15 +82,15 @@ public Object call() throws Error {
}

// using default exception handler (retry upon any non-runtime exceptions)
ExceptionHandler handler = ExceptionHandler.getDefaultInstance();
HttpExceptionRetryAlgorithm handler = HttpExceptionRetryAlgorithm.getDefaultInstance();
assertValidCallable(new A(), handler);
assertValidCallable(new B(), handler);
assertValidCallable(new C(), handler);
assertValidCallable(new D(), handler);
assertValidCallable(new E(), handler);
assertInvalidCallable(new F(), handler);

handler = ExceptionHandler.newBuilder()
handler = HttpExceptionRetryAlgorithm.newBuilder()
.retryOn(FileNotFoundException.class, NullPointerException.class)
.build();
assertInvalidCallable(new A(), handler);
Expand All @@ -101,11 +101,11 @@ public Object call() throws Error {
assertInvalidCallable(new F(), handler);
}

private static <T> void assertValidCallable(Callable<T> callable, ExceptionHandler handler) {
private static <T> void assertValidCallable(Callable<T> callable, HttpExceptionRetryAlgorithm handler) {
handler.verifyCaller(callable);
}

private static <T> void assertInvalidCallable(Callable<T> callable, ExceptionHandler handler) {
private static <T> void assertInvalidCallable(Callable<T> callable, HttpExceptionRetryAlgorithm handler) {
try {
handler.verifyCaller(callable);
fail("Expected RetryHelper constructor to fail");
Expand All @@ -116,12 +116,12 @@ private static <T> void assertInvalidCallable(Callable<T> callable, ExceptionHan

@Test
public void testShouldTry() {
ExceptionHandler handler = ExceptionHandler.newBuilder().retryOn(IOException.class).build();
HttpExceptionRetryAlgorithm handler = HttpExceptionRetryAlgorithm.newBuilder().retryOn(IOException.class).build();
assertTrue(handler.accept(new IOException()));
assertTrue(handler.accept(new ClosedByInterruptException()));
assertFalse(handler.accept(new RuntimeException()));

ExceptionHandler.Builder builder = ExceptionHandler.newBuilder()
HttpExceptionRetryAlgorithm.Builder builder = HttpExceptionRetryAlgorithm.newBuilder()
.retryOn(IOException.class, NullPointerException.class)
.abortOn(RuntimeException.class, ClosedByInterruptException.class,
InterruptedException.class);
Expand Down Expand Up @@ -188,7 +188,7 @@ public RetryResult afterEval(Exception exception, RetryResult retryResult) {

};

ExceptionHandler handler = ExceptionHandler.newBuilder().addInterceptors(interceptor).build();
HttpExceptionRetryAlgorithm handler = HttpExceptionRetryAlgorithm.newBuilder().addInterceptors(interceptor).build();
thrown.expect(NullPointerException.class);
handler.accept(new Exception());
}
Expand All @@ -210,7 +210,7 @@ public RetryResult afterEval(Exception exception, RetryResult retryResult) {

};

ExceptionHandler handler = ExceptionHandler.newBuilder().addInterceptors(interceptor).build();
HttpExceptionRetryAlgorithm handler = HttpExceptionRetryAlgorithm.newBuilder().addInterceptors(interceptor).build();
thrown.expect(NullPointerException.class);
handler.accept(new Exception());
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,8 @@ public class SerializationTest extends BaseSerializationTest {

private static final BaseServiceException BASE_SERVICE_EXCEPTION =
new BaseServiceException(42, "message", "reason", true);
private static final ExceptionHandler EXCEPTION_HANDLER = ExceptionHandler.getDefaultInstance();
private static final HttpExceptionRetryAlgorithm EXCEPTION_HANDLER =
HttpExceptionRetryAlgorithm.getDefaultInstance();
private static final Identity IDENTITY = Identity.allAuthenticatedUsers();
private static final PageImpl<String> PAGE =
new PageImpl<>(null, "cursor", ImmutableList.of("string1", "string2"));
Expand All @@ -50,31 +51,31 @@ public class SerializationTest extends BaseSerializationTest {
.build();
private static final String JSON_KEY =
"{\n"
+ " \"private_key_id\": \"somekeyid\",\n"
+ " \"private_key\": \"-----BEGIN PRIVATE KEY-----\\nMIIEvgIBADANBgkqhkiG9w0BAQEFAASCBKgwggS"
+ "kAgEAAoIBAQC+K2hSuFpAdrJI\\nnCgcDz2M7t7bjdlsadsasad+fvRSW6TjNQZ3p5LLQY1kSZRqBqylRkzteMOyHg"
+ "aR\\n0Pmxh3ILCND5men43j3h4eDbrhQBuxfEMalkG92sL+PNQSETY2tnvXryOvmBRwa/\\nQP/9dJfIkIDJ9Fw9N4"
+ "Bhhhp6mCcRpdQjV38H7JsyJ7lih/oNjECgYAt\\nknddadwkwewcVxHFhcZJO+XWf6ofLUXpRwiTZakGMn8EE1uVa2"
+ "LgczOjwWHGi99MFjxSer5m9\\n1tCa3/KEGKiS/YL71JvjwX3mb+cewlkcmweBKZHM2JPTk0ZednFSpVZMtycjkbLa"
+ "\\ndYOS8V85AgMBewECggEBAKksaldajfDZDV6nGqbFjMiizAKJolr/M3OQw16K6o3/\\n0S31xIe3sSlgW0+UbYlF"
+ "4U8KifhManD1apVSC3csafaspP4RZUHFhtBywLO9pR5c\\nr6S5aLp+gPWFyIp1pfXbWGvc5VY/v9x7ya1VEa6rXvL"
+ "sKupSeWAW4tMj3eo/64ge\\nsdaceaLYw52KeBYiT6+vpsnYrEkAHO1fF/LavbLLOFJmFTMxmsNaG0tuiJHgjshB\\"
+ "n82DpMCbXG9YcCgI/DbzuIjsdj2JC1cascSP//3PmefWysucBQe7Jryb6NQtASmnv\\nCdDw/0jmZTEjpe4S1lxfHp"
+ "lAhHFtdgYTvyYtaLZiVVkCgYEA8eVpof2rceecw/I6\\n5ng1q3Hl2usdWV/4mZMvR0fOemacLLfocX6IYxT1zA1FF"
+ "JlbXSRsJMf/Qq39mOR2\\nSpW+hr4jCoHeRVYLgsbggtrevGmILAlNoqCMpGZ6vDmJpq6ECV9olliDvpPgWOP+\\nm"
+ "YPDreFBGxWvQrADNbRt2dmGsrsCgYEAyUHqB2wvJHFqdmeBsaacewzV8x9WgmeX\\ngUIi9REwXlGDW0Mz50dxpxcK"
+ "CAYn65+7TCnY5O/jmL0VRxU1J2mSWyWTo1C+17L0\\n3fUqjxL1pkefwecxwecvC+gFFYdJ4CQ/MHHXU81Lwl1iWdF"
+ "Cd2UoGddYaOF+KNeM\\nHC7cmqra+JsCgYEAlUNywzq8nUg7282E+uICfCB0LfwejuymR93CtsFgb7cRd6ak\\nECR"
+ "8FGfCpH8ruWJINllbQfcHVCX47ndLZwqv3oVFKh6pAS/vVI4dpOepP8++7y1u\\ncoOvtreXCX6XqfrWDtKIvv0vjl"
+ "HBhhhp6mCcRpdQjV38H7JsyJ7lih/oNjECgYAt\\nkndj5uNl5SiuVxHFhcZJO+XWf6ofLUregtevZakGMn8EE1uVa"
+ "2AY7eafmoU/nZPT\\n00YB0TBATdCbn/nBSuKDESkhSg9s2GEKQZG5hBmL5uCMfo09z3SfxZIhJdlerreP\\nJ7gSi"
+ "dI12N+EZxYd4xIJh/HFDgp7RRO87f+WJkofMQKBgGTnClK1VMaCRbJZPriw\\nEfeFCoOX75MxKwXs6xgrw4W//AYG"
+ "GUjDt83lD6AZP6tws7gJ2IwY/qP7+lyhjEqN\\nHtfPZRGFkGZsdaksdlaksd323423d+15/UvrlRSFPNj1tWQmNKk"
+ "XyRDW4IG1Oa2p\\nrALStNBx5Y9t0/LQnFI4w3aG\\n-----END PRIVATE KEY-----\\n\",\n"
+ " \"client_email\": \"[email protected]\",\n"
+ " \"client_id\": \"someclientid.apps.googleusercontent.com\",\n"
+ " \"type\": \"service_account\"\n"
+ "}";
+ " \"private_key_id\": \"somekeyid\",\n"
+ " \"private_key\": \"-----BEGIN PRIVATE KEY-----\\nMIIEvgIBADANBgkqhkiG9w0BAQEFAASCBKgwggS"
+ "kAgEAAoIBAQC+K2hSuFpAdrJI\\nnCgcDz2M7t7bjdlsadsasad+fvRSW6TjNQZ3p5LLQY1kSZRqBqylRkzteMOyHg"
+ "aR\\n0Pmxh3ILCND5men43j3h4eDbrhQBuxfEMalkG92sL+PNQSETY2tnvXryOvmBRwa/\\nQP/9dJfIkIDJ9Fw9N4"
+ "Bhhhp6mCcRpdQjV38H7JsyJ7lih/oNjECgYAt\\nknddadwkwewcVxHFhcZJO+XWf6ofLUXpRwiTZakGMn8EE1uVa2"
+ "LgczOjwWHGi99MFjxSer5m9\\n1tCa3/KEGKiS/YL71JvjwX3mb+cewlkcmweBKZHM2JPTk0ZednFSpVZMtycjkbLa"
+ "\\ndYOS8V85AgMBewECggEBAKksaldajfDZDV6nGqbFjMiizAKJolr/M3OQw16K6o3/\\n0S31xIe3sSlgW0+UbYlF"
+ "4U8KifhManD1apVSC3csafaspP4RZUHFhtBywLO9pR5c\\nr6S5aLp+gPWFyIp1pfXbWGvc5VY/v9x7ya1VEa6rXvL"
+ "sKupSeWAW4tMj3eo/64ge\\nsdaceaLYw52KeBYiT6+vpsnYrEkAHO1fF/LavbLLOFJmFTMxmsNaG0tuiJHgjshB\\"
+ "n82DpMCbXG9YcCgI/DbzuIjsdj2JC1cascSP//3PmefWysucBQe7Jryb6NQtASmnv\\nCdDw/0jmZTEjpe4S1lxfHp"
+ "lAhHFtdgYTvyYtaLZiVVkCgYEA8eVpof2rceecw/I6\\n5ng1q3Hl2usdWV/4mZMvR0fOemacLLfocX6IYxT1zA1FF"
+ "JlbXSRsJMf/Qq39mOR2\\nSpW+hr4jCoHeRVYLgsbggtrevGmILAlNoqCMpGZ6vDmJpq6ECV9olliDvpPgWOP+\\nm"
+ "YPDreFBGxWvQrADNbRt2dmGsrsCgYEAyUHqB2wvJHFqdmeBsaacewzV8x9WgmeX\\ngUIi9REwXlGDW0Mz50dxpxcK"
+ "CAYn65+7TCnY5O/jmL0VRxU1J2mSWyWTo1C+17L0\\n3fUqjxL1pkefwecxwecvC+gFFYdJ4CQ/MHHXU81Lwl1iWdF"
+ "Cd2UoGddYaOF+KNeM\\nHC7cmqra+JsCgYEAlUNywzq8nUg7282E+uICfCB0LfwejuymR93CtsFgb7cRd6ak\\nECR"
+ "8FGfCpH8ruWJINllbQfcHVCX47ndLZwqv3oVFKh6pAS/vVI4dpOepP8++7y1u\\ncoOvtreXCX6XqfrWDtKIvv0vjl"
+ "HBhhhp6mCcRpdQjV38H7JsyJ7lih/oNjECgYAt\\nkndj5uNl5SiuVxHFhcZJO+XWf6ofLUregtevZakGMn8EE1uVa"
+ "2AY7eafmoU/nZPT\\n00YB0TBATdCbn/nBSuKDESkhSg9s2GEKQZG5hBmL5uCMfo09z3SfxZIhJdlerreP\\nJ7gSi"
+ "dI12N+EZxYd4xIJh/HFDgp7RRO87f+WJkofMQKBgGTnClK1VMaCRbJZPriw\\nEfeFCoOX75MxKwXs6xgrw4W//AYG"
+ "GUjDt83lD6AZP6tws7gJ2IwY/qP7+lyhjEqN\\nHtfPZRGFkGZsdaksdlaksd323423d+15/UvrlRSFPNj1tWQmNKk"
+ "XyRDW4IG1Oa2p\\nrALStNBx5Y9t0/LQnFI4w3aG\\n-----END PRIVATE KEY-----\\n\",\n"
+ " \"client_email\": \"[email protected]\",\n"
+ " \"client_id\": \"someclientid.apps.googleusercontent.com\",\n"
+ " \"type\": \"service_account\"\n"
+ "}";

@Override
protected Serializable[] serializableObjects() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -100,7 +100,8 @@ public class TranslateImplTest {
private TranslateRpc translateRpcMock;
private Translate translate;

@Rule public ExpectedException thrown = ExpectedException.none();
@Rule
public ExpectedException thrown = ExpectedException.none();

@Before
public void setUp() {
Expand Down
2 changes: 1 addition & 1 deletion pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,7 @@
<github.global.server>github</github.global.server>
<google.auth.version>0.6.0</google.auth.version>
<grpc.version>1.0.3</grpc.version>
<gax.version>0.3.2-SNAPSHOT</gax.version>
<gax.version>0.5.0</gax.version>
<generatedProto.version>0.1.5</generatedProto.version>
<core.version>0.10.1-alpha-SNAPSHOT</core.version>
<beta.version>0.10.1-beta-SNAPSHOT</beta.version>
Expand Down

0 comments on commit f9a0b1f

Please sign in to comment.