Skip to content

Change uses of RetryPolicy to RetryStrategy #4125

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

Merged
merged 1 commit into from
Jun 23, 2023
Merged

Change uses of RetryPolicy to RetryStrategy #4125

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
13 changes: 13 additions & 0 deletions ...n/java/software/amazon/awssdk/codegen/model/config/customization/CustomizationConfig.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -130,6 +130,11 @@ public class CustomizationConfig {
*/
private String customRetryPolicy;

/**
* Custom Retry strategy
*/
private String customRetryStrategy;

private boolean skipSyncClientGeneration;

/**
Expand Down Expand Up @@ -416,10 +421,18 @@ public String getCustomRetryPolicy() {
return customRetryPolicy;
}

public String getCustomRetryStrategy() {
return customRetryStrategy;
}

public void setCustomRetryPolicy(String customRetryPolicy) {
this.customRetryPolicy = customRetryPolicy;
}

public void setCustomRetryStrategy(String customRetryStrategy) {
this.customRetryStrategy = customRetryStrategy;
}

public boolean isSkipSyncClientGeneration() {
return skipSyncClientGeneration;
}
Expand Down
4 changes: 4 additions & 0 deletions ...en/src/main/java/software/amazon/awssdk/codegen/model/intermediate/IntermediateModel.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -205,6 +205,10 @@ public String getCustomRetryPolicy() {
return customizationConfig.getCustomRetryPolicy();
}

public String getCustomRetryStrategy() {
return customizationConfig.getCustomRetryStrategy();
}

public String getSdkModeledExceptionBaseFqcn() {
return String.format("%s.%s",
metadata.getFullModelPackageName(),
Expand Down
6 changes: 6 additions & 0 deletions ...gen/src/main/java/software/amazon/awssdk/codegen/poet/builder/BaseClientBuilderClass.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -342,6 +342,12 @@ private MethodSpec finalizeServiceConfigurationMethod() {
PoetUtils.classNameFromFqcn(model.getCustomizationConfig().getCustomRetryPolicy()));
}

if (StringUtils.isNotBlank(model.getCustomizationConfig().getCustomRetryStrategy())) {
builder.addCode(".option($1T.RETRY_STRATEGY, $2T.resolveRetryStrategy(config))",
SdkClientOption.class,
PoetUtils.classNameFromFqcn(model.getCustomizationConfig().getCustomRetryStrategy()));
}

if (StringUtils.isNotBlank(clientConfigClassName)) {
builder.addCode(".option($T.SERVICE_CONFIGURATION, finalServiceConfig)", SdkClientOption.class);
}
Expand Down
2 changes: 2 additions & 0 deletions ...test/resources/software/amazon/awssdk/codegen/poet/builder/test-client-builder-class.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
import java.util.List;
import software.amazon.MyServiceHttpConfig;
import software.amazon.MyServiceRetryPolicy;
import software.amazon.MyServiceRetryStrategy;
import software.amazon.awssdk.annotations.Generated;
import software.amazon.awssdk.annotations.SdkInternalApi;
import software.amazon.awssdk.auth.signer.Aws4Signer;
Expand Down Expand Up @@ -126,6 +127,7 @@ protected final SdkClientConfiguration finalizeServiceConfiguration(SdkClientCon
.option(AwsClientOption.FIPS_ENDPOINT_ENABLED, finalServiceConfig.fipsModeEnabled())
.option(SdkClientOption.EXECUTION_INTERCEPTORS, interceptors)
.option(SdkClientOption.RETRY_POLICY, MyServiceRetryPolicy.resolveRetryPolicy(config))
.option(SdkClientOption.RETRY_STRATEGY, MyServiceRetryStrategy.resolveRetryStrategy(config))
.option(SdkClientOption.SERVICE_CONFIGURATION, finalServiceConfig).build();
}

Expand Down
1 change: 1 addition & 0 deletions ...c/test/resources/software/amazon/awssdk/codegen/poet/client/c2j/json/customization.config
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
"hasFipsProperty": true
},
"customRetryPolicy": "software.amazon.MyServiceRetryPolicy",
"customRetryStrategy": "software.amazon.MyServiceRetryStrategy",
"verifiedSimpleMethods" : ["paginatedOperationWithResultKey"],
"blacklistedSimpleMethods" : [
"eventStreamOperation"
Expand Down
1 change: 1 addition & 0 deletions ...t/resources/software/amazon/awssdk/codegen/poet/client/c2j/rest-json/customization.config
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
"hasAccelerateModeEnabledProperty":true
},
"customRetryPolicy": "software.amazon.MyServiceRetryPolicy",
"customRetryStrategy": "software.amazon.MyServiceRetryStrategy",
"verifiedSimpleMethods" : ["paginatedOperationWithResultKey"],
"blacklistedSimpleMethods" : [
"eventStreamOperation"
Expand Down
20 changes: 0 additions & 20 deletions .../src/main/java/software/amazon/awssdk/awscore/client/builder/AwsDefaultClientBuilder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -372,28 +372,8 @@ private RetryPolicy resolveAwsRetryPolicy(SdkClientConfiguration config) {
}
}

RetryMode retryMode = RetryMode.resolver()
.profileFile(config.option(SdkClientOption.PROFILE_FILE_SUPPLIER))
.profileName(config.option(SdkClientOption.PROFILE_NAME))
.defaultRetryMode(config.option(SdkClientOption.DEFAULT_RETRY_MODE))
.resolve();
return AwsRetryPolicy.forRetryMode(retryMode);
// TODO: fixme This will be changed like this to pick the configured retry strategy
// if no retry policy is configured.
/*
RetryPolicy policy = config.option(SdkClientOption.RETRY_POLICY);

if (policy != null) {
if (policy.additionalRetryConditionsAllowed()) {
return AwsRetryPolicy.addRetryConditions(policy);
} else {
return policy;
}
}

// If we don't have a configured retry policy we will use the configured retry strategy instead.
return null;
*/
}

