From 2e8d0247821d04567ed16a6751ae62de760a8788 Mon Sep 17 00:00:00 2001 From: Igor Bernstein Date: Mon, 7 Jan 2019 20:18:45 -0500 Subject: [PATCH 1/8] WIP: Add annotations for retries This instruments the retry logic to notify the tracer of the retry lifecycle. It also fixed a bug in the ApiTracer interface that prevented exhausted retries from passing the error. --- .../api/gax/retrying/BasicRetryingFuture.java | 13 +++++++++++++ .../com/google/api/gax/rpc/AttemptCallable.java | 5 +++++ .../google/api/gax/rpc/CheckingAttemptCallable.java | 5 +++++ .../api/gax/rpc/ServerStreamingAttemptCallable.java | 4 ++++ .../java/com/google/api/gax/tracing/ApiTracer.java | 4 +++- .../com/google/api/gax/tracing/NoopApiTracer.java | 2 +- 6 files changed, 31 insertions(+), 2 deletions(-) diff --git a/gax/src/main/java/com/google/api/gax/retrying/BasicRetryingFuture.java b/gax/src/main/java/com/google/api/gax/retrying/BasicRetryingFuture.java index 331a1e304..8516b78d7 100644 --- a/gax/src/main/java/com/google/api/gax/retrying/BasicRetryingFuture.java +++ b/gax/src/main/java/com/google/api/gax/retrying/BasicRetryingFuture.java @@ -142,9 +142,11 @@ void handleAttempt(Throwable throwable, ResponseT response) { clearAttemptServiceData(); if (throwable instanceof CancellationException) { // An attempt triggered cancellation. + retryingContext.getTracer().attemptFailedRetriesExhausted(throwable); super.cancel(false); } else if (throwable instanceof RejectedExecutionException) { // external executor cannot continue retrying + retryingContext.getTracer().attemptPermanentFailure(throwable); super.setException(throwable); } if (isDone()) { @@ -155,21 +157,32 @@ void handleAttempt(Throwable throwable, ResponseT response) { retryAlgorithm.createNextAttempt(throwable, response, attemptSettings); boolean shouldRetry = retryAlgorithm.shouldRetry(throwable, response, nextAttemptSettings); if (shouldRetry) { + retryingContext + .getTracer() + .attemptFailed(throwable, nextAttemptSettings.getRandomizedRetryDelay()); attemptSettings = nextAttemptSettings; setAttemptResult(throwable, response, true); // a new attempt will be (must be) scheduled by an external executor } else if (throwable != null) { + if (retryAlgorithm.getResultAlgorithm().shouldRetry(throwable, response)) { + retryingContext.getTracer().attemptFailedRetriesExhausted(throwable); + } else { + retryingContext.getTracer().attemptPermanentFailure(throwable); + } super.setException(throwable); } else { + retryingContext.getTracer().attemptSucceeded(); super.set(response); } } catch (CancellationException e) { // A retry algorithm triggered cancellation. + retryingContext.getTracer().attemptFailedRetriesExhausted(e); super.cancel(false); } catch (Exception e) { // Should never happen, but still possible in case of buggy retry algorithm implementation. // Any bugs/exceptions (except CancellationException) in retry algorithms immediately // terminate retrying future and set the result to the thrown exception. + retryingContext.getTracer().attemptPermanentFailure(e); super.setException(e); } } diff --git a/gax/src/main/java/com/google/api/gax/rpc/AttemptCallable.java b/gax/src/main/java/com/google/api/gax/rpc/AttemptCallable.java index b0ac657e5..dbd9c995c 100644 --- a/gax/src/main/java/com/google/api/gax/rpc/AttemptCallable.java +++ b/gax/src/main/java/com/google/api/gax/rpc/AttemptCallable.java @@ -78,6 +78,11 @@ public ResponseT call() { if (externalFuture.isDone()) { return null; } + + callContext + .getTracer() + .attemptStarted(externalFuture.getAttemptSettings().getOverallAttemptCount()); + ApiFuture internalFuture = callable.futureCall(request, callContext); externalFuture.setAttemptFuture(internalFuture); } catch (Throwable e) { diff --git a/gax/src/main/java/com/google/api/gax/rpc/CheckingAttemptCallable.java b/gax/src/main/java/com/google/api/gax/rpc/CheckingAttemptCallable.java index 85c2976fa..17670a5e7 100644 --- a/gax/src/main/java/com/google/api/gax/rpc/CheckingAttemptCallable.java +++ b/gax/src/main/java/com/google/api/gax/rpc/CheckingAttemptCallable.java @@ -75,6 +75,11 @@ public ResponseT call() { if (externalFuture.isDone()) { return null; } + + callContext + .getTracer() + .attemptStarted(externalFuture.getAttemptSettings().getOverallAttemptCount()); + // NOTE: The callable here is an OperationCheckingCallable, which will compose its own // request using a resolved operation name and ignore anything that we pass here for the // request. diff --git a/gax/src/main/java/com/google/api/gax/rpc/ServerStreamingAttemptCallable.java b/gax/src/main/java/com/google/api/gax/rpc/ServerStreamingAttemptCallable.java index 61dab03f5..d94885b3b 100644 --- a/gax/src/main/java/com/google/api/gax/rpc/ServerStreamingAttemptCallable.java +++ b/gax/src/main/java/com/google/api/gax/rpc/ServerStreamingAttemptCallable.java @@ -220,6 +220,10 @@ public Void call() { outerRetryingFuture.getAttemptSettings().getRpcTimeout()); } + attemptContext + .getTracer() + .attemptStarted(outerRetryingFuture.getAttemptSettings().getOverallAttemptCount()); + innerCallable.call( request, new StateCheckingResponseObserver() { diff --git a/gax/src/main/java/com/google/api/gax/tracing/ApiTracer.java b/gax/src/main/java/com/google/api/gax/tracing/ApiTracer.java index 8a137b10b..98010e889 100644 --- a/gax/src/main/java/com/google/api/gax/tracing/ApiTracer.java +++ b/gax/src/main/java/com/google/api/gax/tracing/ApiTracer.java @@ -93,8 +93,10 @@ public interface ApiTracer { /** * Adds an annotation that the attempt failed and that no further attempts will be made because * retry limits have been reached. + * + * @param error the last error received before retries were exhausted. */ - void attemptFailedRetriesExhausted(); + void attemptFailedRetriesExhausted(Throwable error); /** * Adds an annotation that the attempt failed and that no further attempts will be made because diff --git a/gax/src/main/java/com/google/api/gax/tracing/NoopApiTracer.java b/gax/src/main/java/com/google/api/gax/tracing/NoopApiTracer.java index 4b569b7f9..173aa2070 100644 --- a/gax/src/main/java/com/google/api/gax/tracing/NoopApiTracer.java +++ b/gax/src/main/java/com/google/api/gax/tracing/NoopApiTracer.java @@ -91,7 +91,7 @@ public void attemptFailed(Throwable error, Duration delay) { } @Override - public void attemptFailedRetriesExhausted() { + public void attemptFailedRetriesExhausted(Throwable error) { // noop } From 0d2ee976b1ccf996f271b40386aaca836aa44a8d Mon Sep 17 00:00:00 2001 From: Igor Bernstein Date: Mon, 7 Jan 2019 20:24:23 -0500 Subject: [PATCH 2/8] fix tests --- .../api/gax/retrying/AbstractRetryingExecutorTest.java | 10 ++++++++-- .../google/api/gax/rpc/testing/FakeCallContext.java | 4 ++++ 2 files changed, 12 insertions(+), 2 deletions(-) diff --git a/gax/src/test/java/com/google/api/gax/retrying/AbstractRetryingExecutorTest.java b/gax/src/test/java/com/google/api/gax/retrying/AbstractRetryingExecutorTest.java index 08f6562eb..212098810 100644 --- a/gax/src/test/java/com/google/api/gax/retrying/AbstractRetryingExecutorTest.java +++ b/gax/src/test/java/com/google/api/gax/retrying/AbstractRetryingExecutorTest.java @@ -40,20 +40,21 @@ import com.google.api.core.ApiFuture; import com.google.api.core.NanoClock; import com.google.api.gax.retrying.FailingCallable.CustomException; +import com.google.api.gax.rpc.testing.FakeCallContext; import java.util.concurrent.CancellationException; import java.util.concurrent.ExecutionException; import java.util.concurrent.TimeUnit; import java.util.concurrent.TimeoutException; import java.util.concurrent.atomic.AtomicInteger; +import org.junit.Before; import org.junit.Test; import org.junit.runner.RunWith; -import org.mockito.Mock; import org.mockito.junit.MockitoJUnitRunner; import org.threeten.bp.Duration; @RunWith(MockitoJUnitRunner.class) public abstract class AbstractRetryingExecutorTest { - @Mock protected RetryingContext retryingContext; + protected RetryingContext retryingContext; protected abstract RetryingExecutorWithContext getExecutor( RetryAlgorithm retryAlgorithm); @@ -61,6 +62,11 @@ protected abstract RetryingExecutorWithContext getExecutor( protected abstract RetryAlgorithm getAlgorithm( RetrySettings retrySettings, int apocalypseCountDown, RuntimeException apocalypseException); + @Before + public void setUp() { + retryingContext = FakeCallContext.createDefault(); + } + @Test public void testSuccess() throws Exception { FailingCallable callable = new FailingCallable(0, "SUCCESS"); diff --git a/gax/src/test/java/com/google/api/gax/rpc/testing/FakeCallContext.java b/gax/src/test/java/com/google/api/gax/rpc/testing/FakeCallContext.java index aa2a7dcb9..61f668573 100644 --- a/gax/src/test/java/com/google/api/gax/rpc/testing/FakeCallContext.java +++ b/gax/src/test/java/com/google/api/gax/rpc/testing/FakeCallContext.java @@ -35,6 +35,7 @@ import com.google.api.gax.rpc.TransportChannel; import com.google.api.gax.rpc.internal.Headers; import com.google.api.gax.tracing.ApiTracer; +import com.google.api.gax.tracing.NoopApiTracer; import com.google.auth.Credentials; import com.google.common.base.Preconditions; import com.google.common.collect.ImmutableMap; @@ -276,6 +277,9 @@ public Map> getExtraHeaders() { @Override @Nonnull public ApiTracer getTracer() { + if (tracer == null) { + return NoopApiTracer.getInstance(); + } return tracer; } From d452c18e02502d467977f27da3daa098bc204052 Mon Sep 17 00:00:00 2001 From: Igor Bernstein Date: Tue, 8 Jan 2019 15:30:04 -0500 Subject: [PATCH 3/8] add tests --- .../AbstractRetryingExecutorTest.java | 80 ++++++++++++++++--- .../api/gax/retrying/FailingCallable.java | 12 ++- .../ScheduledRetryingExecutorTest.java | 10 +-- 3 files changed, 83 insertions(+), 19 deletions(-) diff --git a/gax/src/test/java/com/google/api/gax/retrying/AbstractRetryingExecutorTest.java b/gax/src/test/java/com/google/api/gax/retrying/AbstractRetryingExecutorTest.java index 212098810..2097d680a 100644 --- a/gax/src/test/java/com/google/api/gax/retrying/AbstractRetryingExecutorTest.java +++ b/gax/src/test/java/com/google/api/gax/retrying/AbstractRetryingExecutorTest.java @@ -41,19 +41,28 @@ import com.google.api.core.NanoClock; import com.google.api.gax.retrying.FailingCallable.CustomException; import com.google.api.gax.rpc.testing.FakeCallContext; +import com.google.api.gax.tracing.ApiTracer; import java.util.concurrent.CancellationException; import java.util.concurrent.ExecutionException; import java.util.concurrent.TimeUnit; import java.util.concurrent.TimeoutException; import java.util.concurrent.atomic.AtomicInteger; import org.junit.Before; +import org.junit.Rule; import org.junit.Test; import org.junit.runner.RunWith; -import org.mockito.junit.MockitoJUnitRunner; +import org.junit.runners.JUnit4; +import org.mockito.Mock; +import org.mockito.Mockito; +import org.mockito.junit.MockitoJUnit; +import org.mockito.junit.MockitoRule; import org.threeten.bp.Duration; -@RunWith(MockitoJUnitRunner.class) +@RunWith(JUnit4.class) public abstract class AbstractRetryingExecutorTest { + @Rule public final MockitoRule mockitoRule = MockitoJUnit.rule(); + + @Mock protected ApiTracer tracer; protected RetryingContext retryingContext; protected abstract RetryingExecutorWithContext getExecutor( @@ -64,12 +73,12 @@ protected abstract RetryAlgorithm getAlgorithm( @Before public void setUp() { - retryingContext = FakeCallContext.createDefault(); + retryingContext = FakeCallContext.createDefault().withTracer(tracer); } @Test public void testSuccess() throws Exception { - FailingCallable callable = new FailingCallable(0, "SUCCESS"); + FailingCallable callable = new FailingCallable(tracer, 0, "SUCCESS"); RetryingExecutorWithContext executor = getExecutor(getAlgorithm(FAST_RETRY_SETTINGS, 0, null)); RetryingFuture future = executor.createFuture(callable, retryingContext); @@ -77,11 +86,15 @@ public void testSuccess() throws Exception { assertFutureSuccess(future); assertEquals(0, future.getAttemptSettings().getAttemptCount()); + + Mockito.verify(tracer, Mockito.times(1)).attemptStarted(0); + Mockito.verify(tracer, Mockito.times(1)).attemptSucceeded(); + Mockito.verifyNoMoreInteractions(tracer); } @Test public void testSuccessWithFailures() throws Exception { - FailingCallable callable = new FailingCallable(5, "SUCCESS"); + FailingCallable callable = new FailingCallable(tracer, 5, "SUCCESS"); RetryingExecutorWithContext executor = getExecutor(getAlgorithm(FAST_RETRY_SETTINGS, 0, null)); RetryingFuture future = executor.createFuture(callable, retryingContext); @@ -89,11 +102,17 @@ public void testSuccessWithFailures() throws Exception { assertFutureSuccess(future); assertEquals(5, future.getAttemptSettings().getAttemptCount()); + + Mockito.verify(tracer, Mockito.times(6)).attemptStarted(Mockito.anyInt()); + Mockito.verify(tracer, Mockito.times(5)) + .attemptFailed(Mockito.any(Throwable.class), Mockito.any(Duration.class)); + Mockito.verify(tracer, Mockito.times(1)).attemptSucceeded(); + Mockito.verifyNoMoreInteractions(tracer); } @Test public void testSuccessWithFailuresPeekGetAttempt() throws Exception { - FailingCallable callable = new FailingCallable(5, "SUCCESS"); + FailingCallable callable = new FailingCallable(tracer, 5, "SUCCESS"); RetryingExecutorWithContext executor = getExecutor(getAlgorithm(FAST_RETRY_SETTINGS, 0, null)); RetryingFuture future = executor.createFuture(callable, retryingContext); @@ -119,7 +138,7 @@ public void testSuccessWithFailuresPeekGetAttempt() throws Exception { @Test public void testMaxRetriesExceeded() throws Exception { - FailingCallable callable = new FailingCallable(6, "FAILURE"); + FailingCallable callable = new FailingCallable(tracer, 6, "FAILURE"); RetryingExecutorWithContext executor = getExecutor(getAlgorithm(FAST_RETRY_SETTINGS, 0, null)); RetryingFuture future = executor.createFuture(callable, retryingContext); @@ -127,6 +146,13 @@ public void testMaxRetriesExceeded() throws Exception { assertFutureFail(future, CustomException.class); assertEquals(5, future.getAttemptSettings().getAttemptCount()); + + Mockito.verify(tracer, Mockito.times(6)).attemptStarted(Mockito.anyInt()); + Mockito.verify(tracer, Mockito.times(5)) + .attemptFailed(Mockito.any(Throwable.class), Mockito.any(Duration.class)); + Mockito.verify(tracer, Mockito.times(1)) + .attemptFailedRetriesExhausted(Mockito.any(Throwable.class)); + Mockito.verifyNoMoreInteractions(tracer); } @Test @@ -139,17 +165,22 @@ public void testTotalTimeoutExceeded() throws Exception { .build(); RetryingExecutorWithContext executor = getExecutor(getAlgorithm(retrySettings, 0, null)); - FailingCallable callable = new FailingCallable(6, "FAILURE"); + FailingCallable callable = new FailingCallable(tracer, 6, "FAILURE"); RetryingFuture future = executor.createFuture(callable, retryingContext); future.setAttemptFuture(executor.submit(future)); assertFutureFail(future, CustomException.class); assertTrue(future.getAttemptSettings().getAttemptCount() < 4); + + Mockito.verify(tracer, Mockito.times(1)).attemptStarted(Mockito.anyInt()); + Mockito.verify(tracer, Mockito.times(1)) + .attemptFailedRetriesExhausted(Mockito.any(Throwable.class)); + Mockito.verifyNoMoreInteractions(tracer); } @Test public void testCancelOuterFutureBeforeStart() throws Exception { - FailingCallable callable = new FailingCallable(4, "SUCCESS"); + FailingCallable callable = new FailingCallable(tracer, 4, "SUCCESS"); RetrySettings retrySettings = FAST_RETRY_SETTINGS @@ -169,11 +200,13 @@ public void testCancelOuterFutureBeforeStart() throws Exception { assertFutureCancel(future); assertEquals(0, future.getAttemptSettings().getAttemptCount()); + + Mockito.verifyNoMoreInteractions(tracer); } @Test public void testCancelByRetryingAlgorithm() throws Exception { - FailingCallable callable = new FailingCallable(6, "FAILURE"); + FailingCallable callable = new FailingCallable(tracer, 6, "FAILURE"); RetryingExecutorWithContext executor = getExecutor(getAlgorithm(FAST_RETRY_SETTINGS, 5, new CancellationException())); RetryingFuture future = executor.createFuture(callable, retryingContext); @@ -181,11 +214,20 @@ public void testCancelByRetryingAlgorithm() throws Exception { assertFutureCancel(future); assertEquals(4, future.getAttemptSettings().getAttemptCount()); + + Mockito.verify(tracer, Mockito.times(5)).attemptStarted(Mockito.anyInt()); + // Pre-apocalypse failures + Mockito.verify(tracer, Mockito.times(4)) + .attemptFailed(Mockito.any(Throwable.class), Mockito.any(Duration.class)); + // Apocalypse failure + Mockito.verify(tracer, Mockito.times(1)) + .attemptFailedRetriesExhausted(Mockito.any(CancellationException.class)); + Mockito.verifyNoMoreInteractions(tracer); } @Test public void testUnexpectedExceptionFromRetryAlgorithm() throws Exception { - FailingCallable callable = new FailingCallable(6, "FAILURE"); + FailingCallable callable = new FailingCallable(tracer, 6, "FAILURE"); RetryingExecutorWithContext executor = getExecutor(getAlgorithm(FAST_RETRY_SETTINGS, 5, new RuntimeException())); RetryingFuture future = executor.createFuture(callable, retryingContext); @@ -193,6 +235,15 @@ public void testUnexpectedExceptionFromRetryAlgorithm() throws Exception { assertFutureFail(future, RuntimeException.class); assertEquals(4, future.getAttemptSettings().getAttemptCount()); + + Mockito.verify(tracer, Mockito.times(5)).attemptStarted(Mockito.anyInt()); + // Pre-apocalypse failures + Mockito.verify(tracer, Mockito.times(4)) + .attemptFailed(Mockito.any(Throwable.class), Mockito.any(Duration.class)); + // Apocalypse failure + Mockito.verify(tracer, Mockito.times(1)) + .attemptPermanentFailure(Mockito.any(RuntimeException.class)); + Mockito.verifyNoMoreInteractions(tracer); } @Test @@ -210,12 +261,17 @@ public void testPollExceptionByPollAlgorithm() throws Exception { new ExponentialPollAlgorithm(retrySettings, NanoClock.getDefaultClock())); RetryingExecutorWithContext executor = getExecutor(retryAlgorithm); - FailingCallable callable = new FailingCallable(6, "FAILURE"); + FailingCallable callable = new FailingCallable(tracer, 6, "FAILURE"); RetryingFuture future = executor.createFuture(callable, retryingContext); future.setAttemptFuture(executor.submit(future)); assertFutureFail(future, PollException.class); assertTrue(future.getAttemptSettings().getAttemptCount() < 4); + + Mockito.verify(tracer, Mockito.times(1)).attemptStarted(Mockito.anyInt()); + Mockito.verify(tracer, Mockito.times(1)) + .attemptPermanentFailure(Mockito.any(PollException.class)); + Mockito.verifyNoMoreInteractions(tracer); } protected static class TestResultRetryAlgorithm diff --git a/gax/src/test/java/com/google/api/gax/retrying/FailingCallable.java b/gax/src/test/java/com/google/api/gax/retrying/FailingCallable.java index 037b15c89..48499d2da 100644 --- a/gax/src/test/java/com/google/api/gax/retrying/FailingCallable.java +++ b/gax/src/test/java/com/google/api/gax/retrying/FailingCallable.java @@ -29,6 +29,7 @@ */ package com.google.api.gax.retrying; +import com.google.api.gax.tracing.ApiTracer; import java.util.concurrent.Callable; import java.util.concurrent.atomic.AtomicInteger; import org.threeten.bp.Duration; @@ -48,19 +49,26 @@ class FailingCallable implements Callable { .build(); private AtomicInteger attemptsCount = new AtomicInteger(0); + private final ApiTracer tracer; private final int expectedFailuresCount; private final String result; - FailingCallable(int expectedFailuresCount, String result) { + FailingCallable(ApiTracer tracer, int expectedFailuresCount, String result) { + this.tracer = tracer; this.expectedFailuresCount = expectedFailuresCount; this.result = result; } @Override public String call() throws Exception { - if (attemptsCount.getAndIncrement() < expectedFailuresCount) { + int attemptNumber = attemptsCount.getAndIncrement(); + + tracer.attemptStarted(attemptNumber); + + if (attemptNumber < expectedFailuresCount) { throw new CustomException(); } + return result; } diff --git a/gax/src/test/java/com/google/api/gax/retrying/ScheduledRetryingExecutorTest.java b/gax/src/test/java/com/google/api/gax/retrying/ScheduledRetryingExecutorTest.java index 2d4708871..ba35d47c7 100644 --- a/gax/src/test/java/com/google/api/gax/retrying/ScheduledRetryingExecutorTest.java +++ b/gax/src/test/java/com/google/api/gax/retrying/ScheduledRetryingExecutorTest.java @@ -89,7 +89,7 @@ public void testSuccessWithFailuresPeekAttempt() throws Exception { final int maxRetries = 100; ScheduledExecutorService localExecutor = Executors.newSingleThreadScheduledExecutor(); - FailingCallable callable = new FailingCallable(15, "SUCCESS"); + FailingCallable callable = new FailingCallable(tracer, 15, "SUCCESS"); RetrySettings retrySettings = FAST_RETRY_SETTINGS @@ -139,7 +139,7 @@ public void testSuccessWithFailuresGetAttempt() throws Exception { final int maxRetries = 100; ScheduledExecutorService localExecutor = Executors.newSingleThreadScheduledExecutor(); - FailingCallable callable = new FailingCallable(15, "SUCCESS"); + FailingCallable callable = new FailingCallable(tracer, 15, "SUCCESS"); RetrySettings retrySettings = FAST_RETRY_SETTINGS .toBuilder() @@ -191,7 +191,7 @@ public void testCancelGetAttempt() throws Exception { ScheduledExecutorService localExecutor = Executors.newSingleThreadScheduledExecutor(); final int maxRetries = 100; - FailingCallable callable = new FailingCallable(maxRetries - 1, "SUCCESS"); + FailingCallable callable = new FailingCallable(tracer, maxRetries - 1, "SUCCESS"); RetrySettings retrySettings = FAST_RETRY_SETTINGS .toBuilder() @@ -247,7 +247,7 @@ public void testCancelGetAttempt() throws Exception { public void testCancelOuterFutureAfterStart() throws Exception { for (int executionsCount = 0; executionsCount < EXECUTIONS_COUNT; executionsCount++) { ScheduledExecutorService localExecutor = Executors.newSingleThreadScheduledExecutor(); - FailingCallable callable = new FailingCallable(4, "SUCCESS"); + FailingCallable callable = new FailingCallable(tracer, 4, "SUCCESS"); RetrySettings retrySettings = FAST_RETRY_SETTINGS .toBuilder() @@ -275,7 +275,7 @@ public void testCancelProxiedFutureAfterStart() throws Exception { // this is a heavy test, which takes a lot of time, so only few executions. for (int executionsCount = 0; executionsCount < 2; executionsCount++) { ScheduledExecutorService localExecutor = Executors.newSingleThreadScheduledExecutor(); - FailingCallable callable = new FailingCallable(5, "SUCCESS"); + FailingCallable callable = new FailingCallable(tracer, 5, "SUCCESS"); RetrySettings retrySettings = FAST_RETRY_SETTINGS .toBuilder() From 4fea657509e75e28178d175438217cab0eb9e362 Mon Sep 17 00:00:00 2001 From: Igor Bernstein Date: Wed, 9 Jan 2019 22:54:56 -0500 Subject: [PATCH 4/8] address some feedback --- .../api/gax/retrying/BasicRetryingFuture.java | 20 ++++++++++--------- 1 file changed, 11 insertions(+), 9 deletions(-) diff --git a/gax/src/main/java/com/google/api/gax/retrying/BasicRetryingFuture.java b/gax/src/main/java/com/google/api/gax/retrying/BasicRetryingFuture.java index 8516b78d7..3bfa4d002 100644 --- a/gax/src/main/java/com/google/api/gax/retrying/BasicRetryingFuture.java +++ b/gax/src/main/java/com/google/api/gax/retrying/BasicRetryingFuture.java @@ -34,6 +34,7 @@ import com.google.api.core.ApiFuture; import com.google.api.core.ApiFutures; +import com.google.api.gax.tracing.ApiTracer; import com.google.common.util.concurrent.AbstractFuture; import com.google.common.util.concurrent.MoreExecutors; import java.util.concurrent.Callable; @@ -137,16 +138,18 @@ void clearAttemptServiceData() { // "super." is used here to avoid infinite loops of callback chains void handleAttempt(Throwable throwable, ResponseT response) { + ApiTracer tracer = retryingContext.getTracer(); + synchronized (lock) { try { clearAttemptServiceData(); if (throwable instanceof CancellationException) { // An attempt triggered cancellation. - retryingContext.getTracer().attemptFailedRetriesExhausted(throwable); + tracer.attemptFailedRetriesExhausted(throwable); super.cancel(false); } else if (throwable instanceof RejectedExecutionException) { // external executor cannot continue retrying - retryingContext.getTracer().attemptPermanentFailure(throwable); + tracer.attemptPermanentFailure(throwable); super.setException(throwable); } if (isDone()) { @@ -157,32 +160,31 @@ void handleAttempt(Throwable throwable, ResponseT response) { retryAlgorithm.createNextAttempt(throwable, response, attemptSettings); boolean shouldRetry = retryAlgorithm.shouldRetry(throwable, response, nextAttemptSettings); if (shouldRetry) { - retryingContext - .getTracer() + tracer .attemptFailed(throwable, nextAttemptSettings.getRandomizedRetryDelay()); attemptSettings = nextAttemptSettings; setAttemptResult(throwable, response, true); // a new attempt will be (must be) scheduled by an external executor } else if (throwable != null) { if (retryAlgorithm.getResultAlgorithm().shouldRetry(throwable, response)) { - retryingContext.getTracer().attemptFailedRetriesExhausted(throwable); + tracer.attemptFailedRetriesExhausted(throwable); } else { - retryingContext.getTracer().attemptPermanentFailure(throwable); + tracer.attemptPermanentFailure(throwable); } super.setException(throwable); } else { - retryingContext.getTracer().attemptSucceeded(); + tracer.attemptSucceeded(); super.set(response); } } catch (CancellationException e) { // A retry algorithm triggered cancellation. - retryingContext.getTracer().attemptFailedRetriesExhausted(e); + tracer.attemptFailedRetriesExhausted(e); super.cancel(false); } catch (Exception e) { // Should never happen, but still possible in case of buggy retry algorithm implementation. // Any bugs/exceptions (except CancellationException) in retry algorithms immediately // terminate retrying future and set the result to the thrown exception. - retryingContext.getTracer().attemptPermanentFailure(e); + tracer.attemptPermanentFailure(e); super.setException(e); } } From 48f264db27e42d8b46e6d81a935b17864c1062b7 Mon Sep 17 00:00:00 2001 From: Igor Bernstein Date: Thu, 10 Jan 2019 16:23:44 -0500 Subject: [PATCH 5/8] Trace user attempt cancellation --- .../api/gax/retrying/BasicRetryingFuture.java | 10 ++++--- .../com/google/api/gax/tracing/ApiTracer.java | 3 +++ .../google/api/gax/tracing/NoopApiTracer.java | 5 ++++ .../ScheduledRetryingExecutorTest.java | 27 +++++++++++++++++++ 4 files changed, 42 insertions(+), 3 deletions(-) diff --git a/gax/src/main/java/com/google/api/gax/retrying/BasicRetryingFuture.java b/gax/src/main/java/com/google/api/gax/retrying/BasicRetryingFuture.java index 3bfa4d002..5608b7449 100644 --- a/gax/src/main/java/com/google/api/gax/retrying/BasicRetryingFuture.java +++ b/gax/src/main/java/com/google/api/gax/retrying/BasicRetryingFuture.java @@ -145,7 +145,12 @@ void handleAttempt(Throwable throwable, ResponseT response) { clearAttemptServiceData(); if (throwable instanceof CancellationException) { // An attempt triggered cancellation. - tracer.attemptFailedRetriesExhausted(throwable); + // In almost all cases, the operation caller caused the attempt to trigger the + // cancellation by invoking cancel() on the CallbackChainRetryingFuture, which cancelled + // the current attempt. + // In a theoretical scenario, the attempt callable might've thrown the exception on its + // own volition. However it's currently impossible to disambiguate the 2 scenarios. + tracer.attemptCancelled(); super.cancel(false); } else if (throwable instanceof RejectedExecutionException) { // external executor cannot continue retrying @@ -160,8 +165,7 @@ void handleAttempt(Throwable throwable, ResponseT response) { retryAlgorithm.createNextAttempt(throwable, response, attemptSettings); boolean shouldRetry = retryAlgorithm.shouldRetry(throwable, response, nextAttemptSettings); if (shouldRetry) { - tracer - .attemptFailed(throwable, nextAttemptSettings.getRandomizedRetryDelay()); + tracer.attemptFailed(throwable, nextAttemptSettings.getRandomizedRetryDelay()); attemptSettings = nextAttemptSettings; setAttemptResult(throwable, response, true); // a new attempt will be (must be) scheduled by an external executor diff --git a/gax/src/main/java/com/google/api/gax/tracing/ApiTracer.java b/gax/src/main/java/com/google/api/gax/tracing/ApiTracer.java index 98010e889..92cdf51e5 100644 --- a/gax/src/main/java/com/google/api/gax/tracing/ApiTracer.java +++ b/gax/src/main/java/com/google/api/gax/tracing/ApiTracer.java @@ -82,6 +82,9 @@ public interface ApiTracer { /** Adds an annotation that the attempt succeeded. */ void attemptSucceeded(); + /** Add an annotation that the attempt was cancelled by the user. */ + void attemptCancelled(); + /** * Adds an annotation that the attempt failed, but another attempt will be made after the delay. * diff --git a/gax/src/main/java/com/google/api/gax/tracing/NoopApiTracer.java b/gax/src/main/java/com/google/api/gax/tracing/NoopApiTracer.java index 173aa2070..8b8bc2d62 100644 --- a/gax/src/main/java/com/google/api/gax/tracing/NoopApiTracer.java +++ b/gax/src/main/java/com/google/api/gax/tracing/NoopApiTracer.java @@ -85,6 +85,11 @@ public void attemptSucceeded() { // noop } + @Override + public void attemptCancelled() { + // noop + } + @Override public void attemptFailed(Throwable error, Duration delay) { // noop diff --git a/gax/src/test/java/com/google/api/gax/retrying/ScheduledRetryingExecutorTest.java b/gax/src/test/java/com/google/api/gax/retrying/ScheduledRetryingExecutorTest.java index ba35d47c7..8bf83bef6 100644 --- a/gax/src/test/java/com/google/api/gax/retrying/ScheduledRetryingExecutorTest.java +++ b/gax/src/test/java/com/google/api/gax/retrying/ScheduledRetryingExecutorTest.java @@ -49,6 +49,7 @@ import org.junit.After; import org.junit.Test; import org.junit.runner.RunWith; +import org.mockito.Mockito; import org.mockito.junit.MockitoJUnitRunner; import org.threeten.bp.Duration; @@ -270,6 +271,32 @@ public void testCancelOuterFutureAfterStart() throws Exception { } } + @Test + public void testCancelIsTraced() throws Exception { + ScheduledExecutorService localExecutor = Executors.newSingleThreadScheduledExecutor(); + FailingCallable callable = new FailingCallable(tracer, 4, "SUCCESS"); + RetrySettings retrySettings = + FAST_RETRY_SETTINGS + .toBuilder() + .setInitialRetryDelay(Duration.ofMillis(1_000L)) + .setMaxRetryDelay(Duration.ofMillis(1_000L)) + .setTotalTimeout(Duration.ofMillis(10_0000L)) + .build(); + RetryingExecutorWithContext executor = + getRetryingExecutor(getAlgorithm(retrySettings, 0, null), localExecutor); + RetryingFuture future = executor.createFuture(callable, retryingContext); + future.setAttemptFuture(executor.submit(future)); + + Thread.sleep(30L); + + boolean res = future.cancel(false); + assertTrue(res); + assertFutureCancel(future); + + Mockito.verify(tracer).attemptCancelled(); + localExecutor.shutdownNow(); + } + @Test public void testCancelProxiedFutureAfterStart() throws Exception { // this is a heavy test, which takes a lot of time, so only few executions. From ce203efeaa68e4b409eb522c7a084ce663a47cef Mon Sep 17 00:00:00 2001 From: Igor Bernstein Date: Thu, 10 Jan 2019 16:25:55 -0500 Subject: [PATCH 6/8] static imports for mockito --- .../AbstractRetryingExecutorTest.java | 76 ++++++++++--------- 1 file changed, 40 insertions(+), 36 deletions(-) diff --git a/gax/src/test/java/com/google/api/gax/retrying/AbstractRetryingExecutorTest.java b/gax/src/test/java/com/google/api/gax/retrying/AbstractRetryingExecutorTest.java index 2097d680a..85a8a7078 100644 --- a/gax/src/test/java/com/google/api/gax/retrying/AbstractRetryingExecutorTest.java +++ b/gax/src/test/java/com/google/api/gax/retrying/AbstractRetryingExecutorTest.java @@ -36,6 +36,11 @@ import static org.junit.Assert.assertNull; import static org.junit.Assert.assertSame; import static org.junit.Assert.assertTrue; +import static org.mockito.Mockito.any; +import static org.mockito.Mockito.anyInt; +import static org.mockito.Mockito.times; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.verifyNoMoreInteractions; import com.google.api.core.ApiFuture; import com.google.api.core.NanoClock; @@ -53,7 +58,6 @@ import org.junit.runner.RunWith; import org.junit.runners.JUnit4; import org.mockito.Mock; -import org.mockito.Mockito; import org.mockito.junit.MockitoJUnit; import org.mockito.junit.MockitoRule; import org.threeten.bp.Duration; @@ -87,9 +91,9 @@ public void testSuccess() throws Exception { assertFutureSuccess(future); assertEquals(0, future.getAttemptSettings().getAttemptCount()); - Mockito.verify(tracer, Mockito.times(1)).attemptStarted(0); - Mockito.verify(tracer, Mockito.times(1)).attemptSucceeded(); - Mockito.verifyNoMoreInteractions(tracer); + verify(tracer, times(1)).attemptStarted(0); + verify(tracer, times(1)).attemptSucceeded(); + verifyNoMoreInteractions(tracer); } @Test @@ -103,11 +107,11 @@ public void testSuccessWithFailures() throws Exception { assertFutureSuccess(future); assertEquals(5, future.getAttemptSettings().getAttemptCount()); - Mockito.verify(tracer, Mockito.times(6)).attemptStarted(Mockito.anyInt()); - Mockito.verify(tracer, Mockito.times(5)) - .attemptFailed(Mockito.any(Throwable.class), Mockito.any(Duration.class)); - Mockito.verify(tracer, Mockito.times(1)).attemptSucceeded(); - Mockito.verifyNoMoreInteractions(tracer); + verify(tracer, times(6)).attemptStarted(anyInt()); + verify(tracer, times(5)) + .attemptFailed(any(Throwable.class), any(Duration.class)); + verify(tracer, times(1)).attemptSucceeded(); + verifyNoMoreInteractions(tracer); } @Test @@ -147,12 +151,12 @@ public void testMaxRetriesExceeded() throws Exception { assertFutureFail(future, CustomException.class); assertEquals(5, future.getAttemptSettings().getAttemptCount()); - Mockito.verify(tracer, Mockito.times(6)).attemptStarted(Mockito.anyInt()); - Mockito.verify(tracer, Mockito.times(5)) - .attemptFailed(Mockito.any(Throwable.class), Mockito.any(Duration.class)); - Mockito.verify(tracer, Mockito.times(1)) - .attemptFailedRetriesExhausted(Mockito.any(Throwable.class)); - Mockito.verifyNoMoreInteractions(tracer); + verify(tracer, times(6)).attemptStarted(anyInt()); + verify(tracer, times(5)) + .attemptFailed(any(Throwable.class), any(Duration.class)); + verify(tracer, times(1)) + .attemptFailedRetriesExhausted(any(Throwable.class)); + verifyNoMoreInteractions(tracer); } @Test @@ -172,10 +176,10 @@ public void testTotalTimeoutExceeded() throws Exception { assertFutureFail(future, CustomException.class); assertTrue(future.getAttemptSettings().getAttemptCount() < 4); - Mockito.verify(tracer, Mockito.times(1)).attemptStarted(Mockito.anyInt()); - Mockito.verify(tracer, Mockito.times(1)) - .attemptFailedRetriesExhausted(Mockito.any(Throwable.class)); - Mockito.verifyNoMoreInteractions(tracer); + verify(tracer, times(1)).attemptStarted(anyInt()); + verify(tracer, times(1)) + .attemptFailedRetriesExhausted(any(Throwable.class)); + verifyNoMoreInteractions(tracer); } @Test @@ -201,7 +205,7 @@ public void testCancelOuterFutureBeforeStart() throws Exception { assertFutureCancel(future); assertEquals(0, future.getAttemptSettings().getAttemptCount()); - Mockito.verifyNoMoreInteractions(tracer); + verifyNoMoreInteractions(tracer); } @Test @@ -215,14 +219,14 @@ public void testCancelByRetryingAlgorithm() throws Exception { assertFutureCancel(future); assertEquals(4, future.getAttemptSettings().getAttemptCount()); - Mockito.verify(tracer, Mockito.times(5)).attemptStarted(Mockito.anyInt()); + verify(tracer, times(5)).attemptStarted(anyInt()); // Pre-apocalypse failures - Mockito.verify(tracer, Mockito.times(4)) - .attemptFailed(Mockito.any(Throwable.class), Mockito.any(Duration.class)); + verify(tracer, times(4)) + .attemptFailed(any(Throwable.class), any(Duration.class)); // Apocalypse failure - Mockito.verify(tracer, Mockito.times(1)) - .attemptFailedRetriesExhausted(Mockito.any(CancellationException.class)); - Mockito.verifyNoMoreInteractions(tracer); + verify(tracer, times(1)) + .attemptFailedRetriesExhausted(any(CancellationException.class)); + verifyNoMoreInteractions(tracer); } @Test @@ -236,14 +240,14 @@ public void testUnexpectedExceptionFromRetryAlgorithm() throws Exception { assertFutureFail(future, RuntimeException.class); assertEquals(4, future.getAttemptSettings().getAttemptCount()); - Mockito.verify(tracer, Mockito.times(5)).attemptStarted(Mockito.anyInt()); + verify(tracer, times(5)).attemptStarted(anyInt()); // Pre-apocalypse failures - Mockito.verify(tracer, Mockito.times(4)) - .attemptFailed(Mockito.any(Throwable.class), Mockito.any(Duration.class)); + verify(tracer, times(4)) + .attemptFailed(any(Throwable.class), any(Duration.class)); // Apocalypse failure - Mockito.verify(tracer, Mockito.times(1)) - .attemptPermanentFailure(Mockito.any(RuntimeException.class)); - Mockito.verifyNoMoreInteractions(tracer); + verify(tracer, times(1)) + .attemptPermanentFailure(any(RuntimeException.class)); + verifyNoMoreInteractions(tracer); } @Test @@ -268,10 +272,10 @@ public void testPollExceptionByPollAlgorithm() throws Exception { assertFutureFail(future, PollException.class); assertTrue(future.getAttemptSettings().getAttemptCount() < 4); - Mockito.verify(tracer, Mockito.times(1)).attemptStarted(Mockito.anyInt()); - Mockito.verify(tracer, Mockito.times(1)) - .attemptPermanentFailure(Mockito.any(PollException.class)); - Mockito.verifyNoMoreInteractions(tracer); + verify(tracer, times(1)).attemptStarted(anyInt()); + verify(tracer, times(1)) + .attemptPermanentFailure(any(PollException.class)); + verifyNoMoreInteractions(tracer); } protected static class TestResultRetryAlgorithm From 5f1056dd362814e67e3476d8506c028e3348bd93 Mon Sep 17 00:00:00 2001 From: Igor Bernstein Date: Thu, 10 Jan 2019 16:26:51 -0500 Subject: [PATCH 7/8] make tracer last param in test helper --- .../retrying/AbstractRetryingExecutorTest.java | 18 +++++++++--------- .../api/gax/retrying/FailingCallable.java | 2 +- .../ScheduledRetryingExecutorTest.java | 12 ++++++------ 3 files changed, 16 insertions(+), 16 deletions(-) diff --git a/gax/src/test/java/com/google/api/gax/retrying/AbstractRetryingExecutorTest.java b/gax/src/test/java/com/google/api/gax/retrying/AbstractRetryingExecutorTest.java index 85a8a7078..0d9adb052 100644 --- a/gax/src/test/java/com/google/api/gax/retrying/AbstractRetryingExecutorTest.java +++ b/gax/src/test/java/com/google/api/gax/retrying/AbstractRetryingExecutorTest.java @@ -82,7 +82,7 @@ public void setUp() { @Test public void testSuccess() throws Exception { - FailingCallable callable = new FailingCallable(tracer, 0, "SUCCESS"); + FailingCallable callable = new FailingCallable(0, "SUCCESS", tracer); RetryingExecutorWithContext executor = getExecutor(getAlgorithm(FAST_RETRY_SETTINGS, 0, null)); RetryingFuture future = executor.createFuture(callable, retryingContext); @@ -98,7 +98,7 @@ public void testSuccess() throws Exception { @Test public void testSuccessWithFailures() throws Exception { - FailingCallable callable = new FailingCallable(tracer, 5, "SUCCESS"); + FailingCallable callable = new FailingCallable(5, "SUCCESS", tracer); RetryingExecutorWithContext executor = getExecutor(getAlgorithm(FAST_RETRY_SETTINGS, 0, null)); RetryingFuture future = executor.createFuture(callable, retryingContext); @@ -116,7 +116,7 @@ public void testSuccessWithFailures() throws Exception { @Test public void testSuccessWithFailuresPeekGetAttempt() throws Exception { - FailingCallable callable = new FailingCallable(tracer, 5, "SUCCESS"); + FailingCallable callable = new FailingCallable(5, "SUCCESS", tracer); RetryingExecutorWithContext executor = getExecutor(getAlgorithm(FAST_RETRY_SETTINGS, 0, null)); RetryingFuture future = executor.createFuture(callable, retryingContext); @@ -142,7 +142,7 @@ public void testSuccessWithFailuresPeekGetAttempt() throws Exception { @Test public void testMaxRetriesExceeded() throws Exception { - FailingCallable callable = new FailingCallable(tracer, 6, "FAILURE"); + FailingCallable callable = new FailingCallable(6, "FAILURE", tracer); RetryingExecutorWithContext executor = getExecutor(getAlgorithm(FAST_RETRY_SETTINGS, 0, null)); RetryingFuture future = executor.createFuture(callable, retryingContext); @@ -169,7 +169,7 @@ public void testTotalTimeoutExceeded() throws Exception { .build(); RetryingExecutorWithContext executor = getExecutor(getAlgorithm(retrySettings, 0, null)); - FailingCallable callable = new FailingCallable(tracer, 6, "FAILURE"); + FailingCallable callable = new FailingCallable(6, "FAILURE", tracer); RetryingFuture future = executor.createFuture(callable, retryingContext); future.setAttemptFuture(executor.submit(future)); @@ -184,7 +184,7 @@ public void testTotalTimeoutExceeded() throws Exception { @Test public void testCancelOuterFutureBeforeStart() throws Exception { - FailingCallable callable = new FailingCallable(tracer, 4, "SUCCESS"); + FailingCallable callable = new FailingCallable(4, "SUCCESS", tracer); RetrySettings retrySettings = FAST_RETRY_SETTINGS @@ -210,7 +210,7 @@ public void testCancelOuterFutureBeforeStart() throws Exception { @Test public void testCancelByRetryingAlgorithm() throws Exception { - FailingCallable callable = new FailingCallable(tracer, 6, "FAILURE"); + FailingCallable callable = new FailingCallable(6, "FAILURE", tracer); RetryingExecutorWithContext executor = getExecutor(getAlgorithm(FAST_RETRY_SETTINGS, 5, new CancellationException())); RetryingFuture future = executor.createFuture(callable, retryingContext); @@ -231,7 +231,7 @@ public void testCancelByRetryingAlgorithm() throws Exception { @Test public void testUnexpectedExceptionFromRetryAlgorithm() throws Exception { - FailingCallable callable = new FailingCallable(tracer, 6, "FAILURE"); + FailingCallable callable = new FailingCallable(6, "FAILURE", tracer); RetryingExecutorWithContext executor = getExecutor(getAlgorithm(FAST_RETRY_SETTINGS, 5, new RuntimeException())); RetryingFuture future = executor.createFuture(callable, retryingContext); @@ -265,7 +265,7 @@ public void testPollExceptionByPollAlgorithm() throws Exception { new ExponentialPollAlgorithm(retrySettings, NanoClock.getDefaultClock())); RetryingExecutorWithContext executor = getExecutor(retryAlgorithm); - FailingCallable callable = new FailingCallable(tracer, 6, "FAILURE"); + FailingCallable callable = new FailingCallable(6, "FAILURE", tracer); RetryingFuture future = executor.createFuture(callable, retryingContext); future.setAttemptFuture(executor.submit(future)); diff --git a/gax/src/test/java/com/google/api/gax/retrying/FailingCallable.java b/gax/src/test/java/com/google/api/gax/retrying/FailingCallable.java index 48499d2da..3cc9e9f1d 100644 --- a/gax/src/test/java/com/google/api/gax/retrying/FailingCallable.java +++ b/gax/src/test/java/com/google/api/gax/retrying/FailingCallable.java @@ -53,7 +53,7 @@ class FailingCallable implements Callable { private final int expectedFailuresCount; private final String result; - FailingCallable(ApiTracer tracer, int expectedFailuresCount, String result) { + FailingCallable(int expectedFailuresCount, String result, ApiTracer tracer) { this.tracer = tracer; this.expectedFailuresCount = expectedFailuresCount; this.result = result; diff --git a/gax/src/test/java/com/google/api/gax/retrying/ScheduledRetryingExecutorTest.java b/gax/src/test/java/com/google/api/gax/retrying/ScheduledRetryingExecutorTest.java index 8bf83bef6..f7281459e 100644 --- a/gax/src/test/java/com/google/api/gax/retrying/ScheduledRetryingExecutorTest.java +++ b/gax/src/test/java/com/google/api/gax/retrying/ScheduledRetryingExecutorTest.java @@ -90,7 +90,7 @@ public void testSuccessWithFailuresPeekAttempt() throws Exception { final int maxRetries = 100; ScheduledExecutorService localExecutor = Executors.newSingleThreadScheduledExecutor(); - FailingCallable callable = new FailingCallable(tracer, 15, "SUCCESS"); + FailingCallable callable = new FailingCallable(15, "SUCCESS", tracer); RetrySettings retrySettings = FAST_RETRY_SETTINGS @@ -140,7 +140,7 @@ public void testSuccessWithFailuresGetAttempt() throws Exception { final int maxRetries = 100; ScheduledExecutorService localExecutor = Executors.newSingleThreadScheduledExecutor(); - FailingCallable callable = new FailingCallable(tracer, 15, "SUCCESS"); + FailingCallable callable = new FailingCallable(15, "SUCCESS", tracer); RetrySettings retrySettings = FAST_RETRY_SETTINGS .toBuilder() @@ -192,7 +192,7 @@ public void testCancelGetAttempt() throws Exception { ScheduledExecutorService localExecutor = Executors.newSingleThreadScheduledExecutor(); final int maxRetries = 100; - FailingCallable callable = new FailingCallable(tracer, maxRetries - 1, "SUCCESS"); + FailingCallable callable = new FailingCallable(maxRetries - 1, "SUCCESS", tracer); RetrySettings retrySettings = FAST_RETRY_SETTINGS .toBuilder() @@ -248,7 +248,7 @@ public void testCancelGetAttempt() throws Exception { public void testCancelOuterFutureAfterStart() throws Exception { for (int executionsCount = 0; executionsCount < EXECUTIONS_COUNT; executionsCount++) { ScheduledExecutorService localExecutor = Executors.newSingleThreadScheduledExecutor(); - FailingCallable callable = new FailingCallable(tracer, 4, "SUCCESS"); + FailingCallable callable = new FailingCallable(4, "SUCCESS", tracer); RetrySettings retrySettings = FAST_RETRY_SETTINGS .toBuilder() @@ -274,7 +274,7 @@ public void testCancelOuterFutureAfterStart() throws Exception { @Test public void testCancelIsTraced() throws Exception { ScheduledExecutorService localExecutor = Executors.newSingleThreadScheduledExecutor(); - FailingCallable callable = new FailingCallable(tracer, 4, "SUCCESS"); + FailingCallable callable = new FailingCallable(4, "SUCCESS", tracer); RetrySettings retrySettings = FAST_RETRY_SETTINGS .toBuilder() @@ -302,7 +302,7 @@ public void testCancelProxiedFutureAfterStart() throws Exception { // this is a heavy test, which takes a lot of time, so only few executions. for (int executionsCount = 0; executionsCount < 2; executionsCount++) { ScheduledExecutorService localExecutor = Executors.newSingleThreadScheduledExecutor(); - FailingCallable callable = new FailingCallable(tracer, 5, "SUCCESS"); + FailingCallable callable = new FailingCallable(5, "SUCCESS", tracer); RetrySettings retrySettings = FAST_RETRY_SETTINGS .toBuilder() From 7f0bcab8550ae6d2c549cb69f8277087755ed377 Mon Sep 17 00:00:00 2001 From: Igor Bernstein Date: Thu, 10 Jan 2019 16:27:36 -0500 Subject: [PATCH 8/8] reformat --- .../AbstractRetryingExecutorTest.java | 27 +++++++------------ 1 file changed, 9 insertions(+), 18 deletions(-) diff --git a/gax/src/test/java/com/google/api/gax/retrying/AbstractRetryingExecutorTest.java b/gax/src/test/java/com/google/api/gax/retrying/AbstractRetryingExecutorTest.java index 0d9adb052..cb2e6bf9c 100644 --- a/gax/src/test/java/com/google/api/gax/retrying/AbstractRetryingExecutorTest.java +++ b/gax/src/test/java/com/google/api/gax/retrying/AbstractRetryingExecutorTest.java @@ -108,8 +108,7 @@ public void testSuccessWithFailures() throws Exception { assertEquals(5, future.getAttemptSettings().getAttemptCount()); verify(tracer, times(6)).attemptStarted(anyInt()); - verify(tracer, times(5)) - .attemptFailed(any(Throwable.class), any(Duration.class)); + verify(tracer, times(5)).attemptFailed(any(Throwable.class), any(Duration.class)); verify(tracer, times(1)).attemptSucceeded(); verifyNoMoreInteractions(tracer); } @@ -152,10 +151,8 @@ public void testMaxRetriesExceeded() throws Exception { assertEquals(5, future.getAttemptSettings().getAttemptCount()); verify(tracer, times(6)).attemptStarted(anyInt()); - verify(tracer, times(5)) - .attemptFailed(any(Throwable.class), any(Duration.class)); - verify(tracer, times(1)) - .attemptFailedRetriesExhausted(any(Throwable.class)); + verify(tracer, times(5)).attemptFailed(any(Throwable.class), any(Duration.class)); + verify(tracer, times(1)).attemptFailedRetriesExhausted(any(Throwable.class)); verifyNoMoreInteractions(tracer); } @@ -177,8 +174,7 @@ public void testTotalTimeoutExceeded() throws Exception { assertTrue(future.getAttemptSettings().getAttemptCount() < 4); verify(tracer, times(1)).attemptStarted(anyInt()); - verify(tracer, times(1)) - .attemptFailedRetriesExhausted(any(Throwable.class)); + verify(tracer, times(1)).attemptFailedRetriesExhausted(any(Throwable.class)); verifyNoMoreInteractions(tracer); } @@ -221,11 +217,9 @@ public void testCancelByRetryingAlgorithm() throws Exception { verify(tracer, times(5)).attemptStarted(anyInt()); // Pre-apocalypse failures - verify(tracer, times(4)) - .attemptFailed(any(Throwable.class), any(Duration.class)); + verify(tracer, times(4)).attemptFailed(any(Throwable.class), any(Duration.class)); // Apocalypse failure - verify(tracer, times(1)) - .attemptFailedRetriesExhausted(any(CancellationException.class)); + verify(tracer, times(1)).attemptFailedRetriesExhausted(any(CancellationException.class)); verifyNoMoreInteractions(tracer); } @@ -242,11 +236,9 @@ public void testUnexpectedExceptionFromRetryAlgorithm() throws Exception { verify(tracer, times(5)).attemptStarted(anyInt()); // Pre-apocalypse failures - verify(tracer, times(4)) - .attemptFailed(any(Throwable.class), any(Duration.class)); + verify(tracer, times(4)).attemptFailed(any(Throwable.class), any(Duration.class)); // Apocalypse failure - verify(tracer, times(1)) - .attemptPermanentFailure(any(RuntimeException.class)); + verify(tracer, times(1)).attemptPermanentFailure(any(RuntimeException.class)); verifyNoMoreInteractions(tracer); } @@ -273,8 +265,7 @@ public void testPollExceptionByPollAlgorithm() throws Exception { assertTrue(future.getAttemptSettings().getAttemptCount() < 4); verify(tracer, times(1)).attemptStarted(anyInt()); - verify(tracer, times(1)) - .attemptPermanentFailure(any(PollException.class)); + verify(tracer, times(1)).attemptPermanentFailure(any(PollException.class)); verifyNoMoreInteractions(tracer); }