From a272b6ddebb7b439010722f257bfe570a1767098 Mon Sep 17 00:00:00 2001 From: Simon Moreno <30335873+simorenoh@users.noreply.github.com> Date: Fri, 8 Oct 2021 15:18:10 -0400 Subject: [PATCH] Updated retryWith values and added salting to match compute/.NET behavior (#24619) * updated values and added salt * added tests * removed unused logic * addressed comments * using threadlocalrandom * removed unused import, made random private --- .../GoneAndRetryWithRetryPolicy.java | 8 ++-- .../GoneAndRetryWithRetryPolicyTest.java | 44 +++++++++++++++++++ 2 files changed, 49 insertions(+), 3 deletions(-) diff --git a/sdk/cosmos/azure-cosmos/src/main/java/com/azure/cosmos/implementation/directconnectivity/GoneAndRetryWithRetryPolicy.java b/sdk/cosmos/azure-cosmos/src/main/java/com/azure/cosmos/implementation/directconnectivity/GoneAndRetryWithRetryPolicy.java index 78080a1487840..1aae68b46dc43 100644 --- a/sdk/cosmos/azure-cosmos/src/main/java/com/azure/cosmos/implementation/directconnectivity/GoneAndRetryWithRetryPolicy.java +++ b/sdk/cosmos/azure-cosmos/src/main/java/com/azure/cosmos/implementation/directconnectivity/GoneAndRetryWithRetryPolicy.java @@ -23,6 +23,7 @@ import java.time.Duration; import java.time.Instant; +import java.util.concurrent.ThreadLocalRandom; import static com.azure.cosmos.implementation.guava25.base.Preconditions.checkNotNull; @@ -36,6 +37,7 @@ public class GoneAndRetryWithRetryPolicy implements IRetryPolicy { private volatile RetryWithException lastRetryWithException; private RetryContext retryContext; + private static final ThreadLocalRandom random = ThreadLocalRandom.current(); public GoneAndRetryWithRetryPolicy(RxDocumentServiceRequest request, Integer waitTimeInSeconds) { this.retryContext = BridgeInternal.getRetryContext(request.requestContext.cosmosDiagnostics); @@ -170,7 +172,6 @@ public Mono shouldRetry(Exception exception) { Duration timeout; boolean forceRefreshAddressCache; if (isNonRetryableException(exception)) { - logger.debug("Operation will NOT be retried. Current attempt {}, Exception: ", this.attemptCount, exception); return Mono.just(ShouldRetryResult.noRetryOnNonRelatedException()); @@ -290,9 +291,10 @@ private Pair, Boolean> handleInvalidPartitionException(I class RetryWithRetryPolicy implements IRetryPolicy { private final static int DEFAULT_WAIT_TIME_IN_SECONDS = 30; - private final static int MAXIMUM_BACKOFF_TIME_IN_MS = 15000; + private final static int MAXIMUM_BACKOFF_TIME_IN_MS = 1000; private final static int INITIAL_BACKOFF_TIME_MS = 10; private final static int BACK_OFF_MULTIPLIER = 2; + private final static int RANDOM_SALT_IN_MS = 5; private volatile int attemptCount = 1; private volatile int currentBackoffMilliseconds = RetryWithRetryPolicy.INITIAL_BACKOFF_TIME_MS; @@ -333,7 +335,7 @@ public Mono shouldRetry(Exception exception) { backoffTime = Duration.ofMillis( Math.min( - Math.min(this.currentBackoffMilliseconds, remainingMilliseconds), + Math.min(this.currentBackoffMilliseconds + random.nextInt(RANDOM_SALT_IN_MS), remainingMilliseconds), RetryWithRetryPolicy.MAXIMUM_BACKOFF_TIME_IN_MS)); this.currentBackoffMilliseconds *= RetryWithRetryPolicy.BACK_OFF_MULTIPLIER; logger.debug("BackoffTime: {} ms.", backoffTime.toMillis()); diff --git a/sdk/cosmos/azure-cosmos/src/test/java/com/azure/cosmos/implementation/directconnectivity/GoneAndRetryWithRetryPolicyTest.java b/sdk/cosmos/azure-cosmos/src/test/java/com/azure/cosmos/implementation/directconnectivity/GoneAndRetryWithRetryPolicyTest.java index 387d6951cbaa6..6773fa8e8396a 100644 --- a/sdk/cosmos/azure-cosmos/src/test/java/com/azure/cosmos/implementation/directconnectivity/GoneAndRetryWithRetryPolicyTest.java +++ b/sdk/cosmos/azure-cosmos/src/test/java/com/azure/cosmos/implementation/directconnectivity/GoneAndRetryWithRetryPolicyTest.java @@ -18,6 +18,7 @@ import com.azure.cosmos.implementation.RxDocumentServiceRequest; import com.azure.cosmos.implementation.ShouldRetryResult; import com.azure.cosmos.implementation.guava25.base.Supplier; +import com.azure.cosmos.implementation.RetryWithException; import org.mockito.Mockito; import org.testng.annotations.Test; import reactor.core.publisher.Mono; @@ -325,6 +326,49 @@ public void shouldRetryWithGenericException() { assertThat(shouldRetryResult.shouldRetry).isFalse(); } + /** + * Test for custom retryWith values + */ + @Test(groups = { "unit" }, timeOut = TIMEOUT) + public void retryWithDefaultTimeouts() { + int defaultInitialDelayInMs = 10; + int defaultSalt = 5; + RxDocumentServiceRequest request = RxDocumentServiceRequest.create( + mockDiagnosticsClientContext(), + OperationType.Create, + ResourceType.Document); + GoneAndRetryWithRetryPolicy goneAndRetryWithRetryPolicy = new GoneAndRetryWithRetryPolicy(request, 30); + + RetryWithException retryWithException = Mockito.mock(RetryWithException.class); + + Mono singleShouldRetry = goneAndRetryWithRetryPolicy.shouldRetry(retryWithException); + ShouldRetryResult shouldRetryResult = singleShouldRetry.block(); + assertThat(shouldRetryResult.policyArg.getValue3()).isEqualTo(1); + validateRetryWithTimeRange(defaultInitialDelayInMs, shouldRetryResult, defaultSalt); + + singleShouldRetry = goneAndRetryWithRetryPolicy.shouldRetry(retryWithException); + shouldRetryResult = singleShouldRetry.block(); + assertThat(shouldRetryResult.policyArg.getValue3()).isEqualTo(2); + defaultInitialDelayInMs = defaultInitialDelayInMs * 2; //backoff multiplier + validateRetryWithTimeRange(defaultInitialDelayInMs, shouldRetryResult, defaultSalt); + + singleShouldRetry = goneAndRetryWithRetryPolicy.shouldRetry(retryWithException); + shouldRetryResult = singleShouldRetry.block(); + assertThat(shouldRetryResult.policyArg.getValue3()).isEqualTo(3); + defaultInitialDelayInMs = defaultInitialDelayInMs * 2; //backoff multiplier + validateRetryWithTimeRange(defaultInitialDelayInMs, shouldRetryResult, defaultSalt); + } + + private static void validateRetryWithTimeRange( + int expectedDelayInMs, + ShouldRetryResult retryResult, + Integer saltValueInMs) { + assertThat(retryResult.shouldRetry).isTrue(); + assertThat(retryResult.backOffTime.toMillis() >= 0).isTrue(); + assertThat(retryResult.backOffTime.toMillis() > expectedDelayInMs - saltValueInMs).isTrue(); + assertThat(retryResult.backOffTime.toMillis() < expectedDelayInMs + saltValueInMs).isTrue(); + } + /** * After waitTimeInSeconds exhausted, retryWithException will not be retried. */