private RetryStrategy<?, ?> resolveAwsRetryStrategy(SdkClientConfiguration config) {
Expand Down
8 changes: 5 additions & 3 deletions ...ws-core/src/test/java/software/amazon/awssdk/awscore/client/builder/DefaultsModeTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
import static software.amazon.awssdk.awscore.client.config.AwsClientOption.DEFAULTS_MODE;
import static software.amazon.awssdk.core.client.config.SdkClientOption.DEFAULT_RETRY_MODE;
import static software.amazon.awssdk.core.client.config.SdkClientOption.RETRY_POLICY;
import static software.amazon.awssdk.core.client.config.SdkClientOption.RETRY_STRATEGY;
import static software.amazon.awssdk.regions.ServiceMetadataAdvancedOption.DEFAULT_S3_US_EAST_1_REGIONAL_ENDPOINT;

import java.time.Duration;
Expand All @@ -36,6 +37,7 @@
import software.amazon.awssdk.awscore.internal.defaultsmode.DefaultsModeConfiguration;
import software.amazon.awssdk.core.client.config.ClientOverrideConfiguration;
import software.amazon.awssdk.core.client.config.SdkClientConfiguration;
import software.amazon.awssdk.core.internal.retry.SdkDefaultRetryStrategy;
import software.amazon.awssdk.core.retry.RetryMode;
import software.amazon.awssdk.http.SdkHttpClient;
import software.amazon.awssdk.http.SdkHttpConfigurationOption;
Expand Down Expand Up @@ -75,7 +77,7 @@ public void defaultClient_shouldUseLegacyModeWithExistingDefaults() {
.build();

assertThat(client.clientConfiguration.option(DEFAULTS_MODE)).isEqualTo(DefaultsMode.LEGACY);
assertThat(client.clientConfiguration.option(RETRY_POLICY).retryMode()).isEqualTo(RetryMode.defaultRetryMode());
assertThat(SdkDefaultRetryStrategy.retryMode(client.clientConfiguration.option(RETRY_STRATEGY))).isEqualTo(RetryMode.defaultRetryMode());
assertThat(client.clientConfiguration.option(DEFAULT_S3_US_EAST_1_REGIONAL_ENDPOINT)).isNull();
}

Expand All @@ -97,7 +99,7 @@ public void nonLegacyDefaultsMode_shouldApplySdkDefaultsAndHttpDefaults() {

AttributeMap attributes = DefaultsModeConfiguration.defaultConfig(targetMode);

assertThat(client.clientConfiguration.option(RETRY_POLICY).retryMode()).isEqualTo(attributes.get(DEFAULT_RETRY_MODE));
assertThat(SdkDefaultRetryStrategy.retryMode(client.clientConfiguration.option(RETRY_STRATEGY))).isEqualTo(attributes.get(DEFAULT_RETRY_MODE));
assertThat(client.clientConfiguration.option(DEFAULT_S3_US_EAST_1_REGIONAL_ENDPOINT)).isEqualTo("regional");
}

Expand All @@ -119,7 +121,7 @@ public void nonLegacyDefaultsModeAsyncClient_shouldApplySdkDefaultsAndHttpDefaul

AttributeMap attributes = DefaultsModeConfiguration.defaultConfig(targetMode);

assertThat(client.clientConfiguration.option(RETRY_POLICY).retryMode()).isEqualTo(attributes.get(DEFAULT_RETRY_MODE));
assertThat(SdkDefaultRetryStrategy.retryMode(client.clientConfiguration.option(RETRY_STRATEGY))).isEqualTo(attributes.get(DEFAULT_RETRY_MODE));
}

