From 6346b66428c24a0bc507da2c81e3231731e71f7a Mon Sep 17 00:00:00 2001 From: vam Date: Thu, 11 Oct 2018 13:31:39 -0700 Subject: [PATCH 1/3] Fix rpcTimeout calculation in retrying logic --- .../retrying/ExponentialRetryAlgorithm.java | 17 ++- .../ExponentialRetryAlgorithmTest.java | 122 ++++++++++++++++++ .../gax/rpc/OperationCallableImplTest.java | 8 +- 3 files changed, 138 insertions(+), 9 deletions(-) create mode 100644 gax/src/test/java/com/google/api/gax/retrying/ExponentialRetryAlgorithmTest.java diff --git a/gax/src/main/java/com/google/api/gax/retrying/ExponentialRetryAlgorithm.java b/gax/src/main/java/com/google/api/gax/retrying/ExponentialRetryAlgorithm.java index 04c4a1334..976f0c19c 100644 --- a/gax/src/main/java/com/google/api/gax/retrying/ExponentialRetryAlgorithm.java +++ b/gax/src/main/java/com/google/api/gax/retrying/ExponentialRetryAlgorithm.java @@ -88,18 +88,25 @@ public TimedAttemptSettings createFirstAttempt() { public TimedAttemptSettings createNextAttempt(TimedAttemptSettings prevSettings) { RetrySettings settings = prevSettings.getGlobalSettings(); + // The retry delay is determined 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 determined 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)) diff --git a/gax/src/test/java/com/google/api/gax/retrying/ExponentialRetryAlgorithmTest.java b/gax/src/test/java/com/google/api/gax/retrying/ExponentialRetryAlgorithmTest.java new file mode 100644 index 000000000..0f0d32b77 --- /dev/null +++ b/gax/src/test/java/com/google/api/gax/retrying/ExponentialRetryAlgorithmTest.java @@ -0,0 +1,122 @@ +/* + * Copyright 2017, Google LLC All rights reserved. + * + * Redistribution and use in source and binary forms, with or without + * modification, are permitted provided that the following conditions are + * met: + * + * * Redistributions of source code must retain the above copyright + * notice, this list of conditions and the following disclaimer. + * * Redistributions in binary form must reproduce the above + * copyright notice, this list of conditions and the following disclaimer + * in the documentation and/or other materials provided with the + * distribution. + * * Neither the name of Google LLC nor the names of its + * contributors may be used to endorse or promote products derived from + * this software without specific prior written permission. + * + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS + * "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT + * LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR + * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT + * OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, + * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT + * LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, + * DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY + * THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT + * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE + * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. + */ +package com.google.api.gax.retrying; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertTrue; + +import com.google.api.gax.core.FakeApiClock; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.junit.runners.JUnit4; +import org.threeten.bp.Duration; + +@RunWith(JUnit4.class) +public class ExponentialRetryAlgorithmTest { + private final FakeApiClock clock = new FakeApiClock(0L); + private final RetrySettings retrySettings = + RetrySettings.newBuilder() + .setMaxAttempts(6) + .setInitialRetryDelay(Duration.ofMillis(1L)) + .setRetryDelayMultiplier(2.0) + .setMaxRetryDelay(Duration.ofMillis(8L)) + .setInitialRpcTimeout(Duration.ofMillis(1L)) + .setJittered(false) + .setRpcTimeoutMultiplier(2.0) + .setMaxRpcTimeout(Duration.ofMillis(8L)) + .setTotalTimeout(Duration.ofMillis(200L)) + .build(); + private final ExponentialRetryAlgorithm algorithm = + new ExponentialRetryAlgorithm(retrySettings, clock); + + @Test + public void testCreateFirstAttempt() { + TimedAttemptSettings attempt = algorithm.createFirstAttempt(); + + // Checking only the most core values, to not make this test too implementation specific. + assertEquals(0, attempt.getAttemptCount()); + assertEquals(Duration.ZERO, attempt.getRetryDelay()); + assertEquals(Duration.ZERO, attempt.getRandomizedRetryDelay()); + assertEquals(Duration.ofMillis(1L), attempt.getRpcTimeout()); + assertEquals(Duration.ZERO, attempt.getRetryDelay()); + } + + @Test + public void testCreateNextAttempt() { + TimedAttemptSettings firstAttempt = algorithm.createFirstAttempt(); + TimedAttemptSettings secondAttempt = algorithm.createNextAttempt(firstAttempt); + + // Checking only the most core values, to not make this test too implementation specific. + assertEquals(1, secondAttempt.getAttemptCount()); + assertEquals(Duration.ofMillis(1L), secondAttempt.getRetryDelay()); + assertEquals(Duration.ofMillis(1L), secondAttempt.getRandomizedRetryDelay()); + assertEquals(Duration.ofMillis(2L), secondAttempt.getRpcTimeout()); + + TimedAttemptSettings thirdAttempt = algorithm.createNextAttempt(secondAttempt); + assertEquals(2, thirdAttempt.getAttemptCount()); + assertEquals(Duration.ofMillis(2L), thirdAttempt.getRetryDelay()); + assertEquals(Duration.ofMillis(2L), thirdAttempt.getRandomizedRetryDelay()); + assertEquals(Duration.ofMillis(4L), thirdAttempt.getRpcTimeout()); + } + + @Test + public void testShouldRetryTrue() { + TimedAttemptSettings attempt = algorithm.createFirstAttempt(); + for (int i = 0; i < 2; i++) { + attempt = algorithm.createNextAttempt(attempt); + } + + assertTrue(algorithm.shouldRetry(attempt)); + } + + @Test + public void testShouldRetryFalseOnMaxAttempts() { + TimedAttemptSettings attempt = algorithm.createFirstAttempt(); + for (int i = 0; i < 6; i++) { + assertTrue(algorithm.shouldRetry(attempt)); + attempt = algorithm.createNextAttempt(attempt); + } + + assertFalse(algorithm.shouldRetry(attempt)); + } + + @Test + public void testShouldRetryFalseMaxTimeout() { + TimedAttemptSettings attempt = algorithm.createFirstAttempt(); + for (int i = 0; i < 4; i++) { + assertTrue(algorithm.shouldRetry(attempt)); + attempt = algorithm.createNextAttempt(attempt); + clock.incrementNanoTime(Duration.ofMillis(50L).toNanos()); + } + + assertFalse(algorithm.shouldRetry(attempt)); + } +} diff --git a/gax/src/test/java/com/google/api/gax/rpc/OperationCallableImplTest.java b/gax/src/test/java/com/google/api/gax/rpc/OperationCallableImplTest.java index b4121ba1e..911f2bb8e 100644 --- a/gax/src/test/java/com/google/api/gax/rpc/OperationCallableImplTest.java +++ b/gax/src/test/java/com/google/api/gax/rpc/OperationCallableImplTest.java @@ -515,14 +515,14 @@ public void testFutureCallPollRPCTimeout() throws Exception { for (ApiCallContext callContext : callContextCaptor.getAllValues()) { actualTimeouts.add(callContext.getTimeout()); } - assertThat(actualTimeouts).containsAllOf(Duration.ofMillis(100), Duration.ofMillis(200)); + + List 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); From 7212fda2c1295ea309e0b6136f5b34031a9943c0 Mon Sep 17 00:00:00 2001 From: vam Date: Thu, 11 Oct 2018 13:35:58 -0700 Subject: [PATCH 2/3] Fix test method name --- .../google/api/gax/retrying/ExponentialRetryAlgorithmTest.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/gax/src/test/java/com/google/api/gax/retrying/ExponentialRetryAlgorithmTest.java b/gax/src/test/java/com/google/api/gax/retrying/ExponentialRetryAlgorithmTest.java index 0f0d32b77..3adcbe409 100644 --- a/gax/src/test/java/com/google/api/gax/retrying/ExponentialRetryAlgorithmTest.java +++ b/gax/src/test/java/com/google/api/gax/retrying/ExponentialRetryAlgorithmTest.java @@ -109,7 +109,7 @@ public void testShouldRetryFalseOnMaxAttempts() { } @Test - public void testShouldRetryFalseMaxTimeout() { + public void testShouldRetryFalseOnMaxTimeout() { TimedAttemptSettings attempt = algorithm.createFirstAttempt(); for (int i = 0; i < 4; i++) { assertTrue(algorithm.shouldRetry(attempt)); From afd7489a7227d17f8f90608ec0c12be22f51ec4c Mon Sep 17 00:00:00 2001 From: vam Date: Thu, 11 Oct 2018 13:40:03 -0700 Subject: [PATCH 3/3] Fix license header --- .../google/api/gax/retrying/ExponentialRetryAlgorithmTest.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/gax/src/test/java/com/google/api/gax/retrying/ExponentialRetryAlgorithmTest.java b/gax/src/test/java/com/google/api/gax/retrying/ExponentialRetryAlgorithmTest.java index 3adcbe409..418aad7e9 100644 --- a/gax/src/test/java/com/google/api/gax/retrying/ExponentialRetryAlgorithmTest.java +++ b/gax/src/test/java/com/google/api/gax/retrying/ExponentialRetryAlgorithmTest.java @@ -1,5 +1,5 @@ /* - * Copyright 2017, Google LLC All rights reserved. + * Copyright 2018 Google LLC * * Redistribution and use in source and binary forms, with or without * modification, are permitted provided that the following conditions are