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.

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

External names used for retry modes only support 'adaptive' #5265

Merged
merged 5 commits into from
Jun 5, 2024
Merged
Show file tree
Hide file tree
Changes from 3 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
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,7 @@ public static RetryStrategy addRetryConditions(RetryStrategy strategy) {
}

/**
* Returns a retry strategy that do not retry.
* Returns a retry strategy that does not retry.
*
* @return A retry strategy that do not retry.
*/
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ private DefaultRetryStrategy() {
}

/**
* Returns a retry strategy that do not retry.
* Returns a retry strategy that does not retry.
*/
public static StandardRetryStrategy doNotRetry() {
return standardStrategyBuilder()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -197,8 +197,6 @@ private static Optional<RetryMode> fromString(String string) {
case "standard":
return Optional.of(STANDARD);
case "adaptive":
return Optional.of(ADAPTIVE);
case "adaptive_v2":
return Optional.of(ADAPTIVE_V2);
default:
throw new IllegalStateException("Unsupported retry policy mode configured: " + string);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -50,10 +50,10 @@ public static Collection<Object> data() {
// Test resolution
new TestData("legacy", null, null, null, RetryMode.LEGACY),
new TestData("standard", null, null, null, RetryMode.STANDARD),
new TestData("adaptive", null, null, null, RetryMode.ADAPTIVE),
new TestData("adaptive", null, null, null, RetryMode.ADAPTIVE_V2),
new TestData("lEgAcY", null, null, null, RetryMode.LEGACY),
new TestData("sTanDaRd", null, null, null, RetryMode.STANDARD),
new TestData("aDaPtIvE", null, null, null, RetryMode.ADAPTIVE),
new TestData("aDaPtIvE", null, null, null, RetryMode.ADAPTIVE_V2),

// Test precedence
new TestData("standard", "legacy", "PropertySetToLegacy", RetryMode.LEGACY, RetryMode.STANDARD),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,7 @@ void resolve_retryModeSetWithSupplier_resolvesFromSupplier() {
RetryStrategy retryStrategy = DynamoDbRetryPolicy.resolveRetryStrategy(sdkClientConfiguration);
RetryMode retryMode = SdkDefaultRetryStrategy.retryMode(retryStrategy);

assertThat(retryMode).isEqualTo(RetryMode.ADAPTIVE);
assertThat(retryMode).isEqualTo(RetryMode.ADAPTIVE_V2);
}

@Test
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +94,7 @@ private void setupRetryPolicy(BuilderT builder, RetryScenario scenario) {
RetryModeSetup setup = scenario.setup();
switch (setup) {
case PROFILE_USING_MODE:
setupProfile(builder, scenario.mode());
setupProfile(builder, scenario);
break;
case CLIENT_OVERRIDE_USING_MODE:
builder.overrideConfiguration(o -> o.retryPolicy(mode));
Expand All @@ -115,7 +115,7 @@ private void setupRetryStrategy(BuilderT builder, RetryScenario scenario) {
// client.
switch (scenario.setup()) {
case PROFILE_USING_MODE:
setupProfile(builder, scenario.mode());
setupProfile(builder, scenario);
break;
case CLIENT_OVERRIDE_USING_MODE:
builder.overrideConfiguration(o -> o.retryStrategy(mode));
Expand All @@ -130,8 +130,8 @@ private void setupRetryStrategy(BuilderT builder, RetryScenario scenario) {
}
}

private void setupProfile(BuilderT builder, RetryMode mode) {
String modeName = mode.toString().toLowerCase(Locale.ROOT);
private void setupProfile(BuilderT builder, RetryScenario scenario) {
String modeName = scenario.modeExternalName();
ProfileFile profileFile = ProfileFile.builder()
.content(new StringInputStream("[profile retry_test]\n" +
"retry_mode = " + modeName))
Expand Down Expand Up @@ -165,7 +165,7 @@ private BuilderT clientBuilder() {

private void setupScenarioBefore(RetryScenario scenario) {
if (scenario.setup() == RetryModeSetup.SYSTEM_PROPERTY_USING_MODE) {
System.setProperty("aws.retryMode", scenario.mode().name().toLowerCase(Locale.ROOT));
System.setProperty("aws.retryMode", scenario.modeExternalName());
}
}

Expand Down Expand Up @@ -236,6 +236,16 @@ private static boolean isSupportedScenario(RetryScenario scenario) {
return false;
}

// System property or profile do not support the internal "adaptive_v2" name, only adaptive,
// and it's mapped to adaptive_v2. We mark here adaptive using profile or system property
// and map in te tests "adaptive_v2" to "adaptive" such that everything comes together at
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: map in the tests

sugmanue marked this conversation as resolved.
Show resolved Hide resolved
// the end.
if (scenario.mode() == RetryMode.ADAPTIVE
&& (scenario.setup() == RetryModeSetup.PROFILE_USING_MODE
|| scenario.setup() == RetryModeSetup.SYSTEM_PROPERTY_USING_MODE)) {
return false;
}

// Retry policies only support the legacy ADAPTIVE mode.
if (scenario.retryImplementation() == RetryImplementation.POLICY
&& scenario.mode() == RetryMode.ADAPTIVE_V2) {
Expand Down Expand Up @@ -323,6 +333,25 @@ public Builder toBuilder() {
return new Builder(this);
}

/**
* Returns the name used externally of the given mode. This name is used in the profile `retry_mode` setting or in the
* system property. Externally, "adaptive" gets mapped to RetryMode.ADAPTIVE_V2, and "adaptive_v2" an internal name
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: "adaptive_v2" is an internal name

sugmanue marked this conversation as resolved.
Show resolved Hide resolved
* only and not supported externally.
*/
public String modeExternalName() {
switch (mode) {
case ADAPTIVE:
case ADAPTIVE_V2:
return "adaptive";
case LEGACY:
return "legacy";
case STANDARD:
return "standard";
default:
throw new RuntimeException("Unsupported mode: " + mode);
}
}

@Override
public String toString() {
return mode + " " + retryImplementation + " " + setup;
Expand Down
Loading