@Test
Expand Down
7 changes: 7 additions & 0 deletions core/retries-api/src/main/java/software/amazon/awssdk/retries/api/RetryStrategy.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,13 @@ public interface RetryStrategy<
*/
RecordSuccessResponse recordSuccess(RecordSuccessRequest request);

/**
* Returns the maximum numbers attempts that this retry policy will allow.
*
* @return the maximum numbers attempts that this retry policy will allow.
*/
int maxAttempts();

/**
* Create a new {@link Builder} with the current configuration.
*
Expand Down
6 changes: 6 additions & 0 deletions ...etries-api/src/test/java/software/amazon/awssdk/retries/api/RetryStrategyBuilderTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -176,10 +176,16 @@ public RecordSuccessResponse recordSuccess(RecordSuccessRequest request) {
return null;
}

@Override
public int maxAttempts() {
return 0;
}

@Override
public BuilderToTestDefaults toBuilder() {
return null;
}

}

}
5 changes: 5 additions & 0 deletions core/retries/src/main/java/software/amazon/awssdk/retries/internal/BaseRetryStrategy.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -138,6 +138,11 @@ public final RecordSuccessResponse recordSuccess(RecordSuccessRequest request) {
return RecordSuccessResponse.create(refreshedToken);
}

@Override
public int maxAttempts() {
return maxAttempts;
}

@Override
public abstract B toBuilder();

Expand Down
15 changes: 0 additions & 15 deletions ...ore/src/main/java/software/amazon/awssdk/core/client/builder/SdkDefaultClientBuilder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -358,22 +358,7 @@ private String resolveClientUserAgent(SdkClientConfiguration config, String retr
}

private RetryPolicy resolveRetryPolicy(SdkClientConfiguration config) {
RetryPolicy policy = config.option(SdkClientOption.RETRY_POLICY);
if (policy != null) {
return policy;
}

RetryMode retryMode = RetryMode.resolver()
.profileFile(config.option(SdkClientOption.PROFILE_FILE_SUPPLIER))
.profileName(config.option(SdkClientOption.PROFILE_NAME))
.defaultRetryMode(config.option(SdkClientOption.DEFAULT_RETRY_MODE))
.resolve();
return RetryPolicy.forRetryMode(retryMode);
// TODO: fixme This will be changed like this to pick the configured retry strategy
// if no retry policy is configured.
/*
return config.option(SdkClientOption.RETRY_POLICY);
*/
}

