From e57452a1de6853ee4d3cbe6977c9a268404a1e76 Mon Sep 17 00:00:00 2001 From: Igor Bernstein Date: Mon, 6 Apr 2020 11:55:51 -0400 Subject: [PATCH] feat: add ability to partially update retry settings (#993) * feat: add ability to partially update retry settings * Fix mutability of ServerStreamingCallSettings.retryCodes * fix tests * address feedback * doc * fix bad rebase --- .../com/google/api/gax/grpc/SettingsTest.java | 8 ++- .../gax/rpc/ServerStreamingCallSettings.java | 35 ++++++++--- .../google/api/gax/rpc/UnaryCallSettings.java | 39 ++++++++++-- .../api/gax/rpc/BatchingCallSettingsTest.java | 31 ++++++++-- .../api/gax/rpc/PagedCallSettingsTest.java | 33 ++++++++-- .../rpc/ServerStreamingCallSettingsTest.java | 62 ++++++++++++++----- .../api/gax/rpc/UnaryCallSettingsTest.java | 35 +++++++++-- 7 files changed, 195 insertions(+), 48 deletions(-) diff --git a/gax-grpc/src/test/java/com/google/api/gax/grpc/SettingsTest.java b/gax-grpc/src/test/java/com/google/api/gax/grpc/SettingsTest.java index 778e4c908..3c32d049a 100644 --- a/gax-grpc/src/test/java/com/google/api/gax/grpc/SettingsTest.java +++ b/gax-grpc/src/test/java/com/google/api/gax/grpc/SettingsTest.java @@ -634,7 +634,7 @@ private static void assertIsReflectionEqual( UnaryCallSettings.Builder builderB) { assertIsReflectionEqual(path, builderA, builderB, new String[] {"retrySettingsBuilder"}); assertIsReflectionEqual( - path + ".getRetrySettings", builderA.getRetrySettings(), builderB.getRetrySettings()); + path + ".retrySettingsBuilder", builderA.getRetrySettings(), builderB.getRetrySettings()); } private static void assertIsReflectionEqual( @@ -664,10 +664,12 @@ private static void assertIsReflectionEqual( path, builderA, builderB, - new String[] {"retrySettings", "batchingDescriptor", "batchingSettings", "flowController"}); + new String[] { + "retrySettingsBuilder", "batchingDescriptor", "batchingSettings", "flowController" + }); assertIsReflectionEqual( - path + ".getRetrySettings", builderA.getRetrySettings(), builderB.getRetrySettings()); + path + ".retrySettingsBuilder", builderA.getRetrySettings(), builderB.getRetrySettings()); assertIsReflectionEqual( path + ".getBatchingSettings", builderA.getBatchingSettings(), diff --git a/gax/src/main/java/com/google/api/gax/rpc/ServerStreamingCallSettings.java b/gax/src/main/java/com/google/api/gax/rpc/ServerStreamingCallSettings.java index 9025625e3..3fd29550a 100644 --- a/gax/src/main/java/com/google/api/gax/rpc/ServerStreamingCallSettings.java +++ b/gax/src/main/java/com/google/api/gax/rpc/ServerStreamingCallSettings.java @@ -83,7 +83,7 @@ public final class ServerStreamingCallSettings private ServerStreamingCallSettings(Builder builder) { this.retryableCodes = ImmutableSet.copyOf(builder.retryableCodes); - this.retrySettings = builder.retrySettings; + this.retrySettings = builder.retrySettingsBuilder.build(); this.resumptionStrategy = builder.resumptionStrategy; this.idleTimeout = builder.idleTimeout; } @@ -135,7 +135,7 @@ public static Builder newBuilder() { public static class Builder extends StreamingCallSettings.Builder { @Nonnull private Set retryableCodes; - @Nonnull private RetrySettings retrySettings; + @Nonnull private RetrySettings.Builder retrySettingsBuilder; @Nonnull private StreamResumptionStrategy resumptionStrategy; @Nonnull private Duration idleTimeout; @@ -143,7 +143,7 @@ public static class Builder /** Initialize the builder with default settings */ private Builder() { this.retryableCodes = ImmutableSet.of(); - this.retrySettings = RetrySettings.newBuilder().build(); + this.retrySettingsBuilder = RetrySettings.newBuilder(); this.resumptionStrategy = new SimpleStreamResumptionStrategy<>(); this.idleTimeout = Duration.ZERO; @@ -152,7 +152,7 @@ private Builder() { private Builder(ServerStreamingCallSettings settings) { super(settings); this.retryableCodes = settings.retryableCodes; - this.retrySettings = settings.retrySettings; + this.retrySettingsBuilder = settings.retrySettings.toBuilder(); this.resumptionStrategy = settings.resumptionStrategy; this.idleTimeout = settings.idleTimeout; @@ -183,18 +183,37 @@ public Set getRetryableCodes() { } /** - * See the class documentation of {@link ServerStreamingCallSettings} for a description of what - * retrySettings do. + * Returns the underlying {@link RetrySettings.Builder}, which allows callers to augment the + * existing {@link RetrySettings}. + */ + public RetrySettings.Builder retrySettings() { + return this.retrySettingsBuilder; + } + + /** + * Replaces the {@link RetrySettings} for the associated {@link ServerStreamingCallable}. + * + *

When using the method, make sure that the {@link RetrySettings} are complete. For example, + * the following code will disable retries because the retry delay is not set: + * + *

{@code
+     * stubSettings.setRetrySettings(
+     *   RetrySettings.newBuilder()
+     *     .setTotalTimeout(Duration.ofSeconds(10)
+     * );
+     * }
+ * + * @see #retrySettings() */ public Builder setRetrySettings(@Nonnull RetrySettings retrySettings) { Preconditions.checkNotNull(retrySettings); - this.retrySettings = retrySettings; + this.retrySettingsBuilder = retrySettings.toBuilder(); return this; } @Nonnull public RetrySettings getRetrySettings() { - return retrySettings; + return retrySettingsBuilder.build(); } /** Disables retries and sets the overall timeout. */ diff --git a/gax/src/main/java/com/google/api/gax/rpc/UnaryCallSettings.java b/gax/src/main/java/com/google/api/gax/rpc/UnaryCallSettings.java index 745b90c21..6b218badb 100644 --- a/gax/src/main/java/com/google/api/gax/rpc/UnaryCallSettings.java +++ b/gax/src/main/java/com/google/api/gax/rpc/UnaryCallSettings.java @@ -31,7 +31,6 @@ import com.google.api.core.InternalExtensionOnly; import com.google.api.gax.retrying.RetrySettings; -import com.google.common.base.Preconditions; import com.google.common.collect.ImmutableSet; import com.google.common.collect.Sets; import java.util.Set; @@ -85,7 +84,7 @@ public Builder toBuilder() { protected UnaryCallSettings(Builder builder) { this.retryableCodes = ImmutableSet.copyOf(builder.retryableCodes); - this.retrySettings = builder.retrySettings; + this.retrySettings = builder.retrySettingsBuilder.build(); } /** @@ -98,11 +97,11 @@ protected UnaryCallSettings(Builder builder) { public static class Builder { private Set retryableCodes; - private RetrySettings retrySettings; + private RetrySettings.Builder retrySettingsBuilder; protected Builder() { retryableCodes = Sets.newHashSet(); - retrySettings = RetrySettings.newBuilder().build(); + retrySettingsBuilder = RetrySettings.newBuilder(); } protected Builder(UnaryCallSettings unaryCallSettings) { @@ -130,9 +129,32 @@ public UnaryCallSettings.Builder setRetryableCodes( return this; } + /** + * Returns the underlying {@link RetrySettings.Builder}, which allows callers to augment the + * existing {@link RetrySettings}. + */ + public RetrySettings.Builder retrySettings() { + return this.retrySettingsBuilder; + } + + /** + * Replaces the {@link RetrySettings} for the associated {@link UnaryCallable}. + * + *

When using the method, make sure that the {@link RetrySettings} are complete. For example, + * the following code will disable retries because the retry delay is not set: + * + *

{@code
+     * stubSettings.setRetrySettings(
+     *   RetrySettings.newBuilder()
+     *     .setTotalTimeout(Duration.ofSeconds(10)
+     * );
+     * }
+ * + * @see #retrySettings() + */ public UnaryCallSettings.Builder setRetrySettings( RetrySettings retrySettings) { - this.retrySettings = Preconditions.checkNotNull(retrySettings); + this.retrySettingsBuilder = retrySettings.toBuilder(); return this; } @@ -162,8 +184,13 @@ public Set getRetryableCodes() { return this.retryableCodes; } + /** + * Returns an immutable {@link RetrySettings} currently set in this Builder. + * + *

Unlike {@link #retrySettings()}, objects returned by this method are frozen in time. + */ public RetrySettings getRetrySettings() { - return this.retrySettings; + return this.retrySettingsBuilder.build(); } /** diff --git a/gax/src/test/java/com/google/api/gax/rpc/BatchingCallSettingsTest.java b/gax/src/test/java/com/google/api/gax/rpc/BatchingCallSettingsTest.java index 234ff707f..215d6da6d 100644 --- a/gax/src/test/java/com/google/api/gax/rpc/BatchingCallSettingsTest.java +++ b/gax/src/test/java/com/google/api/gax/rpc/BatchingCallSettingsTest.java @@ -40,6 +40,7 @@ import org.junit.runner.RunWith; import org.junit.runners.JUnit4; import org.mockito.Mockito; +import org.threeten.bp.Duration; @RunWith(JUnit4.class) public class BatchingCallSettingsTest { @@ -82,7 +83,17 @@ public void testBuilder() { BatchingSettings.newBuilder().setElementCountThreshold(1L).build(); FlowController flowController = Mockito.mock(FlowController.class); Set retryCodes = Sets.newHashSet(Code.UNAVAILABLE); - RetrySettings retrySettings = RetrySettings.newBuilder().build(); + RetrySettings retrySettings = + RetrySettings.newBuilder() + .setInitialRetryDelay(Duration.ofMillis(5)) + .setMaxRetryDelay(Duration.ofSeconds(1)) + .setRetryDelayMultiplier(2) + .setInitialRpcTimeout(Duration.ofMillis(100)) + .setMaxRpcTimeout(Duration.ofMillis(200)) + .setRpcTimeoutMultiplier(1.1) + .setJittered(true) + .setMaxAttempts(10) + .build(); builder.setBatchingSettings(batchingSettings); builder.setFlowController(flowController); @@ -93,7 +104,7 @@ public void testBuilder() { Truth.assertThat(builder.getBatchingSettings()).isSameInstanceAs(batchingSettings); Truth.assertThat(builder.getFlowController()).isSameInstanceAs(flowController); Truth.assertThat(builder.getRetryableCodes().size()).isEqualTo(1); - Truth.assertThat(builder.getRetrySettings()).isSameInstanceAs(retrySettings); + Truth.assertThat(builder.getRetrySettings()).isEqualTo(retrySettings); BatchingCallSettings settings = builder.build(); @@ -101,7 +112,7 @@ public void testBuilder() { Truth.assertThat(settings.getBatchingSettings()).isSameInstanceAs(batchingSettings); Truth.assertThat(settings.getFlowController()).isSameInstanceAs(flowController); Truth.assertThat(settings.getRetryableCodes().size()).isEqualTo(1); - Truth.assertThat(settings.getRetrySettings()).isSameInstanceAs(retrySettings); + Truth.assertThat(settings.getRetrySettings()).isEqualTo(retrySettings); } @Test @@ -116,7 +127,17 @@ public void testBuilderFromSettings() throws Exception { BatchingSettings.newBuilder().setElementCountThreshold(1L).build(); FlowController flowController = Mockito.mock(FlowController.class); Set retryCodes = Sets.newHashSet(Code.UNAVAILABLE); - RetrySettings retrySettings = RetrySettings.newBuilder().build(); + RetrySettings retrySettings = + RetrySettings.newBuilder() + .setInitialRetryDelay(Duration.ofMillis(5)) + .setMaxRetryDelay(Duration.ofSeconds(1)) + .setRetryDelayMultiplier(2) + .setInitialRpcTimeout(Duration.ofMillis(100)) + .setMaxRpcTimeout(Duration.ofMillis(200)) + .setRpcTimeoutMultiplier(1.1) + .setJittered(true) + .setMaxAttempts(10) + .build(); builder.setBatchingSettings(batchingSettings); builder.setFlowController(flowController); @@ -130,7 +151,7 @@ public void testBuilderFromSettings() throws Exception { Truth.assertThat(newBuilder.getBatchingSettings()).isSameInstanceAs(batchingSettings); Truth.assertThat(newBuilder.getFlowController()).isSameInstanceAs(flowController); Truth.assertThat(newBuilder.getRetryableCodes().size()).isEqualTo(1); - Truth.assertThat(newBuilder.getRetrySettings()).isSameInstanceAs(retrySettings); + Truth.assertThat(newBuilder.getRetrySettings()).isEqualTo(retrySettings); } @Test diff --git a/gax/src/test/java/com/google/api/gax/rpc/PagedCallSettingsTest.java b/gax/src/test/java/com/google/api/gax/rpc/PagedCallSettingsTest.java index e3314e0a7..25c6b8064 100644 --- a/gax/src/test/java/com/google/api/gax/rpc/PagedCallSettingsTest.java +++ b/gax/src/test/java/com/google/api/gax/rpc/PagedCallSettingsTest.java @@ -38,6 +38,7 @@ import org.junit.runner.RunWith; import org.junit.runners.JUnit4; import org.mockito.Mockito; +import org.threeten.bp.Duration; @RunWith(JUnit4.class) public class PagedCallSettingsTest { @@ -72,18 +73,28 @@ public void testBuilder() { PagedCallSettings.newBuilder(pagedListResponseFactory); Set retryCodes = Sets.newHashSet(Code.UNAVAILABLE); - RetrySettings retrySettings = RetrySettings.newBuilder().build(); + RetrySettings retrySettings = + RetrySettings.newBuilder() + .setInitialRetryDelay(Duration.ofMillis(5)) + .setMaxRetryDelay(Duration.ofSeconds(1)) + .setRetryDelayMultiplier(2) + .setInitialRpcTimeout(Duration.ofMillis(100)) + .setMaxRpcTimeout(Duration.ofMillis(200)) + .setRpcTimeoutMultiplier(1.1) + .setJittered(true) + .setMaxAttempts(10) + .build(); builder.setRetryableCodes(retryCodes); builder.setRetrySettings(retrySettings); Truth.assertThat(builder.getRetryableCodes().size()).isEqualTo(1); - Truth.assertThat(builder.getRetrySettings()).isSameInstanceAs(retrySettings); + Truth.assertThat(builder.getRetrySettings()).isEqualTo(retrySettings); PagedCallSettings settings = builder.build(); Truth.assertThat(settings.getRetryableCodes().size()).isEqualTo(1); - Truth.assertThat(settings.getRetrySettings()).isSameInstanceAs(retrySettings); + Truth.assertThat(settings.getRetrySettings()).isEqualTo(retrySettings); } @Test @@ -95,18 +106,28 @@ public void testBuilderFromSettings() throws Exception { PagedCallSettings.newBuilder(pagedListResponseFactory); Set retryCodes = Sets.newHashSet(Code.UNAVAILABLE); - RetrySettings retrySettings = RetrySettings.newBuilder().build(); + RetrySettings retrySettings = + RetrySettings.newBuilder() + .setInitialRetryDelay(Duration.ofMillis(5)) + .setMaxRetryDelay(Duration.ofSeconds(1)) + .setRetryDelayMultiplier(2) + .setInitialRpcTimeout(Duration.ofMillis(100)) + .setMaxRpcTimeout(Duration.ofMillis(200)) + .setRpcTimeoutMultiplier(1.1) + .setJittered(true) + .setMaxAttempts(10) + .build(); builder.setRetryableCodes(retryCodes); builder.setRetrySettings(retrySettings); Truth.assertThat(builder.getRetryableCodes().size()).isEqualTo(1); - Truth.assertThat(builder.getRetrySettings()).isSameInstanceAs(retrySettings); + Truth.assertThat(builder.getRetrySettings()).isEqualTo(retrySettings); PagedCallSettings settings = builder.build(); PagedCallSettings.Builder newBuilder = settings.toBuilder(); Truth.assertThat(newBuilder.getRetryableCodes().size()).isEqualTo(1); - Truth.assertThat(newBuilder.getRetrySettings()).isSameInstanceAs(retrySettings); + Truth.assertThat(newBuilder.getRetrySettings()).isEqualTo(retrySettings); } } diff --git a/gax/src/test/java/com/google/api/gax/rpc/ServerStreamingCallSettingsTest.java b/gax/src/test/java/com/google/api/gax/rpc/ServerStreamingCallSettingsTest.java index 8dfade286..66faca02d 100644 --- a/gax/src/test/java/com/google/api/gax/rpc/ServerStreamingCallSettingsTest.java +++ b/gax/src/test/java/com/google/api/gax/rpc/ServerStreamingCallSettingsTest.java @@ -29,15 +29,15 @@ */ package com.google.api.gax.rpc; +import static com.google.common.truth.Truth.assertThat; + import com.google.api.gax.retrying.RetrySettings; import com.google.api.gax.rpc.StatusCode.Code; import com.google.common.collect.ImmutableSet; -import com.google.common.truth.Truth; import java.util.Set; import org.junit.Test; import org.junit.runner.RunWith; import org.junit.runners.JUnit4; -import org.mockito.Mockito; import org.threeten.bp.Duration; @RunWith(JUnit4.class) @@ -49,10 +49,9 @@ public void retryableCodesAreNotLost() { ServerStreamingCallSettings.newBuilder(); builder.setRetryableCodes(codes); - Truth.assertThat(builder.getRetryableCodes()).containsExactlyElementsIn(codes); - Truth.assertThat(builder.build().getRetryableCodes()).containsExactlyElementsIn(codes); - Truth.assertThat(builder.build().toBuilder().getRetryableCodes()) - .containsExactlyElementsIn(codes); + assertThat(builder.getRetryableCodes()).containsExactlyElementsIn(codes); + assertThat(builder.build().getRetryableCodes()).containsExactlyElementsIn(codes); + assertThat(builder.build().toBuilder().getRetryableCodes()).containsExactlyElementsIn(codes); } @Test @@ -60,21 +59,30 @@ public void retryableCodesVarArgs() { ServerStreamingCallSettings.Builder builder = ServerStreamingCallSettings.newBuilder().setRetryableCodes(Code.UNKNOWN, Code.ABORTED); - Truth.assertThat(builder.getRetryableCodes()).containsExactly(Code.UNKNOWN, Code.ABORTED); + assertThat(builder.getRetryableCodes()).containsExactly(Code.UNKNOWN, Code.ABORTED); } @Test public void retryableSettingsAreNotLost() { - RetrySettings retrySettings = Mockito.mock(RetrySettings.class); + RetrySettings retrySettings = + RetrySettings.newBuilder() + .setInitialRetryDelay(Duration.ofMillis(5)) + .setMaxRetryDelay(Duration.ofSeconds(1)) + .setRetryDelayMultiplier(2) + .setInitialRpcTimeout(Duration.ofMillis(100)) + .setMaxRpcTimeout(Duration.ofMillis(200)) + .setRpcTimeoutMultiplier(1.1) + .setJittered(true) + .setMaxAttempts(10) + .build(); ServerStreamingCallSettings.Builder builder = ServerStreamingCallSettings.newBuilder(); builder.setRetrySettings(retrySettings); - Truth.assertThat(builder.getRetrySettings()).isSameInstanceAs(retrySettings); - Truth.assertThat(builder.build().getRetrySettings()).isSameInstanceAs(retrySettings); - Truth.assertThat(builder.build().toBuilder().getRetrySettings()) - .isSameInstanceAs(retrySettings); + assertThat(builder.getRetrySettings()).isEqualTo(retrySettings); + assertThat(builder.build().getRetrySettings()).isEqualTo(retrySettings); + assertThat(builder.build().toBuilder().getRetrySettings()).isEqualTo(retrySettings); } @Test @@ -85,8 +93,32 @@ public void idleTimeoutIsNotLost() { ServerStreamingCallSettings.newBuilder(); builder.setIdleTimeout(idleTimeout); - Truth.assertThat(builder.getIdleTimeout()).isEqualTo(idleTimeout); - Truth.assertThat(builder.build().getIdleTimeout()).isEqualTo(idleTimeout); - Truth.assertThat(builder.build().toBuilder().getIdleTimeout()).isEqualTo(idleTimeout); + assertThat(builder.getIdleTimeout()).isEqualTo(idleTimeout); + assertThat(builder.build().getIdleTimeout()).isEqualTo(idleTimeout); + assertThat(builder.build().toBuilder().getIdleTimeout()).isEqualTo(idleTimeout); + } + + @Test + public void testRetrySettingsBuilder() { + RetrySettings initialSettings = + RetrySettings.newBuilder() + .setInitialRetryDelay(Duration.ofMillis(5)) + .setMaxRetryDelay(Duration.ofSeconds(1)) + .setRetryDelayMultiplier(2) + .setInitialRpcTimeout(Duration.ofMillis(100)) + .setMaxRpcTimeout(Duration.ofMillis(200)) + .setRpcTimeoutMultiplier(1.1) + .setJittered(true) + .setMaxAttempts(10) + .build(); + + ServerStreamingCallSettings.Builder builder = + ServerStreamingCallSettings.newBuilder().setRetrySettings(initialSettings); + + builder.retrySettings().setMaxRetryDelay(Duration.ofMinutes(1)); + + assertThat(builder.getRetrySettings().getMaxRetryDelay()).isEqualTo(Duration.ofMinutes(1)); + assertThat(builder.build().getRetrySettings().getMaxRetryDelay()) + .isEqualTo(Duration.ofMinutes(1)); } } diff --git a/gax/src/test/java/com/google/api/gax/rpc/UnaryCallSettingsTest.java b/gax/src/test/java/com/google/api/gax/rpc/UnaryCallSettingsTest.java index ee53ea3ef..f20e22fee 100644 --- a/gax/src/test/java/com/google/api/gax/rpc/UnaryCallSettingsTest.java +++ b/gax/src/test/java/com/google/api/gax/rpc/UnaryCallSettingsTest.java @@ -29,7 +29,9 @@ */ package com.google.api.gax.rpc; -import com.google.common.truth.Truth; +import static com.google.common.truth.Truth.assertThat; + +import com.google.api.gax.retrying.RetrySettings; import org.junit.Test; import org.junit.runner.RunWith; import org.junit.runners.JUnit4; @@ -43,9 +45,32 @@ public void testSetSimpleTimeoutNoRetries() { UnaryCallSettings.Builder builder = new UnaryCallSettings.Builder(); builder.setSimpleTimeoutNoRetries(Duration.ofSeconds(13)); - Truth.assertThat(builder.getRetryableCodes().size()).isEqualTo(0); - Truth.assertThat(builder.getRetrySettings().getMaxAttempts()).isEqualTo(1); - Truth.assertThat(builder.getRetrySettings().getTotalTimeout()) - .isEqualTo(Duration.ofSeconds(13)); + assertThat(builder.getRetryableCodes().size()).isEqualTo(0); + assertThat(builder.getRetrySettings().getMaxAttempts()).isEqualTo(1); + assertThat(builder.getRetrySettings().getTotalTimeout()).isEqualTo(Duration.ofSeconds(13)); + } + + @Test + public void testRetrySettingsBuilder() { + RetrySettings initialSettings = + RetrySettings.newBuilder() + .setInitialRetryDelay(Duration.ofMillis(5)) + .setMaxRetryDelay(Duration.ofSeconds(1)) + .setRetryDelayMultiplier(2) + .setInitialRpcTimeout(Duration.ofMillis(100)) + .setMaxRpcTimeout(Duration.ofMillis(200)) + .setRpcTimeoutMultiplier(1.1) + .setJittered(true) + .setMaxAttempts(10) + .build(); + + UnaryCallSettings.Builder builder = + new UnaryCallSettings.Builder().setRetrySettings(initialSettings); + + builder.retrySettings().setMaxRetryDelay(Duration.ofMinutes(1)); + + assertThat(builder.getRetrySettings().getMaxRetryDelay()).isEqualTo(Duration.ofMinutes(1)); + assertThat(builder.build().getRetrySettings().getMaxRetryDelay()) + .isEqualTo(Duration.ofMinutes(1)); } }