From aade2f28eb6205c0d3710d3e7e86a67df2a41714 Mon Sep 17 00:00:00 2001 From: Andrea Lin Date: Mon, 13 May 2019 12:54:01 -0700 Subject: [PATCH 1/7] diff --- .../main/java/com/google/api/gax/rpc/Callables.java | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/gax/src/main/java/com/google/api/gax/rpc/Callables.java b/gax/src/main/java/com/google/api/gax/rpc/Callables.java index c0f48ea67..ecabc9655 100644 --- a/gax/src/main/java/com/google/api/gax/rpc/Callables.java +++ b/gax/src/main/java/com/google/api/gax/rpc/Callables.java @@ -35,7 +35,6 @@ import com.google.api.gax.longrunning.OperationSnapshot; import com.google.api.gax.retrying.ExponentialRetryAlgorithm; import com.google.api.gax.retrying.RetryAlgorithm; -import com.google.api.gax.retrying.RetrySettings; import com.google.api.gax.retrying.ScheduledRetryingExecutor; import com.google.api.gax.retrying.StreamingRetryAlgorithm; import java.util.Collection; @@ -56,7 +55,7 @@ public static UnaryCallable retrying( UnaryCallSettings callSettings, ClientContext clientContext) { - if (areRetriesDisabled(callSettings.getRetryableCodes(), callSettings.getRetrySettings())) { + if (areRetriesDisabled(callSettings.getRetryableCodes())) { return innerCallable; } @@ -77,7 +76,7 @@ public static ServerStreamingCallable ServerStreamingCallSettings callSettings, ClientContext clientContext) { - if (areRetriesDisabled(callSettings.getRetryableCodes(), callSettings.getRetrySettings())) { + if (areRetriesDisabled(callSettings.getRetryableCodes())) { return innerCallable; } @@ -218,8 +217,7 @@ OperationCallableImpl longRunningOperationImpl( initialCallable, scheduler, longRunningClient, operationCallSettings); } - private static boolean areRetriesDisabled( - Collection retryableCodes, RetrySettings retrySettings) { - return retrySettings.getMaxAttempts() == 1 || retryableCodes.isEmpty(); + private static boolean areRetriesDisabled(Collection retryableCodes) { + return retryableCodes.isEmpty(); } } From c6ac96d2b712a4a7c2c7a814ce3e0c40c52c9801 Mon Sep 17 00:00:00 2001 From: Andrea Lin Date: Wed, 15 May 2019 14:34:00 -0700 Subject: [PATCH 2/7] testing + hotfix --- .../api/gax/grpc/GrpcCallableFactory.java | 1 + .../com/google/api/gax/grpc/RetryingTest.java | 169 ++++++++++++++++++ .../api/gax/retrying/RetrySettings.java | 1 + .../com/google/api/gax/rpc/Callables.java | 10 +- .../google/api/gax/rpc/ClientContextTest.java | 10 +- .../com/google/api/gax/rpc/TimeoutTest.java | 32 ++++ 6 files changed, 216 insertions(+), 7 deletions(-) create mode 100644 gax-grpc/src/test/java/com/google/api/gax/grpc/RetryingTest.java create mode 100644 gax/src/test/java/com/google/api/gax/rpc/TimeoutTest.java diff --git a/gax-grpc/src/main/java/com/google/api/gax/grpc/GrpcCallableFactory.java b/gax-grpc/src/main/java/com/google/api/gax/grpc/GrpcCallableFactory.java index 5b45c9bff..40bfbc724 100644 --- a/gax-grpc/src/main/java/com/google/api/gax/grpc/GrpcCallableFactory.java +++ b/gax-grpc/src/main/java/com/google/api/gax/grpc/GrpcCallableFactory.java @@ -89,6 +89,7 @@ public static UnaryCallable createBas callable = new GrpcUnaryRequestParamCallable<>(callable, grpcCallSettings.getParamsExtractor()); } + callable = new GrpcExceptionCallable<>(callable, callSettings.getRetryableCodes()); callable = Callables.retrying(callable, callSettings, clientContext); diff --git a/gax-grpc/src/test/java/com/google/api/gax/grpc/RetryingTest.java b/gax-grpc/src/test/java/com/google/api/gax/grpc/RetryingTest.java new file mode 100644 index 000000000..7bdaa8764 --- /dev/null +++ b/gax-grpc/src/test/java/com/google/api/gax/grpc/RetryingTest.java @@ -0,0 +1,169 @@ +/* + * Copyright 2019 Google LLC + * + * Redistribution and use in source and binary forms, with or without + * modification, are permitted provided that the following conditions are + * met: + * + * * Redistributions of source code must retain the above copyright + * notice, this list of conditions and the following disclaimer. + * * Redistributions in binary form must reproduce the above + * copyright notice, this list of conditions and the following disclaimer + * in the documentation and/or other materials provided with the + * distribution. + * * Neither the name of Google LLC nor the names of its + * contributors may be used to endorse or promote products derived from + * this software without specific prior written permission. + * + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS + * "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT + * LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR + * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT + * OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, + * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT + * LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, + * DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY + * THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT + * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE + * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. + */ +package com.google.api.gax.grpc; + +import static com.google.common.truth.Truth.assertThat; + +import com.google.api.client.util.IOUtils; +import com.google.api.core.ApiFuture; +import com.google.api.core.ApiFutures; +import com.google.api.gax.core.FakeApiClock; +import com.google.api.gax.core.RecordingScheduler; +import com.google.api.gax.retrying.RetrySettings; +import com.google.api.gax.rpc.ApiException; +import com.google.api.gax.rpc.ClientContext; +import com.google.api.gax.rpc.RequestParamsExtractor; +import com.google.api.gax.rpc.StatusCode; +import com.google.api.gax.rpc.StatusCode.Code; +import com.google.api.gax.rpc.UnaryCallSettings; +import com.google.api.gax.rpc.UnaryCallable; +import com.google.api.gax.rpc.testing.FakeCallContext; +import com.google.api.gax.rpc.testing.FakeChannel; +import com.google.api.gax.rpc.testing.FakeStatusCode; +import com.google.api.gax.rpc.testing.FakeTransportChannel; +import com.google.common.collect.ImmutableMap; +import com.google.common.collect.ImmutableSet; +import io.grpc.CallOptions; +import io.grpc.Channel; +import io.grpc.ClientCall; +import io.grpc.Deadline; +import io.grpc.ManagedChannel; +import io.grpc.MethodDescriptor; +import io.grpc.MethodDescriptor.Marshaller; +import io.grpc.MethodDescriptor.MethodType; +import java.io.ByteArrayInputStream; +import java.io.ByteArrayOutputStream; +import java.io.IOException; +import java.io.InputStream; +import java.io.OutputStream; +import java.util.Map; +import java.util.concurrent.TimeUnit; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.junit.runners.JUnit4; +import org.mockito.ArgumentCaptor; +import org.mockito.ArgumentMatchers; +import org.mockito.Mockito; +import org.threeten.bp.Duration; + +@RunWith(JUnit4.class) +public class RetryingTest { + + @Test(expected = ApiException.class) + public void testNonRetrySettings() { + String CALL_OPTIONS_AUTHORITY = "RETRYING_TEST"; + + ImmutableSet emptyRetryCodes = ImmutableSet.of(); + Duration totalTimeout = Duration.ofDays(2); + + RetrySettings retrySettings = + RetrySettings.newBuilder() + .setTotalTimeout(totalTimeout) + .setInitialRetryDelay(Duration.ZERO) + .setRetryDelayMultiplier(1.0) + .setMaxRetryDelay(Duration.ZERO) + .setMaxAttempts(1) + .setJittered(true) + .setInitialRpcTimeout(totalTimeout) + .setRpcTimeoutMultiplier(1.0) + .setMaxRpcTimeout(totalTimeout) + .build(); + + @SuppressWarnings("unchecked") + Marshaller stringMarshaller = Mockito.mock(Marshaller.class); + + ManagedChannel managedChannel = Mockito.mock(ManagedChannel.class); + + MethodDescriptor methodDescriptor = + MethodDescriptor.newBuilder() + .setSchemaDescriptor("yaml") + .setFullMethodName("fake.test/Greet") + .setResponseMarshaller(stringMarshaller) + .setRequestMarshaller(stringMarshaller) + .setType(MethodType.UNARY) + .build(); + RequestParamsExtractor paramsExtractor = + new RequestParamsExtractor() { + @Override + public Map extract(String request) { + return ImmutableMap.of(request, request); + } + }; + + @SuppressWarnings("unchecked") + ClientCall clientCall = Mockito.mock(ClientCall.class); + + // Clobber the "authority" property with an identifier that allows us to trace + // the use of this CallOptions variable. + CallOptions spyCallOptions = CallOptions.DEFAULT.withAuthority("RETRYING_TEST"); + GrpcCallContext grpcCallContext = GrpcCallContext.createDefault() + .withChannel(managedChannel) + .withCallOptions(spyCallOptions); + + ArgumentCaptor callOptionsArgumentCaptor = ArgumentCaptor.forClass(CallOptions.class); + + Mockito + .doReturn(clientCall) + .when(managedChannel) + .newCall(ArgumentMatchers.eq(methodDescriptor), ArgumentMatchers.any(CallOptions.class)); + + Mockito + .doThrow(new ApiException(new RuntimeException(), FakeStatusCode.of(Code.UNAVAILABLE), false)) + .when(clientCall) + .halfClose(); + + GrpcCallSettings grpcCallSettings = + GrpcCallSettings.newBuilder() + .setMethodDescriptor(methodDescriptor) + .setParamsExtractor(paramsExtractor) + .build(); + UnaryCallSettings nonRetriedCallSettings = + UnaryCallSettings.newUnaryCallSettingsBuilder() + .setRetrySettings(retrySettings) + .setRetryableCodes(emptyRetryCodes) + .build(); + UnaryCallable callable = + GrpcCallableFactory.createUnaryCallable( + grpcCallSettings, + nonRetriedCallSettings, + ClientContext.newBuilder().setDefaultCallContext(grpcCallContext).build()); + + ApiFuture future = callable.futureCall("Is your refrigerator running?"); + + Mockito + .verify(managedChannel) + .newCall(ArgumentMatchers.eq(methodDescriptor), callOptionsArgumentCaptor.capture()); + CallOptions callOptionsUsed = callOptionsArgumentCaptor.getValue(); + + assertThat(callOptionsUsed.getDeadline()).isNotNull(); + assertThat(callOptionsUsed.getDeadline()).isGreaterThan(Deadline.after(1, TimeUnit.DAYS)); + assertThat(callOptionsUsed.getAuthority()).isEqualTo(CALL_OPTIONS_AUTHORITY); + } +} diff --git a/gax/src/main/java/com/google/api/gax/retrying/RetrySettings.java b/gax/src/main/java/com/google/api/gax/retrying/RetrySettings.java index 4d6c4d8aa..00fc1a8c6 100644 --- a/gax/src/main/java/com/google/api/gax/retrying/RetrySettings.java +++ b/gax/src/main/java/com/google/api/gax/retrying/RetrySettings.java @@ -313,6 +313,7 @@ public RetrySettings build() { if (params.getRpcTimeoutMultiplier() < 1.0) { throw new IllegalStateException("rpc timeout multiplier must be at least 1"); } + return params; } diff --git a/gax/src/main/java/com/google/api/gax/rpc/Callables.java b/gax/src/main/java/com/google/api/gax/rpc/Callables.java index ecabc9655..e6384ca03 100644 --- a/gax/src/main/java/com/google/api/gax/rpc/Callables.java +++ b/gax/src/main/java/com/google/api/gax/rpc/Callables.java @@ -56,7 +56,10 @@ public static UnaryCallable retrying( ClientContext clientContext) { if (areRetriesDisabled(callSettings.getRetryableCodes())) { - return innerCallable; + return innerCallable.withDefaultCallContext( + clientContext + .getDefaultCallContext() + .withTimeout(callSettings.getRetrySettings().getTotalTimeout())); } RetryAlgorithm retryAlgorithm = @@ -77,7 +80,10 @@ public static ServerStreamingCallable ClientContext clientContext) { if (areRetriesDisabled(callSettings.getRetryableCodes())) { - return innerCallable; + return innerCallable.withDefaultCallContext( + clientContext + .getDefaultCallContext() + .withTimeout(callSettings.getRetrySettings().getTotalTimeout())); } StreamingRetryAlgorithm retryAlgorithm = diff --git a/gax/src/test/java/com/google/api/gax/rpc/ClientContextTest.java b/gax/src/test/java/com/google/api/gax/rpc/ClientContextTest.java index e6e58baf8..8d4a5bb5f 100644 --- a/gax/src/test/java/com/google/api/gax/rpc/ClientContextTest.java +++ b/gax/src/test/java/com/google/api/gax/rpc/ClientContextTest.java @@ -52,7 +52,7 @@ @RunWith(JUnit4.class) public class ClientContextTest { - private static class InterceptingExecutor extends ScheduledThreadPoolExecutor { + public static class InterceptingExecutor extends ScheduledThreadPoolExecutor { boolean shutdownCalled = false; public InterceptingExecutor(int corePoolSize) { @@ -64,11 +64,11 @@ public void shutdown() { } } - private static class FakeExecutorProvider implements ExecutorProvider { + public static class FakeExecutorProvider implements ExecutorProvider { ScheduledExecutorService executor; boolean shouldAutoClose; - FakeExecutorProvider(ScheduledExecutorService executor, boolean shouldAutoClose) { + public FakeExecutorProvider(ScheduledExecutorService executor, boolean shouldAutoClose) { this.executor = executor; this.shouldAutoClose = shouldAutoClose; } @@ -84,13 +84,13 @@ public ScheduledExecutorService getExecutor() { } } - private static class FakeTransportProvider implements TransportChannelProvider { + public static class FakeTransportProvider implements TransportChannelProvider { final ScheduledExecutorService executor; final FakeTransportChannel transport; final boolean shouldAutoClose; final Map headers; - FakeTransportProvider( + public FakeTransportProvider( FakeTransportChannel transport, ScheduledExecutorService executor, boolean shouldAutoClose, diff --git a/gax/src/test/java/com/google/api/gax/rpc/TimeoutTest.java b/gax/src/test/java/com/google/api/gax/rpc/TimeoutTest.java new file mode 100644 index 000000000..a0486e320 --- /dev/null +++ b/gax/src/test/java/com/google/api/gax/rpc/TimeoutTest.java @@ -0,0 +1,32 @@ +/* + * Copyright 2019 Google LLC + * + * Redistribution and use in source and binary forms, with or without + * modification, are permitted provided that the following conditions are + * met: + * + * * Redistributions of source code must retain the above copyright + * notice, this list of conditions and the following disclaimer. + * * Redistributions in binary form must reproduce the above + * copyright notice, this list of conditions and the following disclaimer + * in the documentation and/or other materials provided with the + * distribution. + * * Neither the name of Google LLC nor the names of its + * contributors may be used to endorse or promote products derived from + * this software without specific prior written permission. + * + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS + * "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT + * LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR + * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT + * OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, + * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT + * LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, + * DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY + * THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT + * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE + * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. + */ +package com.google.api.gax.rpc; + +public class TimeoutTest {} From f355042cdf05f3e37cafe4b5f3ab607f28cb58f8 Mon Sep 17 00:00:00 2001 From: Andrea Lin Date: Wed, 15 May 2019 14:36:37 -0700 Subject: [PATCH 3/7] mock out unnecessary things" --- .../com/google/api/gax/grpc/RetryingTest.java | 45 ++++++++----------- 1 file changed, 19 insertions(+), 26 deletions(-) diff --git a/gax-grpc/src/test/java/com/google/api/gax/grpc/RetryingTest.java b/gax-grpc/src/test/java/com/google/api/gax/grpc/RetryingTest.java index 7bdaa8764..802cf0402 100644 --- a/gax-grpc/src/test/java/com/google/api/gax/grpc/RetryingTest.java +++ b/gax-grpc/src/test/java/com/google/api/gax/grpc/RetryingTest.java @@ -79,26 +79,12 @@ public class RetryingTest { @Test(expected = ApiException.class) public void testNonRetrySettings() { String CALL_OPTIONS_AUTHORITY = "RETRYING_TEST"; - ImmutableSet emptyRetryCodes = ImmutableSet.of(); Duration totalTimeout = Duration.ofDays(2); - RetrySettings retrySettings = - RetrySettings.newBuilder() - .setTotalTimeout(totalTimeout) - .setInitialRetryDelay(Duration.ZERO) - .setRetryDelayMultiplier(1.0) - .setMaxRetryDelay(Duration.ZERO) - .setMaxAttempts(1) - .setJittered(true) - .setInitialRpcTimeout(totalTimeout) - .setRpcTimeoutMultiplier(1.0) - .setMaxRpcTimeout(totalTimeout) - .build(); - @SuppressWarnings("unchecked") Marshaller stringMarshaller = Mockito.mock(Marshaller.class); - + RequestParamsExtractor paramsExtractor = Mockito.mock(RequestParamsExtractor.class); ManagedChannel managedChannel = Mockito.mock(ManagedChannel.class); MethodDescriptor methodDescriptor = @@ -109,16 +95,26 @@ public void testNonRetrySettings() { .setRequestMarshaller(stringMarshaller) .setType(MethodType.UNARY) .build(); - RequestParamsExtractor paramsExtractor = - new RequestParamsExtractor() { - @Override - public Map extract(String request) { - return ImmutableMap.of(request, request); - } - }; + + RetrySettings retrySettings = + RetrySettings.newBuilder() + .setTotalTimeout(totalTimeout) + .setInitialRetryDelay(Duration.ZERO) + .setRetryDelayMultiplier(1.0) + .setMaxRetryDelay(Duration.ZERO) + .setMaxAttempts(1) + .setJittered(true) + .setInitialRpcTimeout(totalTimeout) + .setRpcTimeoutMultiplier(1.0) + .setMaxRpcTimeout(totalTimeout) + .build(); @SuppressWarnings("unchecked") ClientCall clientCall = Mockito.mock(ClientCall.class); + Mockito + .doReturn(clientCall) + .when(managedChannel) + .newCall(ArgumentMatchers.eq(methodDescriptor), ArgumentMatchers.any(CallOptions.class)); // Clobber the "authority" property with an identifier that allows us to trace // the use of this CallOptions variable. @@ -129,10 +125,7 @@ public Map extract(String request) { ArgumentCaptor callOptionsArgumentCaptor = ArgumentCaptor.forClass(CallOptions.class); - Mockito - .doReturn(clientCall) - .when(managedChannel) - .newCall(ArgumentMatchers.eq(methodDescriptor), ArgumentMatchers.any(CallOptions.class)); + Mockito .doThrow(new ApiException(new RuntimeException(), FakeStatusCode.of(Code.UNAVAILABLE), false)) From a422e4df4b0b5e0158558b5bbf22001bc637f80c Mon Sep 17 00:00:00 2001 From: Andrea Lin Date: Wed, 15 May 2019 14:52:49 -0700 Subject: [PATCH 4/7] renaming to TimeoutTest --- .../{RetryingTest.java => TimeoutTest.java} | 113 ++++++++---------- .../com/google/api/gax/rpc/TimeoutTest.java | 32 ----- 2 files changed, 52 insertions(+), 93 deletions(-) rename gax-grpc/src/test/java/com/google/api/gax/grpc/{RetryingTest.java => TimeoutTest.java} (62%) delete mode 100644 gax/src/test/java/com/google/api/gax/rpc/TimeoutTest.java diff --git a/gax-grpc/src/test/java/com/google/api/gax/grpc/RetryingTest.java b/gax-grpc/src/test/java/com/google/api/gax/grpc/TimeoutTest.java similarity index 62% rename from gax-grpc/src/test/java/com/google/api/gax/grpc/RetryingTest.java rename to gax-grpc/src/test/java/com/google/api/gax/grpc/TimeoutTest.java index 802cf0402..ae84a7317 100644 --- a/gax-grpc/src/test/java/com/google/api/gax/grpc/RetryingTest.java +++ b/gax-grpc/src/test/java/com/google/api/gax/grpc/TimeoutTest.java @@ -31,11 +31,7 @@ import static com.google.common.truth.Truth.assertThat; -import com.google.api.client.util.IOUtils; import com.google.api.core.ApiFuture; -import com.google.api.core.ApiFutures; -import com.google.api.gax.core.FakeApiClock; -import com.google.api.gax.core.RecordingScheduler; import com.google.api.gax.retrying.RetrySettings; import com.google.api.gax.rpc.ApiException; import com.google.api.gax.rpc.ClientContext; @@ -44,26 +40,15 @@ import com.google.api.gax.rpc.StatusCode.Code; import com.google.api.gax.rpc.UnaryCallSettings; import com.google.api.gax.rpc.UnaryCallable; -import com.google.api.gax.rpc.testing.FakeCallContext; -import com.google.api.gax.rpc.testing.FakeChannel; import com.google.api.gax.rpc.testing.FakeStatusCode; -import com.google.api.gax.rpc.testing.FakeTransportChannel; -import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableSet; import io.grpc.CallOptions; -import io.grpc.Channel; import io.grpc.ClientCall; import io.grpc.Deadline; import io.grpc.ManagedChannel; import io.grpc.MethodDescriptor; import io.grpc.MethodDescriptor.Marshaller; import io.grpc.MethodDescriptor.MethodType; -import java.io.ByteArrayInputStream; -import java.io.ByteArrayOutputStream; -import java.io.IOException; -import java.io.InputStream; -import java.io.OutputStream; -import java.util.Map; import java.util.concurrent.TimeUnit; import org.junit.Test; import org.junit.runner.RunWith; @@ -74,61 +59,63 @@ import org.threeten.bp.Duration; @RunWith(JUnit4.class) -public class RetryingTest { +public class TimeoutTest { + private static final String CALL_OPTIONS_AUTHORITY = "RETRYING_TEST"; + private static final int DEADLINE_IN_DAYS = 7; + private static final ImmutableSet emptyRetryCodes = ImmutableSet.of(); + private static final Duration totalTimeout = Duration.ofDays(DEADLINE_IN_DAYS); + + @SuppressWarnings("unchecked") + private static final Marshaller stringMarshaller = Mockito.mock(Marshaller.class); + + @SuppressWarnings("unchecked") + private static final RequestParamsExtractor paramsExtractor = + Mockito.mock(RequestParamsExtractor.class); + + private static final ManagedChannel managedChannel = Mockito.mock(ManagedChannel.class); + + private static final MethodDescriptor methodDescriptor = + MethodDescriptor.newBuilder() + .setSchemaDescriptor("yaml") + .setFullMethodName("fake.test/RingRing") + .setResponseMarshaller(stringMarshaller) + .setRequestMarshaller(stringMarshaller) + .setType(MethodType.UNARY) + .build(); + + private static final RetrySettings nonRetrySettings = + RetrySettings.newBuilder() + .setTotalTimeout(totalTimeout) + .setInitialRetryDelay(Duration.ZERO) + .setRetryDelayMultiplier(1.0) + .setMaxRetryDelay(Duration.ZERO) + .setMaxAttempts(1) + .setJittered(true) + .setInitialRpcTimeout(totalTimeout) + .setRpcTimeoutMultiplier(1.0) + .setMaxRpcTimeout(totalTimeout) + .build(); @Test(expected = ApiException.class) - public void testNonRetrySettings() { - String CALL_OPTIONS_AUTHORITY = "RETRYING_TEST"; - ImmutableSet emptyRetryCodes = ImmutableSet.of(); - Duration totalTimeout = Duration.ofDays(2); - - @SuppressWarnings("unchecked") - Marshaller stringMarshaller = Mockito.mock(Marshaller.class); - RequestParamsExtractor paramsExtractor = Mockito.mock(RequestParamsExtractor.class); - ManagedChannel managedChannel = Mockito.mock(ManagedChannel.class); - - MethodDescriptor methodDescriptor = - MethodDescriptor.newBuilder() - .setSchemaDescriptor("yaml") - .setFullMethodName("fake.test/Greet") - .setResponseMarshaller(stringMarshaller) - .setRequestMarshaller(stringMarshaller) - .setType(MethodType.UNARY) - .build(); - - RetrySettings retrySettings = - RetrySettings.newBuilder() - .setTotalTimeout(totalTimeout) - .setInitialRetryDelay(Duration.ZERO) - .setRetryDelayMultiplier(1.0) - .setMaxRetryDelay(Duration.ZERO) - .setMaxAttempts(1) - .setJittered(true) - .setInitialRpcTimeout(totalTimeout) - .setRpcTimeoutMultiplier(1.0) - .setMaxRpcTimeout(totalTimeout) - .build(); + public void testNonRetryUnarySettings() { @SuppressWarnings("unchecked") ClientCall clientCall = Mockito.mock(ClientCall.class); - Mockito - .doReturn(clientCall) + Mockito.doReturn(clientCall) .when(managedChannel) .newCall(ArgumentMatchers.eq(methodDescriptor), ArgumentMatchers.any(CallOptions.class)); // Clobber the "authority" property with an identifier that allows us to trace // the use of this CallOptions variable. CallOptions spyCallOptions = CallOptions.DEFAULT.withAuthority("RETRYING_TEST"); - GrpcCallContext grpcCallContext = GrpcCallContext.createDefault() - .withChannel(managedChannel) - .withCallOptions(spyCallOptions); - - ArgumentCaptor callOptionsArgumentCaptor = ArgumentCaptor.forClass(CallOptions.class); + GrpcCallContext grpcCallContext = + GrpcCallContext.createDefault().withChannel(managedChannel).withCallOptions(spyCallOptions); + ArgumentCaptor callOptionsArgumentCaptor = + ArgumentCaptor.forClass(CallOptions.class); - - Mockito - .doThrow(new ApiException(new RuntimeException(), FakeStatusCode.of(Code.UNAVAILABLE), false)) + Mockito.doThrow( + new ApiException(new RuntimeException(), FakeStatusCode.of(Code.UNAVAILABLE), false)) .when(clientCall) .halfClose(); @@ -139,7 +126,7 @@ public void testNonRetrySettings() { .build(); UnaryCallSettings nonRetriedCallSettings = UnaryCallSettings.newUnaryCallSettingsBuilder() - .setRetrySettings(retrySettings) + .setRetrySettings(nonRetrySettings) .setRetryableCodes(emptyRetryCodes) .build(); UnaryCallable callable = @@ -150,13 +137,17 @@ public void testNonRetrySettings() { ApiFuture future = callable.futureCall("Is your refrigerator running?"); - Mockito - .verify(managedChannel) + Mockito.verify(managedChannel) .newCall(ArgumentMatchers.eq(methodDescriptor), callOptionsArgumentCaptor.capture()); CallOptions callOptionsUsed = callOptionsArgumentCaptor.getValue(); + // Verify that the gRPC channel used the CallOptions with our custom timeout of ~2 Days. + assertThat(callOptionsUsed.getDeadline()).isNotNull(); - assertThat(callOptionsUsed.getDeadline()).isGreaterThan(Deadline.after(1, TimeUnit.DAYS)); + assertThat(callOptionsUsed.getDeadline()) + .isGreaterThan(Deadline.after(DEADLINE_IN_DAYS - 1, TimeUnit.DAYS)); + assertThat(callOptionsUsed.getDeadline()) + .isLessThan(Deadline.after(DEADLINE_IN_DAYS, TimeUnit.DAYS)); assertThat(callOptionsUsed.getAuthority()).isEqualTo(CALL_OPTIONS_AUTHORITY); } } diff --git a/gax/src/test/java/com/google/api/gax/rpc/TimeoutTest.java b/gax/src/test/java/com/google/api/gax/rpc/TimeoutTest.java deleted file mode 100644 index a0486e320..000000000 --- a/gax/src/test/java/com/google/api/gax/rpc/TimeoutTest.java +++ /dev/null @@ -1,32 +0,0 @@ -/* - * Copyright 2019 Google LLC - * - * Redistribution and use in source and binary forms, with or without - * modification, are permitted provided that the following conditions are - * met: - * - * * Redistributions of source code must retain the above copyright - * notice, this list of conditions and the following disclaimer. - * * Redistributions in binary form must reproduce the above - * copyright notice, this list of conditions and the following disclaimer - * in the documentation and/or other materials provided with the - * distribution. - * * Neither the name of Google LLC nor the names of its - * contributors may be used to endorse or promote products derived from - * this software without specific prior written permission. - * - * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS - * "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT - * LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR - * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT - * OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, - * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT - * LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, - * DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY - * THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT - * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE - * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. - */ -package com.google.api.gax.rpc; - -public class TimeoutTest {} From 6d77e44763e15213d73d11bfb994f60ead49e639 Mon Sep 17 00:00:00 2001 From: Andrea Lin Date: Wed, 15 May 2019 14:58:14 -0700 Subject: [PATCH 5/7] compile --- .../com/google/api/gax/grpc/GrpcCallableFactory.java | 1 - .../com/google/api/gax/retrying/RetrySettings.java | 1 - .../main/java/com/google/api/gax/rpc/Callables.java | 10 ++++++---- .../java/com/google/api/gax/rpc/ClientContextTest.java | 10 +++++----- 4 files changed, 11 insertions(+), 11 deletions(-) diff --git a/gax-grpc/src/main/java/com/google/api/gax/grpc/GrpcCallableFactory.java b/gax-grpc/src/main/java/com/google/api/gax/grpc/GrpcCallableFactory.java index 40bfbc724..5b45c9bff 100644 --- a/gax-grpc/src/main/java/com/google/api/gax/grpc/GrpcCallableFactory.java +++ b/gax-grpc/src/main/java/com/google/api/gax/grpc/GrpcCallableFactory.java @@ -89,7 +89,6 @@ public static UnaryCallable createBas callable = new GrpcUnaryRequestParamCallable<>(callable, grpcCallSettings.getParamsExtractor()); } - callable = new GrpcExceptionCallable<>(callable, callSettings.getRetryableCodes()); callable = Callables.retrying(callable, callSettings, clientContext); diff --git a/gax/src/main/java/com/google/api/gax/retrying/RetrySettings.java b/gax/src/main/java/com/google/api/gax/retrying/RetrySettings.java index 00fc1a8c6..4d6c4d8aa 100644 --- a/gax/src/main/java/com/google/api/gax/retrying/RetrySettings.java +++ b/gax/src/main/java/com/google/api/gax/retrying/RetrySettings.java @@ -313,7 +313,6 @@ public RetrySettings build() { if (params.getRpcTimeoutMultiplier() < 1.0) { throw new IllegalStateException("rpc timeout multiplier must be at least 1"); } - return params; } diff --git a/gax/src/main/java/com/google/api/gax/rpc/Callables.java b/gax/src/main/java/com/google/api/gax/rpc/Callables.java index e6384ca03..a66090907 100644 --- a/gax/src/main/java/com/google/api/gax/rpc/Callables.java +++ b/gax/src/main/java/com/google/api/gax/rpc/Callables.java @@ -35,6 +35,7 @@ import com.google.api.gax.longrunning.OperationSnapshot; import com.google.api.gax.retrying.ExponentialRetryAlgorithm; import com.google.api.gax.retrying.RetryAlgorithm; +import com.google.api.gax.retrying.RetrySettings; import com.google.api.gax.retrying.ScheduledRetryingExecutor; import com.google.api.gax.retrying.StreamingRetryAlgorithm; import java.util.Collection; @@ -55,7 +56,7 @@ public static UnaryCallable retrying( UnaryCallSettings callSettings, ClientContext clientContext) { - if (areRetriesDisabled(callSettings.getRetryableCodes())) { + if (areRetriesDisabled(callSettings.getRetryableCodes(), callSettings.getRetrySettings())) { return innerCallable.withDefaultCallContext( clientContext .getDefaultCallContext() @@ -79,7 +80,7 @@ public static ServerStreamingCallable ServerStreamingCallSettings callSettings, ClientContext clientContext) { - if (areRetriesDisabled(callSettings.getRetryableCodes())) { + if (areRetriesDisabled(callSettings.getRetryableCodes(), callSettings.getRetrySettings())) { return innerCallable.withDefaultCallContext( clientContext .getDefaultCallContext() @@ -223,7 +224,8 @@ OperationCallableImpl longRunningOperationImpl( initialCallable, scheduler, longRunningClient, operationCallSettings); } - private static boolean areRetriesDisabled(Collection retryableCodes) { - return retryableCodes.isEmpty(); + private static boolean areRetriesDisabled( + Collection retryableCodes, RetrySettings retrySettings) { + return retrySettings.getMaxAttempts() == 1 || retryableCodes.isEmpty(); } } diff --git a/gax/src/test/java/com/google/api/gax/rpc/ClientContextTest.java b/gax/src/test/java/com/google/api/gax/rpc/ClientContextTest.java index 8d4a5bb5f..e6e58baf8 100644 --- a/gax/src/test/java/com/google/api/gax/rpc/ClientContextTest.java +++ b/gax/src/test/java/com/google/api/gax/rpc/ClientContextTest.java @@ -52,7 +52,7 @@ @RunWith(JUnit4.class) public class ClientContextTest { - public static class InterceptingExecutor extends ScheduledThreadPoolExecutor { + private static class InterceptingExecutor extends ScheduledThreadPoolExecutor { boolean shutdownCalled = false; public InterceptingExecutor(int corePoolSize) { @@ -64,11 +64,11 @@ public void shutdown() { } } - public static class FakeExecutorProvider implements ExecutorProvider { + private static class FakeExecutorProvider implements ExecutorProvider { ScheduledExecutorService executor; boolean shouldAutoClose; - public FakeExecutorProvider(ScheduledExecutorService executor, boolean shouldAutoClose) { + FakeExecutorProvider(ScheduledExecutorService executor, boolean shouldAutoClose) { this.executor = executor; this.shouldAutoClose = shouldAutoClose; } @@ -84,13 +84,13 @@ public ScheduledExecutorService getExecutor() { } } - public static class FakeTransportProvider implements TransportChannelProvider { + private static class FakeTransportProvider implements TransportChannelProvider { final ScheduledExecutorService executor; final FakeTransportChannel transport; final boolean shouldAutoClose; final Map headers; - public FakeTransportProvider( + FakeTransportProvider( FakeTransportChannel transport, ScheduledExecutorService executor, boolean shouldAutoClose, From afec81c30d5ad19f2359c292da2cedac6e41c5ae Mon Sep 17 00:00:00 2001 From: Andrea Lin Date: Wed, 15 May 2019 15:04:35 -0700 Subject: [PATCH 6/7] fix tests --- .../java/com/google/api/gax/grpc/TimeoutTest.java | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/gax-grpc/src/test/java/com/google/api/gax/grpc/TimeoutTest.java b/gax-grpc/src/test/java/com/google/api/gax/grpc/TimeoutTest.java index ae84a7317..fde7cea1f 100644 --- a/gax-grpc/src/test/java/com/google/api/gax/grpc/TimeoutTest.java +++ b/gax-grpc/src/test/java/com/google/api/gax/grpc/TimeoutTest.java @@ -30,6 +30,7 @@ package com.google.api.gax.grpc; import static com.google.common.truth.Truth.assertThat; +import static org.mockito.Mockito.times; import com.google.api.core.ApiFuture; import com.google.api.gax.retrying.RetrySettings; @@ -96,7 +97,7 @@ public class TimeoutTest { .setMaxRpcTimeout(totalTimeout) .build(); - @Test(expected = ApiException.class) + @Test public void testNonRetryUnarySettings() { @SuppressWarnings("unchecked") @@ -114,6 +115,8 @@ public void testNonRetryUnarySettings() { ArgumentCaptor callOptionsArgumentCaptor = ArgumentCaptor.forClass(CallOptions.class); + // Throw an exception during the gRPC channel business so we don't have to deal with + // processing the channel output. Mockito.doThrow( new ApiException(new RuntimeException(), FakeStatusCode.of(Code.UNAVAILABLE), false)) .when(clientCall) @@ -135,9 +138,12 @@ public void testNonRetryUnarySettings() { nonRetriedCallSettings, ClientContext.newBuilder().setDefaultCallContext(grpcCallContext).build()); - ApiFuture future = callable.futureCall("Is your refrigerator running?"); + try { + ApiFuture future = callable.futureCall("Is your refrigerator running?"); + } catch (ApiException e) { + } - Mockito.verify(managedChannel) + Mockito.verify(managedChannel, times(1)) .newCall(ArgumentMatchers.eq(methodDescriptor), callOptionsArgumentCaptor.capture()); CallOptions callOptionsUsed = callOptionsArgumentCaptor.getValue(); From 19ff63bf244e625bd01176bd9c6f0b1fc37a7393 Mon Sep 17 00:00:00 2001 From: Andrea Lin Date: Wed, 15 May 2019 16:05:37 -0700 Subject: [PATCH 7/7] @Mock and comments --- .../com/google/api/gax/grpc/TimeoutTest.java | 67 ++++++++++--------- .../com/google/api/gax/rpc/Callables.java | 2 + 2 files changed, 39 insertions(+), 30 deletions(-) diff --git a/gax-grpc/src/test/java/com/google/api/gax/grpc/TimeoutTest.java b/gax-grpc/src/test/java/com/google/api/gax/grpc/TimeoutTest.java index fde7cea1f..f9b22e6f3 100644 --- a/gax-grpc/src/test/java/com/google/api/gax/grpc/TimeoutTest.java +++ b/gax-grpc/src/test/java/com/google/api/gax/grpc/TimeoutTest.java @@ -51,12 +51,18 @@ import io.grpc.MethodDescriptor.Marshaller; import io.grpc.MethodDescriptor.MethodType; import java.util.concurrent.TimeUnit; +import org.junit.Before; +import org.junit.Rule; import org.junit.Test; import org.junit.runner.RunWith; import org.junit.runners.JUnit4; import org.mockito.ArgumentCaptor; import org.mockito.ArgumentMatchers; +import org.mockito.Mock; import org.mockito.Mockito; +import org.mockito.junit.MockitoJUnit; +import org.mockito.junit.MockitoRule; +import org.mockito.quality.Strictness; import org.threeten.bp.Duration; @RunWith(JUnit4.class) @@ -66,36 +72,37 @@ public class TimeoutTest { private static final ImmutableSet emptyRetryCodes = ImmutableSet.of(); private static final Duration totalTimeout = Duration.ofDays(DEADLINE_IN_DAYS); - @SuppressWarnings("unchecked") - private static final Marshaller stringMarshaller = Mockito.mock(Marshaller.class); - - @SuppressWarnings("unchecked") - private static final RequestParamsExtractor paramsExtractor = - Mockito.mock(RequestParamsExtractor.class); - - private static final ManagedChannel managedChannel = Mockito.mock(ManagedChannel.class); - - private static final MethodDescriptor methodDescriptor = - MethodDescriptor.newBuilder() - .setSchemaDescriptor("yaml") - .setFullMethodName("fake.test/RingRing") - .setResponseMarshaller(stringMarshaller) - .setRequestMarshaller(stringMarshaller) - .setType(MethodType.UNARY) - .build(); - - private static final RetrySettings nonRetrySettings = - RetrySettings.newBuilder() - .setTotalTimeout(totalTimeout) - .setInitialRetryDelay(Duration.ZERO) - .setRetryDelayMultiplier(1.0) - .setMaxRetryDelay(Duration.ZERO) - .setMaxAttempts(1) - .setJittered(true) - .setInitialRpcTimeout(totalTimeout) - .setRpcTimeoutMultiplier(1.0) - .setMaxRpcTimeout(totalTimeout) - .build(); + @Rule public MockitoRule mockitoRule = MockitoJUnit.rule().strictness(Strictness.STRICT_STUBS); + @Mock private Marshaller stringMarshaller; + @Mock private RequestParamsExtractor paramsExtractor; + @Mock private ManagedChannel managedChannel; + + private MethodDescriptor methodDescriptor; + private RetrySettings nonRetrySettings; + + @Before + public void setUp() { + methodDescriptor = + MethodDescriptor.newBuilder() + .setSchemaDescriptor("yaml") + .setFullMethodName("fake.test/RingRing") + .setResponseMarshaller(stringMarshaller) + .setRequestMarshaller(stringMarshaller) + .setType(MethodType.UNARY) + .build(); + nonRetrySettings = + RetrySettings.newBuilder() + .setTotalTimeout(totalTimeout) + .setInitialRetryDelay(Duration.ZERO) + .setRetryDelayMultiplier(1.0) + .setMaxRetryDelay(Duration.ZERO) + .setMaxAttempts(1) + .setJittered(true) + .setInitialRpcTimeout(totalTimeout) + .setRpcTimeoutMultiplier(1.0) + .setMaxRpcTimeout(totalTimeout) + .build(); + } @Test public void testNonRetryUnarySettings() { diff --git a/gax/src/main/java/com/google/api/gax/rpc/Callables.java b/gax/src/main/java/com/google/api/gax/rpc/Callables.java index a66090907..3e63a18d5 100644 --- a/gax/src/main/java/com/google/api/gax/rpc/Callables.java +++ b/gax/src/main/java/com/google/api/gax/rpc/Callables.java @@ -58,6 +58,7 @@ public static UnaryCallable retrying( if (areRetriesDisabled(callSettings.getRetryableCodes(), callSettings.getRetrySettings())) { return innerCallable.withDefaultCallContext( + // When retries are disabled, the total timeout can be treated as the rpc timeout. clientContext .getDefaultCallContext() .withTimeout(callSettings.getRetrySettings().getTotalTimeout())); @@ -81,6 +82,7 @@ public static ServerStreamingCallable ClientContext clientContext) { if (areRetriesDisabled(callSettings.getRetryableCodes(), callSettings.getRetrySettings())) { + // When retries are disabled, the total timeout can be treated as the rpc timeout. return innerCallable.withDefaultCallContext( clientContext .getDefaultCallContext()