Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Updated retryWith values and added salting to match compute/.NET behavior #24619

Merged
merged 7 commits into from
Oct 8, 2021
Merged

Updated retryWith values and added salting to match compute/.NET behavior #24619

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
5271bdd
updated values and added salt
simorenoh Oct 7, 2021
4bcf079
added tests
simorenoh Oct 7, 2021
652f177
removed unused logic
simorenoh Oct 7, 2021
85f0086
addressed comments
simorenoh Oct 7, 2021
3124606
using threadlocalrandom
simorenoh Oct 7, 2021
581384e
removed unused import, made random private
simorenoh Oct 7, 2021
3840506
Merge branch 'main' into update-retryWith
simorenoh Oct 8, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 6 additions & 3 deletions .../java/com/azure/cosmos/implementation/directconnectivity/GoneAndRetryWithRetryPolicy.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,8 @@

import java.time.Duration;
import java.time.Instant;
import java.util.Random;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this unused import? if so remove

import java.util.concurrent.ThreadLocalRandom;

import static com.azure.cosmos.implementation.guava25.base.Preconditions.checkNotNull;

Expand All @@ -36,6 +38,7 @@ public class GoneAndRetryWithRetryPolicy implements IRetryPolicy {

private volatile RetryWithException lastRetryWithException;
private RetryContext retryContext;
static final ThreadLocalRandom random = ThreadLocalRandom.current();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

private


public GoneAndRetryWithRetryPolicy(RxDocumentServiceRequest request, Integer waitTimeInSeconds) {
this.retryContext = BridgeInternal.getRetryContext(request.requestContext.cosmosDiagnostics);
Expand Down Expand Up @@ -171,7 +174,6 @@ public Mono<ShouldRetryResult> 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());
Expand Down Expand Up @@ -291,9 +293,10 @@ private Pair<Mono<ShouldRetryResult>, 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;
Expand Down Expand Up @@ -334,7 +337,7 @@ public Mono<ShouldRetryResult> shouldRetry(Exception exception) {

backoffTime = Duration.ofMillis(
Math.min(
Math.min(this.currentBackoffMilliseconds, remainingMilliseconds),
Math.min(this.currentBackoffMilliseconds + random.nextInt(RANDOM_SALT_IN_MS), remainingMilliseconds),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it include both positive and negative salt value ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the functionality in the .NET SDK only adds those additional ms so it was mirrored here the same way:
image

RetryWithRetryPolicy.MAXIMUM_BACKOFF_TIME_IN_MS));
this.currentBackoffMilliseconds *= RetryWithRetryPolicy.BACK_OFF_MULTIPLIER;
logger.debug("BackoffTime: {} ms.", backoffTime.toMillis());
Expand Down
45 changes: 45 additions & 0 deletions ...a/com/azure/cosmos/implementation/directconnectivity/GoneAndRetryWithRetryPolicyTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,8 @@
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;

Expand Down Expand Up @@ -321,4 +323,47 @@ 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<ShouldRetryResult> 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();
}

}