Skip to content

Commit

Permalink
fix: first attempt should use the min of RPC timeout and total timeout (
Browse files Browse the repository at this point in the history
#2641)

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 ☕️
  • Loading branch information
mutianf committed Apr 18, 2024
1 parent a75e549 commit 0349232
Show file tree
Hide file tree
Showing 3 changed files with 71 additions and 2 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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)
Expand Down Expand Up @@ -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));
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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).
*
* <p>{@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.
*
* <p>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.
*/
Expand Down Expand Up @@ -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).
*
* <p>{@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.
*
* <p>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.
*/
Expand Down Expand Up @@ -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).
*
* <p>{@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.
*
* <p>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.
*/
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down

0 comments on commit 0349232

Please sign in to comment.