Skip to content

Commit

Permalink
Fix misinterpretation of rpcTimeout in ExponentialRetryAlgorithm
Browse files Browse the repository at this point in the history
  • Loading branch information
vam-google committed Oct 11, 2018
1 parent 70cb018 commit 5dc6661
Show file tree
Hide file tree
Showing 2 changed files with 19 additions and 13 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -88,18 +88,25 @@ public TimedAttemptSettings createFirstAttempt() {
public TimedAttemptSettings createNextAttempt(TimedAttemptSettings prevSettings) {
RetrySettings settings = prevSettings.getGlobalSettings();

// The retry delay is used as follows:
// attempt #0 - not used (initial attempt is always made immediately);
// attempt #1 - use initialRetryDelay;
// attempt #2+ - use the calculated value (i.e. the following if statement is true only
// if we are about to calculate the value for the upcoming 2nd+ attempt).
long newRetryDelay = settings.getInitialRetryDelay().toMillis();
long newRpcTimeout = settings.getInitialRpcTimeout().toMillis();

if (prevSettings.getAttemptCount() > 0) {
newRetryDelay =
(long) (settings.getRetryDelayMultiplier() * prevSettings.getRetryDelay().toMillis());
newRetryDelay = Math.min(newRetryDelay, settings.getMaxRetryDelay().toMillis());
newRpcTimeout =
(long) (settings.getRpcTimeoutMultiplier() * prevSettings.getRpcTimeout().toMillis());
newRpcTimeout = Math.min(newRpcTimeout, settings.getMaxRpcTimeout().toMillis());
}

// The rpc timeout is used as follows:
// attempt #0 - use the initialRpcTimeout;
// attempt #1+ - use the calculated value.
long newRpcTimeout =
(long) (settings.getRpcTimeoutMultiplier() * prevSettings.getRpcTimeout().toMillis());
newRpcTimeout = Math.min(newRpcTimeout, settings.getMaxRpcTimeout().toMillis());

return TimedAttemptSettings.newBuilder()
.setGlobalSettings(prevSettings.getGlobalSettings())
.setRetryDelay(Duration.ofMillis(newRetryDelay))
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -138,6 +138,7 @@ public void setUp() throws IOException {
}

private static class ResponseTransformer implements ApiFunction<OperationSnapshot, Color> {

@Override
public Color apply(OperationSnapshot operationSnapshot) {
if (!operationSnapshot.getErrorCode().getCode().equals(StatusCode.Code.OK)) {
Expand Down Expand Up @@ -169,6 +170,7 @@ public Color apply(OperationSnapshot operationSnapshot) {
}

private static class MetadataTransformer implements ApiFunction<OperationSnapshot, Currency> {

@Override
public Currency apply(OperationSnapshot operationSnapshot) {
if (operationSnapshot.getMetadata() == null) {
Expand Down Expand Up @@ -469,9 +471,6 @@ public void testFutureCallPollDoneOnSecond() throws Exception {

@Test
public void testFutureCallPollRPCTimeout() throws Exception {
// Note: there is a bug in Rechecking callable that will return the initial RPC timeouts
// twice. So, this test works around the issue by polling 3 times and checking for the first
// 2 timeout durations
String opName = "testFutureCallPollRPCTimeout";
pollingAlgorithm =
OperationTimedPollAlgorithm.create(
Expand Down Expand Up @@ -518,18 +517,17 @@ public void testFutureCallPollRPCTimeout() throws Exception {
for (ApiCallContext callContext : callContextCaptor.getAllValues()) {
actualTimeouts.add(callContext.getTimeout());
}
assertThat(actualTimeouts).containsAllOf(Duration.ofMillis(100), Duration.ofMillis(200));

List<Duration> expectedTimeouts =
Lists.newArrayList(Duration.ofMillis(100), Duration.ofMillis(200), Duration.ofMillis(400));
assertThat(actualTimeouts).isEqualTo(expectedTimeouts);
}

@Test
public void testFutureCallContextPropagation() throws Exception {
// Note: there is a bug in Rechecking callable that will return the initial RPC timeouts
// twice. So, this test works around the issue by polling 3 times and checking for the first
// 2 timeout durations
String opName = "testFutureCallContextPropagation";

Color resp = getColor(0.5f);
Currency meta1 = Currency.getInstance("UAH");
Currency meta2 = Currency.getInstance("USD");
OperationSnapshot initialOperation = getOperation(opName, null, null, null, false);
OperationSnapshot resultOperation = getOperation(opName, resp, null, meta2, true);
Expand Down Expand Up @@ -1178,6 +1176,7 @@ public ApiFuture<OperationSnapshot> futureCall(Integer request, ApiCallContext c
}

private class UnsupportedOperationApi implements LongRunningClient {

@Override
public UnaryCallable<String, OperationSnapshot> getOperationCallable() {
throw new UnsupportedOperationException("Didn't expect call to getOperationCallable()");
Expand Down

0 comments on commit 5dc6661

Please sign in to comment.