private RetryStrategy<?, ?> resolveRetryStrategy(SdkClientConfiguration config) {
Expand Down
2 changes: 0 additions & 2 deletions ...re/src/main/java/software/amazon/awssdk/core/client/config/SdkClientOptionValidation.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,8 +46,6 @@ private static void validateClientOptions(SdkClientConfiguration c) {

require("overrideConfiguration.additionalHttpHeaders", c.option(SdkClientOption.ADDITIONAL_HTTP_HEADERS));
require("overrideConfiguration.executionInterceptors", c.option(SdkClientOption.EXECUTION_INTERCEPTORS));
// TODO: fixme, this will be removed as retryPolicy will be optional
require("overrideConfiguration.retryPolicy", c.option(SdkClientOption.RETRY_POLICY));
require("overrideConfiguration.retryStrategy", c.option(SdkClientOption.RETRY_STRATEGY));

require("overrideConfiguration.advancedOption[USER_AGENT_PREFIX]",
Expand Down
61 changes: 58 additions & 3 deletions .../main/java/software/amazon/awssdk/core/internal/http/pipeline/stages/RetryableStage2.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,12 +28,14 @@
import software.amazon.awssdk.core.internal.http.pipeline.RequestToResponsePipeline;
import software.amazon.awssdk.core.internal.http.pipeline.stages.utils.RetryableStageHelper2;
import software.amazon.awssdk.http.SdkHttpFullRequest;
import software.amazon.awssdk.http.SdkHttpFullResponse;

/**
* Wrapper around the pipeline for a single request to provide retry, clock-skew and request throttling functionality.
*/
@SdkInternalApi
public final class RetryableStage2<OutputT> implements RequestToResponsePipeline<OutputT> {
private static final String RETRY_AFTER_HEADER = "Retry-After";
private final RequestPipeline<SdkHttpFullRequest, Response<OutputT>> requestPipeline;
private final HttpClientDependencies dependencies;

Expand All @@ -54,8 +56,13 @@ public Response<OutputT> execute(SdkHttpFullRequest request, RequestExecutionCon
Response<OutputT> response = executeRequest(retryableStageHelper, context);
retryableStageHelper.recordAttemptSucceeded();
return response;
} catch (SdkException | IOException e) {
retryableStageHelper.setLastException(e);
} catch (SdkExceptionWithRetryAfterHint | SdkException | IOException e) {
Throwable throwable = e;
if (e instanceof SdkExceptionWithRetryAfterHint) {
SdkExceptionWithRetryAfterHint wrapper = (SdkExceptionWithRetryAfterHint) e;
throwable = wrapper.cause();
}
retryableStageHelper.setLastException(throwable);
Duration suggestedDelay = suggestedDelay(e);
Optional<Duration> backoffDelay = retryableStageHelper.tryRefreshToken(suggestedDelay);
if (backoffDelay.isPresent()) {
Expand All @@ -70,6 +77,10 @@ public Response<OutputT> execute(SdkHttpFullRequest request, RequestExecutionCon
}

private Duration suggestedDelay(Exception e) {
if (e instanceof SdkExceptionWithRetryAfterHint) {
SdkExceptionWithRetryAfterHint except = (SdkExceptionWithRetryAfterHint) e;
return Duration.ofSeconds(except.retryAfter());
}
return Duration.ZERO;
}

Expand All @@ -83,8 +94,52 @@ private Response<OutputT> executeRequest(RetryableStageHelper2 retryableStageHel
retryableStageHelper.setLastResponse(response.httpResponse());
if (!response.isSuccess()) {
retryableStageHelper.adjustClockIfClockSkew(response);
throw response.exception();
throw responseException(response);
}
return response;
}

private RuntimeException responseException(Response<OutputT> response) {
Optional<Integer> optionalRetryAfter = retryAfter(response.httpResponse());
if (optionalRetryAfter.isPresent()) {
return new SdkExceptionWithRetryAfterHint(optionalRetryAfter.get(), response.exception());
}
return response.exception();
}

private Optional<Integer> retryAfter(SdkHttpFullResponse response) {
Optional<String> optionalRetryAfterHeader = response.firstMatchingHeader(RETRY_AFTER_HEADER);
if (optionalRetryAfterHeader.isPresent()) {
String retryAfterHeader = optionalRetryAfterHeader.get();
try {
return Optional.of(Integer.parseInt(retryAfterHeader));
} catch (NumberFormatException e) {
// Ignore and fallback to returning empty.
}
}
return Optional.empty();
}

// This probably should go directly into SdkException
Copy link
Contributor

Choose a reason for hiding this comment

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

How do you propose to refactor?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd think this can be part of SdkServiceException with a new method that returns Optional<Instant> retryAfterHint() which defaults to Optional.empty(), and which can be added to the methods that we already use for retrying, e.g.,

    public boolean isClockSkewException() { … }
    public boolean isThrottlingException() { … }
    public Optional<Instant> retryAfterHint() { … }

Error handlers per protocol can take care of filling this up.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see. Agreed to handle this in offline design forum.

static class SdkExceptionWithRetryAfterHint extends RuntimeException {
private final SdkException cause;
private final int seconds;

SdkExceptionWithRetryAfterHint(int seconds, SdkException cause) {
this.seconds = seconds;
this.cause = cause;
}

public int retryAfter() {
return seconds;
}

public SdkException cause() {
return cause;
}

public int seconds() {
return seconds;
}
}
}
5 changes: 2 additions & 3 deletions ...oftware/amazon/awssdk/core/internal/http/pipeline/stages/utils/RetryableStageHelper2.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -173,10 +173,9 @@ public void logBackingOff(Duration backoffDelay) {
* Retrieve the request to send to the service, including any detailed retry information headers.
*/
public SdkHttpFullRequest requestToSend() {
// TODO: fixme, we don't longer have this information handy we need to change the interface to access it.
int maxAllowedRetries = 3;
return request.toBuilder()
.putHeader(SDK_RETRY_INFO_HEADER, "attempt=" + attemptNumber + "; max=" + maxAllowedRetries)
.putHeader(SDK_RETRY_INFO_HEADER, "attempt=" + attemptNumber
+ "; max=" + retryStrategy().maxAttempts())
.build();
}

Expand Down
5 changes: 5 additions & 0 deletions ...sdk-core/src/main/java/software/amazon/awssdk/core/internal/retry/RetryPolicyAdapter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,11 @@ public RecordSuccessResponse recordSuccess(RecordSuccessRequest request) {
return RecordSuccessResponse.create(token);
}

@Override
public int maxAttempts() {
return retryPolicy.numRetries() + 1;
}

@Override
public Builder toBuilder() {
return new Builder(this);
Expand Down
Loading