From 034923277750171418d74060e225ffd68ef38bf2 Mon Sep 17 00:00:00 2001 From: Mattie Fu Date: Thu, 18 Apr 2024 13:25:44 -0400 Subject: [PATCH] fix: first attempt should use the min of RPC timeout and total timeout (#2641) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Thank you for opening a Pull Request! For general contributing guidelines, please refer to [contributing guide](https://github.com/googleapis/gapic-generator-java/blob/main/CONTRIBUTING.md) Before submitting your PR, there are a few things you can do to make sure it goes smoothly: - [ ] Make sure to open an issue as a [bug/issue](https://github.com/googleapis/gapic-generator-java/issues/new/choose) before writing your code! That way we can discuss the change, evaluate designs, and agree on the general idea - [ ] Ensure the tests and linter pass - [ ] Code coverage does not decrease (if any source code was changed) - [ ] Appropriate docs were updated (if necessary) Fixes #2643 ☕️ --- .../retrying/ExponentialRetryAlgorithm.java | 19 ++++++++- .../api/gax/retrying/RetrySettings.java | 12 ++++++ .../ExponentialRetryAlgorithmTest.java | 42 +++++++++++++++++++ 3 files changed, 71 insertions(+), 2 deletions(-) diff --git a/gax-java/gax/src/main/java/com/google/api/gax/retrying/ExponentialRetryAlgorithm.java b/gax-java/gax/src/main/java/com/google/api/gax/retrying/ExponentialRetryAlgorithm.java index b035246746..96aba4a1ec 100644 --- a/gax-java/gax/src/main/java/com/google/api/gax/retrying/ExponentialRetryAlgorithm.java +++ b/gax-java/gax/src/main/java/com/google/api/gax/retrying/ExponentialRetryAlgorithm.java @@ -70,7 +70,7 @@ public TimedAttemptSettings createFirstAttempt() { return TimedAttemptSettings.newBuilder() .setGlobalSettings(globalSettings) .setRetryDelay(Duration.ZERO) - .setRpcTimeout(globalSettings.getInitialRpcTimeout()) + .setRpcTimeout(getInitialTimeout(globalSettings)) .setRandomizedRetryDelay(Duration.ZERO) .setAttemptCount(0) .setOverallAttemptCount(0) @@ -93,12 +93,13 @@ public TimedAttemptSettings createFirstAttempt(RetryingContext context) { } RetrySettings retrySettings = context.getRetrySettings(); + return TimedAttemptSettings.newBuilder() // Use the given retrySettings rather than the settings this was created with. // Attempts created using the TimedAttemptSettings built here will use these // retrySettings, but a new call will not (unless overridden again). .setGlobalSettings(retrySettings) - .setRpcTimeout(retrySettings.getInitialRpcTimeout()) + .setRpcTimeout(getInitialTimeout(retrySettings)) .setRetryDelay(Duration.ZERO) .setRandomizedRetryDelay(Duration.ZERO) .setAttemptCount(0) @@ -261,4 +262,18 @@ protected long nextRandomLong(long bound) { ? ThreadLocalRandom.current().nextLong(bound) : bound; } + + /** + * Returns the timeout of the first attempt. The initial timeout will be min(initialRpcTimeout, + * totalTimeout) if totalTimeout is set. + */ + private Duration getInitialTimeout(RetrySettings retrySettings) { + // If the totalTimeout is zero (not set), then retries are capped by the max attempt + // number. The first attempt will use the initialRpcTimeout value for RPC timeout. + long totalTimeout = retrySettings.getTotalTimeout().toMillis(); + return totalTimeout == 0 + ? retrySettings.getInitialRpcTimeout() + : Duration.ofMillis( + Math.min(retrySettings.getInitialRpcTimeout().toMillis(), totalTimeout)); + } } diff --git a/gax-java/gax/src/main/java/com/google/api/gax/retrying/RetrySettings.java b/gax-java/gax/src/main/java/com/google/api/gax/retrying/RetrySettings.java index caf25725dc..12774a81b0 100644 --- a/gax-java/gax/src/main/java/com/google/api/gax/retrying/RetrySettings.java +++ b/gax-java/gax/src/main/java/com/google/api/gax/retrying/RetrySettings.java @@ -158,6 +158,10 @@ public abstract class RetrySettings implements Serializable { * Duration.ZERO} allows the RPC to continue indefinitely (until it hits a Connect Timeout or the * connection has been terminated). * + *

{@link #getTotalTimeout()} caps how long the logic should keep trying the RPC until it gives + * up completely. If {@link #getTotalTimeout()} is set, initialRpcTimeout should be <= + * totalTimeout. + * *

If there are no configurations, Retries have the default initial RPC timeout value of {@code * Duration.ZERO}. LRO polling does not use the Initial RPC Timeout value. */ @@ -283,6 +287,10 @@ public abstract static class Builder { * Duration.ZERO} allows the RPC to continue indefinitely (until it hits a Connect Timeout or * the connection has been terminated). * + *

{@link #getTotalTimeout()} caps how long the logic should keep trying the RPC until it + * gives up completely. If {@link #getTotalTimeout()} is set, initialRpcTimeout should be <= + * totalTimeout. + * *

If there are no configurations, Retries have the default initial RPC timeout value of * {@code Duration.ZERO}. LRO polling does not use the Initial RPC Timeout value. */ @@ -382,6 +390,10 @@ public abstract static class Builder { * Duration.ZERO} allows the RPC to continue indefinitely (until it hits a Connect Timeout or * the connection has been terminated). * + *

{@link #getTotalTimeout()} caps how long the logic should keep trying the RPC until it + * gives up completely. If {@link #getTotalTimeout()} is set, initialRpcTimeout should be <= + * totalTimeout. + * *

If there are no configurations, Retries have the default initial RPC timeout value of * {@code Duration.ZERO}. LRO polling does not use the Initial RPC Timeout value. */ diff --git a/gax-java/gax/src/test/java/com/google/api/gax/retrying/ExponentialRetryAlgorithmTest.java b/gax-java/gax/src/test/java/com/google/api/gax/retrying/ExponentialRetryAlgorithmTest.java index d0c1ee3ed9..ca39a429cd 100644 --- a/gax-java/gax/src/test/java/com/google/api/gax/retrying/ExponentialRetryAlgorithmTest.java +++ b/gax-java/gax/src/test/java/com/google/api/gax/retrying/ExponentialRetryAlgorithmTest.java @@ -97,6 +97,48 @@ public void testCreateFirstAttemptOverride() { assertEquals(Duration.ZERO, attempt.getRetryDelay()); } + @Test + public void testCreateFirstAttemptHasCorrectTimeout() { + long rpcTimeout = 100; + long totalTimeout = 10; + RetrySettings retrySettings = + RetrySettings.newBuilder() + .setMaxAttempts(6) + .setInitialRetryDelay(Duration.ofMillis(1L)) + .setRetryDelayMultiplier(2.0) + .setMaxRetryDelay(Duration.ofMillis(8L)) + .setInitialRpcTimeout(Duration.ofMillis(rpcTimeout)) + .setRpcTimeoutMultiplier(1.0) + .setMaxRpcTimeout(Duration.ofMillis(rpcTimeout)) + .setTotalTimeout(Duration.ofMillis(totalTimeout)) + .build(); + + ExponentialRetryAlgorithm algorithm = new ExponentialRetryAlgorithm(retrySettings, clock); + + TimedAttemptSettings attempt = algorithm.createFirstAttempt(); + assertEquals(Duration.ofMillis(totalTimeout), attempt.getRpcTimeout()); + + long overrideRpcTimeout = 100; + long overrideTotalTimeout = 20; + RetrySettings retrySettingsOverride = + retrySettings + .toBuilder() + .setInitialRpcTimeout(Duration.ofMillis(overrideRpcTimeout)) + .setMaxRpcTimeout(Duration.ofMillis(overrideRpcTimeout)) + .setTotalTimeout(Duration.ofMillis(overrideTotalTimeout)) + .build(); + RetryingContext retryingContext = + FakeCallContext.createDefault().withRetrySettings(retrySettingsOverride); + attempt = algorithm.createFirstAttempt(retryingContext); + assertEquals(Duration.ofMillis(overrideTotalTimeout), attempt.getRpcTimeout()); + + RetrySettings noTotalTimeout = retrySettings.toBuilder().setTotalTimeout(Duration.ZERO).build(); + + algorithm = new ExponentialRetryAlgorithm(noTotalTimeout, clock); + attempt = algorithm.createFirstAttempt(); + assertEquals(attempt.getRpcTimeout(), retrySettings.getInitialRpcTimeout()); + } + @Test public void testCreateNextAttempt() { TimedAttemptSettings firstAttempt = algorithm.createFirstAttempt();