Skip to content

Deprecate legacy classes and use new when possible #4154

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
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
Original file line number Diff line number Diff line change
Expand Up @@ -25,8 +25,11 @@

/**
* Retry Policy used by clients when communicating with AWS services.
*
* @deprecated Use instead {@link AwsRetryStrategy}
*/
@SdkPublicApi
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we remove this @SdkPublicApi since its deprecated i think we can remove this

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'm not sure that we can remove it, if I do the build will bark with:

SDK annotation is missing on this class. eg: @SdkProtectedApi [MissingSdkAnnotation]

If we do, which one should we use instead?

Copy link
Contributor

Choose a reason for hiding this comment

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

Deprecated shouldn't change the public/protected/internalness
Confirmed this with the team.
Thanks for the patience while I was waiting to get clarification on this.

@Deprecated
public final class AwsRetryPolicy {

private AwsRetryPolicy() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,13 +21,13 @@
import java.util.concurrent.Executors;
import software.amazon.awssdk.auth.credentials.DefaultCredentialsProvider;
import software.amazon.awssdk.awscore.client.config.AwsClientOption;
import software.amazon.awssdk.awscore.retry.AwsRetryStrategy;
import software.amazon.awssdk.core.client.config.SdkAdvancedAsyncClientOption;
import software.amazon.awssdk.core.client.config.SdkAdvancedClientOption;
import software.amazon.awssdk.core.client.config.SdkClientConfiguration;
import software.amazon.awssdk.core.client.config.SdkClientOption;
import software.amazon.awssdk.core.internal.http.AmazonSyncHttpClient;
import software.amazon.awssdk.core.internal.http.loader.DefaultSdkHttpClientBuilder;
import software.amazon.awssdk.core.retry.RetryPolicy;
import software.amazon.awssdk.core.signer.NoOpSigner;
import software.amazon.awssdk.http.SdkHttpClient;
import software.amazon.awssdk.http.SdkHttpConfigurationOption;
Expand All @@ -51,7 +51,7 @@ public static SdkClientConfiguration testClientConfiguration() {
return SdkClientConfiguration.builder()
.option(SdkClientOption.EXECUTION_INTERCEPTORS, new ArrayList<>())
.option(SdkClientOption.ENDPOINT, URI.create("http://localhost:8080"))
.option(SdkClientOption.RETRY_POLICY, RetryPolicy.defaultRetryPolicy())
.option(SdkClientOption.RETRY_STRATEGY, AwsRetryStrategy.defaultRetryStrategy())
.option(SdkClientOption.ADDITIONAL_HTTP_HEADERS, new HashMap<>())
.option(SdkClientOption.CRC32_FROM_COMPRESSED_DATA_ENABLED, false)
.option(AwsClientOption.CREDENTIALS_PROVIDER, DefaultCredentialsProvider.create())
Expand All @@ -64,14 +64,8 @@ public static SdkClientConfiguration testClientConfiguration() {
}

public static class TestClientBuilder {
private RetryPolicy retryPolicy;
private SdkHttpClient httpClient;

public TestClientBuilder retryPolicy(RetryPolicy retryPolicy) {
this.retryPolicy = retryPolicy;
return this;
}

public TestClientBuilder httpClient(SdkHttpClient sdkHttpClient) {
this.httpClient = sdkHttpClient;
return this;
Expand All @@ -81,14 +75,7 @@ public AmazonSyncHttpClient build() {
SdkHttpClient sdkHttpClient = this.httpClient != null ? this.httpClient : testSdkHttpClient();
return new AmazonSyncHttpClient(testClientConfiguration().toBuilder()
.option(SdkClientOption.SYNC_HTTP_CLIENT, sdkHttpClient)
.applyMutation(this::configureRetryPolicy)
.build());
}

private void configureRetryPolicy(SdkClientConfiguration.Builder builder) {
if (retryPolicy != null) {
builder.option(SdkClientOption.RETRY_POLICY, retryPolicy);
}
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -204,6 +204,13 @@ default B retryOnRootCauseInstanceOf(Class<? extends Throwable> throwable) {
*/
B maxAttempts(int maxAttempts);

/**
* Configure the backoff strategy used by this executor.
*
* <p>By default, this uses jittered exponential backoff.
*/
B backoffStrategy(BackoffStrategy backoffStrategy);

/**
* Build a new {@link RetryStrategy} with the current configuration on this builder.
*/
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ private BackoffStrategiesConstants() {
* get capped to 30.
*/
static int calculateExponentialDelay(int retriesAttempted, Duration baseDelay, Duration maxBackoffTime) {
int cappedRetries = Math.min(retriesAttempted, BackoffStrategiesConstants.RETRIES_ATTEMPTED_CEILING);
int cappedRetries = Math.min(retriesAttempted, RETRIES_ATTEMPTED_CEILING);
return (int) Math.min(baseDelay.multipliedBy(1L << (cappedRetries - 2)).toMillis(), maxBackoffTime.toMillis());
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -153,6 +153,11 @@ public BuilderToTestDefaults maxAttempts(int maxAttempts) {
return this;
}

@Override
public BuilderToTestDefaults backoffStrategy(BackoffStrategy backoffStrategy) {
return this;
}

@Override
public DummyRetryStrategy build() {
return null;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,7 @@ public static AdaptiveRetryStrategy.Builder adaptiveStrategyBuilder() {

static final class Standard {
static final int MAX_ATTEMPTS = 3;
static final Duration BASE_DELAY = Duration.ofSeconds(1);
static final Duration BASE_DELAY = Duration.ofMillis(100);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Bug fix, the value is 100ms see here.

static final Duration MAX_BACKOFF = Duration.ofSeconds(20);
static final int TOKEN_BUCKET_SIZE = 500;
static final int DEFAULT_EXCEPTION_TOKEN_COST = 5;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -74,13 +74,6 @@ static Builder builder() {
Builder toBuilder();

interface Builder extends RetryStrategy.Builder<Builder, LegacyRetryStrategy> {
/**
* Configure the backoff strategy used by this strategy.
*
* <p>By default, this uses jittered exponential backoff.
*/
Builder backoffStrategy(BackoffStrategy backoffStrategy);

/**
* Configure the backoff strategy used for throttling exceptions by this strategy.
*
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -70,13 +70,6 @@ static Builder builder() {
Builder toBuilder();

interface Builder extends RetryStrategy.Builder<Builder, StandardRetryStrategy> {
/**
* Configure the backoff strategy used by this executor.
*
* <p>By default, this uses jittered exponential backoff.
*/
Builder backoffStrategy(BackoffStrategy backoffStrategy);

/**
* Whether circuit breaking is enabled for this executor.
*
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -146,7 +146,6 @@ public int maxAttempts() {
@Override
public abstract B toBuilder();


/**
* Computes the backoff before the first attempt, by default
* {@link Duration#ZERO}. Extending classes can override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -111,13 +111,14 @@ public Builder treatAsThrottling(Predicate<Throwable> treatAsThrottling) {
return this;
}

public Builder circuitBreakerEnabled(Boolean circuitBreakerEnabled) {
setCircuitBreakerEnabled(circuitBreakerEnabled);
@Override
public Builder backoffStrategy(BackoffStrategy backoffStrategy) {
setBackoffStrategy(backoffStrategy);
return this;
}

public Builder backoffStrategy(BackoffStrategy backoffStrategy) {
setBackoffStrategy(backoffStrategy);
public Builder circuitBreakerEnabled(Boolean circuitBreakerEnabled) {
setCircuitBreakerEnabled(circuitBreakerEnabled);
return this;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
import software.amazon.awssdk.retries.api.RefreshRetryTokenRequest;
import software.amazon.awssdk.retries.internal.circuitbreaker.TokenBucketStore;
import software.amazon.awssdk.utils.Logger;
import software.amazon.awssdk.utils.Validate;

@SdkInternalApi
public final class DefaultLegacyRetryStrategy
Expand All @@ -34,9 +35,9 @@ public final class DefaultLegacyRetryStrategy

DefaultLegacyRetryStrategy(Builder builder) {
super(LOG, builder);
this.throttlingExceptionCost = builder.throttlingExceptionCost;
this.throttlingBackoffStrategy = builder.throttlingBackoffStrategy;
this.treatAsThrottling = builder.treatAsThrottling;
this.throttlingExceptionCost = Validate.paramNotNull(builder.throttlingExceptionCost, "throttlingExceptionCost");
this.throttlingBackoffStrategy = Validate.paramNotNull(builder.throttlingBackoffStrategy, "throttlingBackoffStrategy");
this.treatAsThrottling = Validate.paramNotNull(builder.treatAsThrottling, "treatAsThrottling");
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,10 +28,10 @@
import software.amazon.awssdk.core.exception.SdkClientException;
import software.amazon.awssdk.core.internal.http.AmazonSyncHttpClient;
import software.amazon.awssdk.core.internal.http.response.NullErrorResponseHandler;
import software.amazon.awssdk.core.retry.RetryPolicy;
import software.amazon.awssdk.http.SdkHttpFullRequest;
import software.amazon.awssdk.http.SdkHttpMethod;
import software.amazon.awssdk.http.apache.ApacheHttpClient;
import software.amazon.awssdk.retries.DefaultRetryStrategy;
import utils.HttpTestUtils;

/**
Expand All @@ -48,7 +48,7 @@ public class AmazonHttpClientSslHandshakeTimeoutTest extends UnresponsiveMockSer
@Test(timeout = 60 * 1000)
public void testSslHandshakeTimeout() {
AmazonSyncHttpClient httpClient = HttpTestUtils.testClientBuilder()
.retryPolicy(RetryPolicy.none())
.retryStrategy(DefaultRetryStrategy.none())
.httpClient(ApacheHttpClient.builder()
.socketTimeout(CLIENT_SOCKET_TO)
.build())
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,10 +30,10 @@
import software.amazon.awssdk.core.http.server.MockServer;
import software.amazon.awssdk.core.internal.http.AmazonSyncHttpClient;
import software.amazon.awssdk.core.internal.http.response.EmptySdkResponseHandler;
import software.amazon.awssdk.core.retry.RetryPolicy;
import software.amazon.awssdk.http.SdkHttpFullRequest;
import software.amazon.awssdk.http.SdkHttpMethod;
import software.amazon.awssdk.http.apache.ApacheHttpClient;
import software.amazon.awssdk.retries.DefaultRetryStrategy;
import utils.HttpTestUtils;

public class ConnectionPoolMaxConnectionsIntegrationTest {
Expand All @@ -57,7 +57,7 @@ public static void tearDown() {
public void leasing_a_new_connection_fails_with_connection_pool_timeout() {

AmazonSyncHttpClient httpClient = HttpTestUtils.testClientBuilder()
.retryPolicy(RetryPolicy.none())
.retryStrategy(DefaultRetryStrategy.none())
.httpClient(ApacheHttpClient.builder()
.connectionTimeout(Duration.ofMillis(100))
.maxConnections(1)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@
import software.amazon.awssdk.core.retry.RetryUtils;
import software.amazon.awssdk.retries.api.AcquireInitialTokenRequest;
import software.amazon.awssdk.retries.api.AcquireInitialTokenResponse;
import software.amazon.awssdk.retries.api.BackoffStrategy;
import software.amazon.awssdk.retries.api.RecordSuccessRequest;
import software.amazon.awssdk.retries.api.RecordSuccessResponse;
import software.amazon.awssdk.retries.api.RefreshRetryTokenRequest;
Expand Down Expand Up @@ -164,6 +165,11 @@ public Builder maxAttempts(int maxAttempts) {
throw new UnsupportedOperationException("RetryPolicyAdapter does not support calling retryOnException");
}

@Override
public Builder backoffStrategy(BackoffStrategy backoffStrategy) {
throw new UnsupportedOperationException("RetryPolicyAdapter does not support calling backoffStrategy");
}

public Builder retryPolicy(RetryPolicy retryPolicy) {
this.retryPolicy = retryPolicy;
return this;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -42,9 +42,12 @@
*
* @see RetryCondition for a list of SDK provided retry condition strategies
* @see BackoffStrategy for a list of SDK provided backoff strategies
*
* @deprecated Use instead {@link software.amazon.awssdk.retries.api.RetryStrategy}.
*/
@Immutable
@SdkPublicApi
@Deprecated
public final class RetryPolicy implements ToCopyableBuilder<RetryPolicy.Builder, RetryPolicy> {
private final boolean additionalRetryConditionsAllowed;
private final RetryMode retryMode;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,12 +21,13 @@
import software.amazon.awssdk.core.exception.SdkException;
import software.amazon.awssdk.core.interceptor.ExecutionAttributes;
import software.amazon.awssdk.http.SdkHttpFullRequest;
import software.amazon.awssdk.retries.api.RetryStrategy;
import software.amazon.awssdk.utils.builder.CopyableBuilder;
import software.amazon.awssdk.utils.builder.ToCopyableBuilder;

/**
* Contains useful information about a failed request that can be used to make retry and backoff decisions. See {@link
* RetryPolicy}.
* RetryPolicy} and {@link RetryStrategy}.
*/
@Immutable
@SdkPublicApi
Expand Down Expand Up @@ -163,5 +164,4 @@ public RetryPolicyContext build() {
}

}

}
Original file line number Diff line number Diff line change
Expand Up @@ -21,8 +21,12 @@
import software.amazon.awssdk.core.retry.RetryMode;
import software.amazon.awssdk.core.retry.RetryPolicyContext;

/**
* @deprecated Use instead {@link software.amazon.awssdk.retries.api.BackoffStrategy}
*/
@SdkPublicApi
Copy link
Contributor

Choose a reason for hiding this comment

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

SdkPublicApi can be removed if its deprecated. Generic comment for other classes

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See above, the build requires each class to have one of the API scope annotations. If we remove this one, which one should we use?

@FunctionalInterface
@Deprecated
public interface BackoffStrategy {

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -39,8 +39,12 @@
*
* This is in contrast to {@link FullJitterBackoffStrategy} where the final computed delay before the next retry will be
* between 0 and the computed exponential delay.
*
* @deprecated Use instead {@link software.amazon.awssdk.retries.api.BackoffStrategy} and
* {@link software.amazon.awssdk.retries.api.BackoffStrategy#fixedDelayWithoutJitter(Duration)}
*/
@SdkPublicApi
@Deprecated
public final class EqualJitterBackoffStrategy implements BackoffStrategy,
ToCopyableBuilder<EqualJitterBackoffStrategy.Builder,
EqualJitterBackoffStrategy> {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,8 +24,12 @@

/**
* Simple backoff strategy that always uses a fixed delay for the delay before the next retry attempt.
*
* @deprecated Use instead {@link software.amazon.awssdk.retries.api.BackoffStrategy} and
* {@link software.amazon.awssdk.retries.api.BackoffStrategy#fixedDelay(Duration)}.
*/
@SdkPublicApi
@Deprecated
public final class FixedDelayBackoffStrategy implements BackoffStrategy {

private final Duration fixedBackoff;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -36,8 +36,12 @@
*
* This is in contrast to {@link EqualJitterBackoffStrategy} that computes a new random delay where the final
* computed delay before the next retry will be at least half of the computed exponential delay.
*
* @deprecated Use instead {@link software.amazon.awssdk.retries.api.BackoffStrategy} and
* {@link software.amazon.awssdk.retries.api.BackoffStrategy#exponentialDelayWithoutJitter(Duration, Duration)}.
*/
@SdkPublicApi
@Deprecated
public final class FullJitterBackoffStrategy implements BackoffStrategy,
ToCopyableBuilder<FullJitterBackoffStrategy.Builder,
FullJitterBackoffStrategy> {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,12 +15,17 @@

package software.amazon.awssdk.core.retry.conditions;

import java.util.function.Predicate;
import software.amazon.awssdk.annotations.SdkPublicApi;
import software.amazon.awssdk.core.internal.retry.SdkDefaultRetrySetting;
import software.amazon.awssdk.core.retry.RetryPolicyContext;

/**
* @deprecated Use instead {@link software.amazon.awssdk.retries.api.RetryStrategy.Builder#retryOnException(Predicate)}.
*/
@SdkPublicApi
@FunctionalInterface
@Deprecated
public interface RetryCondition {
/**
* Determine whether a request should or should not be retried.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,6 @@
import software.amazon.awssdk.core.exception.SdkServiceException;
import software.amazon.awssdk.core.http.HttpResponseHandler;
import software.amazon.awssdk.core.protocol.VoidSdkResponse;
import software.amazon.awssdk.core.retry.RetryPolicy;
import software.amazon.awssdk.core.runtime.transform.Marshaller;
import software.amazon.awssdk.core.sync.ResponseTransformer;
import software.amazon.awssdk.http.AbortableInputStream;
Expand All @@ -51,6 +50,7 @@
import software.amazon.awssdk.http.SdkHttpClient;
import software.amazon.awssdk.http.SdkHttpFullRequest;
import software.amazon.awssdk.http.SdkHttpResponse;
import software.amazon.awssdk.retries.DefaultRetryStrategy;
import utils.HttpTestUtils;
import utils.ValidSdkObjects;

Expand Down Expand Up @@ -205,7 +205,7 @@ private ClientExecutionParams<SdkRequest, SdkResponse> clientExecutionParams() {
public SdkClientConfiguration clientConfiguration() {
return HttpTestUtils.testClientConfiguration().toBuilder()
.option(SdkClientOption.SYNC_HTTP_CLIENT, httpClient)
.option(SdkClientOption.RETRY_POLICY, RetryPolicy.none())
.option(SdkClientOption.RETRY_STRATEGY, DefaultRetryStrategy.none())
.build();
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -47,12 +47,10 @@
import software.amazon.awssdk.core.interceptor.InterceptorContext;
import software.amazon.awssdk.core.internal.http.AmazonAsyncHttpClient;
import software.amazon.awssdk.core.internal.http.request.SlowExecutionInterceptor;
import software.amazon.awssdk.core.retry.RetryPolicy;
import software.amazon.awssdk.core.signer.NoOpSigner;
import software.amazon.awssdk.http.SdkHttpFullRequest;
import software.amazon.awssdk.metrics.MetricCollector;
import software.amazon.awssdk.retries.DefaultRetryStrategy;
import software.amazon.awssdk.retries.api.RetryStrategy;
import utils.ValidSdkObjects;

public class AsyncHttpClientApiCallTimeoutTests {
Expand All @@ -65,7 +63,6 @@ public class AsyncHttpClientApiCallTimeoutTests {
@Before
public void setup() {
httpClient = testAsyncClientBuilder()
.retryPolicy(RetryPolicy.none())
.retryStrategy(DefaultRetryStrategy.none())
.apiCallTimeout(API_CALL_TIMEOUT)
.build();
Expand Down
Loading