Skip to content

Commit 806a52e

Browse files
mutianfmutianf
authored
fix: first attempt should use the min of RPC timeout and total timeout (#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 ☕️
1 parent 54d9508 commit 806a52e

File tree

3 files changed

+71
-2
lines changed

3 files changed

+71
-2
lines changed

‎sdk-platform-java/gax-java/gax/src/main/java/com/google/api/gax/retrying/ExponentialRetryAlgorithm.java‎

Lines changed: 17 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -70,7 +70,7 @@ public TimedAttemptSettings createFirstAttempt() {
7070
return TimedAttemptSettings.newBuilder()
7171
.setGlobalSettings(globalSettings)
7272
.setRetryDelay(Duration.ZERO)
73-
.setRpcTimeout(globalSettings.getInitialRpcTimeout())
73+
.setRpcTimeout(getInitialTimeout(globalSettings))
7474
.setRandomizedRetryDelay(Duration.ZERO)
7575
.setAttemptCount(0)
7676
.setOverallAttemptCount(0)
@@ -93,12 +93,13 @@ public TimedAttemptSettings createFirstAttempt(RetryingContext context) {
9393
}
9494

9595
RetrySettings retrySettings = context.getRetrySettings();
96+
9697
return TimedAttemptSettings.newBuilder()
9798
// Use the given retrySettings rather than the settings this was created with.
9899
// Attempts created using the TimedAttemptSettings built here will use these
99100
// retrySettings, but a new call will not (unless overridden again).
100101
.setGlobalSettings(retrySettings)
101-
.setRpcTimeout(retrySettings.getInitialRpcTimeout())
102+
.setRpcTimeout(getInitialTimeout(retrySettings))
102103
.setRetryDelay(Duration.ZERO)
103104
.setRandomizedRetryDelay(Duration.ZERO)
104105
.setAttemptCount(0)
@@ -261,4 +262,18 @@ protected long nextRandomLong(long bound) {
261262
? ThreadLocalRandom.current().nextLong(bound)
262263
: bound;
263264
}
265+
266+
/**
267+
* Returns the timeout of the first attempt. The initial timeout will be min(initialRpcTimeout,
268+
* totalTimeout) if totalTimeout is set.
269+
*/
270+
private Duration getInitialTimeout(RetrySettings retrySettings) {
271+
// If the totalTimeout is zero (not set), then retries are capped by the max attempt
272+
// number. The first attempt will use the initialRpcTimeout value for RPC timeout.
273+
long totalTimeout = retrySettings.getTotalTimeout().toMillis();
274+
return totalTimeout == 0
275+
? retrySettings.getInitialRpcTimeout()
276+
: Duration.ofMillis(
277+
Math.min(retrySettings.getInitialRpcTimeout().toMillis(), totalTimeout));
278+
}
264279
}

‎sdk-platform-java/gax-java/gax/src/main/java/com/google/api/gax/retrying/RetrySettings.java‎

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -158,6 +158,10 @@ public abstract class RetrySettings implements Serializable {
158158
* Duration.ZERO} allows the RPC to continue indefinitely (until it hits a Connect Timeout or the
159159
* connection has been terminated).
160160
*
161+
* <p>{@link #getTotalTimeout()} caps how long the logic should keep trying the RPC until it gives
162+
* up completely. If {@link #getTotalTimeout()} is set, initialRpcTimeout should be <=
163+
* totalTimeout.
164+
*
161165
* <p>If there are no configurations, Retries have the default initial RPC timeout value of {@code
162166
* Duration.ZERO}. LRO polling does not use the Initial RPC Timeout value.
163167
*/
@@ -283,6 +287,10 @@ public abstract static class Builder {
283287
* Duration.ZERO} allows the RPC to continue indefinitely (until it hits a Connect Timeout or
284288
* the connection has been terminated).
285289
*
290+
* <p>{@link #getTotalTimeout()} caps how long the logic should keep trying the RPC until it
291+
* gives up completely. If {@link #getTotalTimeout()} is set, initialRpcTimeout should be <=
292+
* totalTimeout.
293+
*
286294
* <p>If there are no configurations, Retries have the default initial RPC timeout value of
287295
* {@code Duration.ZERO}. LRO polling does not use the Initial RPC Timeout value.
288296
*/
@@ -382,6 +390,10 @@ public abstract static class Builder {
382390
* Duration.ZERO} allows the RPC to continue indefinitely (until it hits a Connect Timeout or
383391
* the connection has been terminated).
384392
*
393+
* <p>{@link #getTotalTimeout()} caps how long the logic should keep trying the RPC until it
394+
* gives up completely. If {@link #getTotalTimeout()} is set, initialRpcTimeout should be <=
395+
* totalTimeout.
396+
*
385397
* <p>If there are no configurations, Retries have the default initial RPC timeout value of
386398
* {@code Duration.ZERO}. LRO polling does not use the Initial RPC Timeout value.
387399
*/

‎sdk-platform-java/gax-java/gax/src/test/java/com/google/api/gax/retrying/ExponentialRetryAlgorithmTest.java‎

Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -97,6 +97,48 @@ public void testCreateFirstAttemptOverride() {
9797
assertEquals(Duration.ZERO, attempt.getRetryDelay());
9898
}
9999

100+
@Test
101+
public void testCreateFirstAttemptHasCorrectTimeout() {
102+
long rpcTimeout = 100;
103+
long totalTimeout = 10;
104+
RetrySettings retrySettings =
105+
RetrySettings.newBuilder()
106+
.setMaxAttempts(6)
107+
.setInitialRetryDelay(Duration.ofMillis(1L))
108+
.setRetryDelayMultiplier(2.0)
109+
.setMaxRetryDelay(Duration.ofMillis(8L))
110+
.setInitialRpcTimeout(Duration.ofMillis(rpcTimeout))
111+
.setRpcTimeoutMultiplier(1.0)
112+
.setMaxRpcTimeout(Duration.ofMillis(rpcTimeout))
113+
.setTotalTimeout(Duration.ofMillis(totalTimeout))
114+
.build();
115+
116+
ExponentialRetryAlgorithm algorithm = new ExponentialRetryAlgorithm(retrySettings, clock);
117+
118+
TimedAttemptSettings attempt = algorithm.createFirstAttempt();
119+
assertEquals(Duration.ofMillis(totalTimeout), attempt.getRpcTimeout());
120+
121+
long overrideRpcTimeout = 100;
122+
long overrideTotalTimeout = 20;
123+
RetrySettings retrySettingsOverride =
124+
retrySettings
125+
.toBuilder()
126+
.setInitialRpcTimeout(Duration.ofMillis(overrideRpcTimeout))
127+
.setMaxRpcTimeout(Duration.ofMillis(overrideRpcTimeout))
128+
.setTotalTimeout(Duration.ofMillis(overrideTotalTimeout))
129+
.build();
130+
RetryingContext retryingContext =
131+
FakeCallContext.createDefault().withRetrySettings(retrySettingsOverride);
132+
attempt = algorithm.createFirstAttempt(retryingContext);
133+
assertEquals(Duration.ofMillis(overrideTotalTimeout), attempt.getRpcTimeout());
134+
135+
RetrySettings noTotalTimeout = retrySettings.toBuilder().setTotalTimeout(Duration.ZERO).build();
136+
137+
algorithm = new ExponentialRetryAlgorithm(noTotalTimeout, clock);
138+
attempt = algorithm.createFirstAttempt();
139+
assertEquals(attempt.getRpcTimeout(), retrySettings.getInitialRpcTimeout());
140+
}
141+
100142
@Test
101143
public void testCreateNextAttempt() {
102144
TimedAttemptSettings firstAttempt = algorithm.createFirstAttempt();

0 commit comments

Comments
 (0)