Skip to content

Use "full jitter" & updated base delay for STANDARD retry mode defaults #2648

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.

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

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
6 changes: 6 additions & 0 deletions .changes/next-release/bugfix-AWSSDKforJavav2-84afdff.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
{
"category": "AWS SDK for Java v2",
"contributor": "",
"type": "bugfix",
"description": "Use \"full jitter\" & updated base delay for STANDARD retry mode defaults"
}
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,9 @@
@SdkInternalApi
public final class SdkDefaultRetrySetting {
public static final class Legacy {
private static final int MAX_ATTEMPTS = 4;
private static final Duration BASE_DELAY = Duration.ofMillis(100);
private static final Duration THROTTLED_BASE_DELAY = Duration.ofMillis(500);
private static final int THROTTLE_EXCEPTION_TOKEN_COST = 0;
private static final int DEFAULT_EXCEPTION_TOKEN_COST = 5;

Expand All @@ -44,6 +47,9 @@ public static final class Legacy {
}

public static final class Standard {
private static final int MAX_ATTEMPTS = 3;
private static final Duration BASE_DELAY = Duration.ofSeconds(1);
private static final Duration THROTTLED_BASE_DELAY = Duration.ofSeconds(1);
private static final int THROTTLE_EXCEPTION_TOKEN_COST = 5;
private static final int DEFAULT_EXCEPTION_TOKEN_COST = 5;

Expand All @@ -56,13 +62,7 @@ public static final class Standard {

public static final int TOKEN_BUCKET_SIZE = 500;

public static final Duration BASE_DELAY = Duration.ofMillis(100);

public static final Duration THROTTLED_BASE_DELAY = Duration.ofMillis(500);

public static final Duration MAX_BACKOFF = Duration.ofMillis(20_000);

public static final Integer DEFAULT_MAX_RETRIES = 3;
public static final Duration MAX_BACKOFF = Duration.ofSeconds(20);

public static final Set<Integer> RETRYABLE_STATUS_CODES;
public static final Set<Class<? extends Exception>> RETRYABLE_EXCEPTIONS;
Expand Down Expand Up @@ -91,13 +91,13 @@ public static Integer maxAttempts(RetryMode retryMode) {
if (maxAttempts == null) {
switch (retryMode) {
case LEGACY:
maxAttempts = 4;
maxAttempts = Legacy.MAX_ATTEMPTS;
break;
case STANDARD:
maxAttempts = 3;
maxAttempts = Standard.MAX_ATTEMPTS;
break;
default:
throw new IllegalArgumentException("Unknown retry mode: " + retryMode);
throw new IllegalStateException("Unsupported RetryMode: " + retryMode);
}
}

Expand All @@ -117,4 +117,26 @@ public static TokenBucketExceptionCostFunction tokenCostFunction(RetryMode retry
public static Integer defaultMaxAttempts() {
return maxAttempts(RetryMode.defaultRetryMode());
}

public static Duration baseDelay(RetryMode retryMode) {
switch (retryMode) {
case LEGACY:
return Legacy.BASE_DELAY;
case STANDARD:
return Standard.BASE_DELAY;
default:
throw new IllegalStateException("Unsupported RetryMode: " + retryMode);
}
}

public static Duration throttledBaseDelay(RetryMode retryMode) {
switch (retryMode) {
case LEGACY:
return Legacy.THROTTLED_BASE_DELAY;
case STANDARD:
return Standard.THROTTLED_BASE_DELAY;
default:
throw new IllegalStateException("Unsupported RetryMode: " + retryMode);
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -329,8 +329,8 @@ private BuilderImpl(RetryMode retryMode) {
this.retryMode = retryMode;
this.numRetries = SdkDefaultRetrySetting.maxAttempts(retryMode) - 1;
this.additionalRetryConditionsAllowed = true;
this.backoffStrategy = BackoffStrategy.defaultStrategy();
this.throttlingBackoffStrategy = BackoffStrategy.defaultThrottlingStrategy();
this.backoffStrategy = BackoffStrategy.defaultStrategy(retryMode);
this.throttlingBackoffStrategy = BackoffStrategy.defaultThrottlingStrategy(retryMode);
this.retryCondition = RetryCondition.defaultRetryCondition();
this.retryCapacityCondition = TokenBucketRetryCondition.forRetryMode(retryMode);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
import java.time.Duration;
import software.amazon.awssdk.annotations.SdkPublicApi;
import software.amazon.awssdk.core.internal.retry.SdkDefaultRetrySetting;
import software.amazon.awssdk.core.retry.RetryMode;
import software.amazon.awssdk.core.retry.RetryPolicyContext;

@SdkPublicApi
Expand All @@ -44,17 +45,35 @@ default int calculateExponentialDelay(int retriesAttempted, Duration baseDelay,
}

static BackoffStrategy defaultStrategy() {
return defaultStrategy(RetryMode.defaultRetryMode());
}

static BackoffStrategy defaultStrategy(RetryMode retryMode) {
return FullJitterBackoffStrategy.builder()
.baseDelay(SdkDefaultRetrySetting.BASE_DELAY)
.baseDelay(SdkDefaultRetrySetting.baseDelay(retryMode))
.maxBackoffTime(SdkDefaultRetrySetting.MAX_BACKOFF)
.build();
}

static BackoffStrategy defaultThrottlingStrategy() {
return EqualJitterBackoffStrategy.builder()
.baseDelay(SdkDefaultRetrySetting.THROTTLED_BASE_DELAY)
.maxBackoffTime(SdkDefaultRetrySetting.MAX_BACKOFF)
.build();
return defaultThrottlingStrategy(RetryMode.defaultRetryMode());
}

static BackoffStrategy defaultThrottlingStrategy(RetryMode retryMode) {
switch (retryMode) {
case LEGACY:
return EqualJitterBackoffStrategy.builder()
.baseDelay(SdkDefaultRetrySetting.throttledBaseDelay(retryMode))
.maxBackoffTime(SdkDefaultRetrySetting.MAX_BACKOFF)
.build();
case STANDARD:
return FullJitterBackoffStrategy.builder()
.baseDelay(SdkDefaultRetrySetting.throttledBaseDelay(retryMode))
.maxBackoffTime(SdkDefaultRetrySetting.MAX_BACKOFF)
.build();
default:
throw new IllegalStateException("Unsupported RetryMode: " + retryMode);
}
}

static BackoffStrategy none() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,8 @@ private FullJitterBackoffStrategy(BuilderImpl builder) {
@Override
public Duration computeDelayBeforeNextRetry(RetryPolicyContext context) {
int ceil = calculateExponentialDelay(context.retriesAttempted(), baseDelay, maxBackoffTime);
return Duration.ofMillis(random.nextInt(ceil));
// Minimum of 1 ms (consistent with BackoffStrategy.none()'s behavior)
return Duration.ofMillis(random.nextInt(ceil) + 1L);
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,12 +19,15 @@
import static org.assertj.core.api.Assertions.assertThatThrownBy;
import static org.mockito.Mockito.verify;

import java.time.Duration;
import org.junit.Assert;
import org.junit.Test;
import org.junit.runner.RunWith;
import org.mockito.Mock;
import org.mockito.runners.MockitoJUnitRunner;
import software.amazon.awssdk.core.retry.backoff.BackoffStrategy;
import software.amazon.awssdk.core.retry.backoff.EqualJitterBackoffStrategy;
import software.amazon.awssdk.core.retry.backoff.FullJitterBackoffStrategy;
import software.amazon.awssdk.core.retry.conditions.RetryCondition;

@RunWith(MockitoJUnitRunner.class)
Expand Down Expand Up @@ -117,4 +120,36 @@ public void maxRetriesFromDefaultRetryModeIsCorrect() {
Assert.fail();
}
}

@Test
public void legacyRetryMode_shouldUseFullJitterAndEqualJitter() {
RetryPolicy legacyRetryPolicy = RetryPolicy.forRetryMode(RetryMode.LEGACY);

assertThat(legacyRetryPolicy.backoffStrategy()).isInstanceOf(FullJitterBackoffStrategy.class);
FullJitterBackoffStrategy backoffStrategy = (FullJitterBackoffStrategy) legacyRetryPolicy.backoffStrategy();
assertThat(backoffStrategy.toBuilder().baseDelay()).isEqualTo(Duration.ofMillis(100));
assertThat(backoffStrategy.toBuilder().maxBackoffTime()).isEqualTo(Duration.ofSeconds(20));

assertThat(legacyRetryPolicy.throttlingBackoffStrategy()).isInstanceOf(EqualJitterBackoffStrategy.class);
EqualJitterBackoffStrategy throttlingBackoffStrategy =
(EqualJitterBackoffStrategy) legacyRetryPolicy.throttlingBackoffStrategy();
assertThat(throttlingBackoffStrategy.toBuilder().baseDelay()).isEqualTo(Duration.ofMillis(500));
assertThat(throttlingBackoffStrategy.toBuilder().maxBackoffTime()).isEqualTo(Duration.ofSeconds(20));
}

@Test
public void standardRetryMode_shouldUseFullJitterOnly() {
RetryPolicy standardRetryPolicy = RetryPolicy.forRetryMode(RetryMode.STANDARD);

assertThat(standardRetryPolicy.backoffStrategy()).isInstanceOf(FullJitterBackoffStrategy.class);
FullJitterBackoffStrategy backoffStrategy = (FullJitterBackoffStrategy) standardRetryPolicy.backoffStrategy();
assertThat(backoffStrategy.toBuilder().baseDelay()).isEqualTo(Duration.ofSeconds(1));
assertThat(backoffStrategy.toBuilder().maxBackoffTime()).isEqualTo(Duration.ofSeconds(20));

assertThat(standardRetryPolicy.throttlingBackoffStrategy()).isInstanceOf(FullJitterBackoffStrategy.class);
FullJitterBackoffStrategy throttlingBackoffStrategy =
(FullJitterBackoffStrategy) standardRetryPolicy.throttlingBackoffStrategy();
assertThat(throttlingBackoffStrategy.toBuilder().baseDelay()).isEqualTo(Duration.ofSeconds(1));
assertThat(throttlingBackoffStrategy.toBuilder().maxBackoffTime()).isEqualTo(Duration.ofSeconds(20));
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,172 @@
/*
* Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved.
*
* Licensed under the Apache License, Version 2.0 (the "License").
* You may not use this file except in compliance with the License.
* A copy of the License is located at
*
* http://aws.amazon.com/apache2.0
*
* or in the "license" file accompanying this file. This file is distributed
* on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either
* express or implied. See the License for the specific language governing
* permissions and limitations under the License.
*/

package software.amazon.awssdk.core.retry.backoff;
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing copyright header


import static org.assertj.core.api.Assertions.assertThat;
import static org.mockito.AdditionalAnswers.returnsFirstArg;
import static org.mockito.Matchers.anyInt;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.when;

import java.time.Duration;
import java.util.Arrays;
import java.util.Collection;
import java.util.Random;
import org.junit.Before;
import org.junit.Test;
import org.junit.runner.RunWith;
import org.junit.runners.Parameterized;
import org.junit.runners.Parameterized.Parameter;
import org.junit.runners.Parameterized.Parameters;
import org.mockito.Mock;
import org.mockito.stubbing.Answer;
import software.amazon.awssdk.core.retry.RetryMode;
import software.amazon.awssdk.core.retry.RetryPolicyContext;

@RunWith(Parameterized.class)
public class FullJitterBackoffStrategyTest {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice tests!


@Parameters
public static Collection<TestCase> parameters() throws Exception {
return Arrays.asList(
new TestCase().backoffStrategy(BackoffStrategy.defaultStrategy(RetryMode.STANDARD))
.retriesAttempted(0)
.expectedMaxDelay(Duration.ofSeconds(1))
.expectedMedDelay(Duration.ofMillis(500))
.expectedMinDelay(Duration.ofMillis(1)),
new TestCase().backoffStrategy(BackoffStrategy.defaultStrategy(RetryMode.STANDARD))
.retriesAttempted(1)
.expectedMaxDelay(Duration.ofSeconds(2))
.expectedMedDelay(Duration.ofSeconds(1))
.expectedMinDelay(Duration.ofMillis(1)),
new TestCase().backoffStrategy(BackoffStrategy.defaultStrategy(RetryMode.STANDARD))
.retriesAttempted(2)
.expectedMaxDelay(Duration.ofSeconds(4))
.expectedMedDelay(Duration.ofSeconds(2))
.expectedMinDelay(Duration.ofMillis(1)),
new TestCase().backoffStrategy(BackoffStrategy.defaultStrategy(RetryMode.STANDARD))
.retriesAttempted(3)
.expectedMaxDelay(Duration.ofSeconds(8))
.expectedMedDelay(Duration.ofSeconds(4))
.expectedMinDelay(Duration.ofMillis(1)),
new TestCase().backoffStrategy(BackoffStrategy.defaultStrategy(RetryMode.STANDARD))
.retriesAttempted(4)
.expectedMaxDelay(Duration.ofSeconds(16))
.expectedMedDelay(Duration.ofSeconds(8))
.expectedMinDelay(Duration.ofMillis(1)),
new TestCase().backoffStrategy(BackoffStrategy.defaultStrategy(RetryMode.STANDARD))
.retriesAttempted(5)
.expectedMaxDelay(Duration.ofSeconds(20))
.expectedMedDelay(Duration.ofSeconds(10))
.expectedMinDelay(Duration.ofMillis(1))
);
}

@Parameter
public TestCase testCase;

@Mock
private Random mockRandom = mock(Random.class);

@Before
public void setUp() throws Exception {
testCase.backoffStrategy = injectMockRandom(testCase.backoffStrategy);
}

@Test
public void testMaxDelay() {
mockMaxRandom();
test(testCase.backoffStrategy, testCase.retriesAttempted, testCase.expectedMaxDelay);
}

@Test
public void testMedDelay() {
mockMediumRandom();
test(testCase.backoffStrategy, testCase.retriesAttempted, testCase.expectedMedDelay);
}

@Test
public void testMinDelay() {
mockMinRandom();
test(testCase.backoffStrategy, testCase.retriesAttempted, testCase.expectedMinDelay);
}

private static void test(BackoffStrategy backoffStrategy, int retriesAttempted, Duration expectedDelay) {
RetryPolicyContext context = RetryPolicyContext.builder()
.retriesAttempted(retriesAttempted)
.build();
Duration computedDelay = backoffStrategy.computeDelayBeforeNextRetry(context);
assertThat(computedDelay).isEqualTo(expectedDelay);
}

private FullJitterBackoffStrategy injectMockRandom(BackoffStrategy strategy) {
FullJitterBackoffStrategy.Builder builder = ((FullJitterBackoffStrategy) strategy).toBuilder();
return new FullJitterBackoffStrategy(builder.baseDelay(), builder.maxBackoffTime(), mockRandom);
}

private void mockMaxRandom() {
when(mockRandom.nextInt(anyInt())).then((Answer<Integer>) invocationOnMock -> {
Integer firstArg = (Integer) returnsFirstArg().answer(invocationOnMock);
return firstArg - 1;
});
}

private void mockMinRandom() {
when(mockRandom.nextInt(anyInt())).then((Answer<Integer>) invocationOnMock -> {
return 0;
});
}

private void mockMediumRandom() {
when(mockRandom.nextInt(anyInt())).then((Answer<Integer>) invocationOnMock -> {
Integer firstArg = (Integer) returnsFirstArg().answer(invocationOnMock);
return firstArg / 2 - 1;
});
}

private static class TestCase {
private BackoffStrategy backoffStrategy;
private int retriesAttempted;
private Duration expectedMinDelay;
private Duration expectedMedDelay;
private Duration expectedMaxDelay;

public TestCase backoffStrategy(BackoffStrategy backoffStrategy) {
this.backoffStrategy = backoffStrategy;
return this;
}

public TestCase retriesAttempted(int retriesAttempted) {
this.retriesAttempted = retriesAttempted;
return this;
}

public TestCase expectedMinDelay(Duration expectedMinDelay) {
this.expectedMinDelay = expectedMinDelay;
return this;
}

public TestCase expectedMedDelay(Duration expectedMedDelay) {
this.expectedMedDelay = expectedMedDelay;
return this;
}

public TestCase expectedMaxDelay(Duration expectedMaxDelay) {
this.expectedMaxDelay = expectedMaxDelay;
return this;
}
}
}