Add new sync and async retryable stages #4062

sugmanue · 2023-06-03T00:10:46Z

This new module includes the interfaces and classes that will be used to implement the new retry logic within the SDK.

Notes

This change creates a retryable stages for the sync and async pipelines along side with a new helper. To avoid noisy diffs I suffixed the classes names with 2. After this is approved I will move the stages and helper to the original names.

Notice that this change does not use yet the new retry policies but instead it will use the adapter, this will allow us to validate that all the current tests cases work fine with the adapter and later on we will change the SdkDefaultClientBuilder.java and AwsSdkDefaultClientBuilder.java classes to return null if no explicit retry policy is configured and thus pick the retry strategy.

Also, there are several fixme comments that will be addressed in a follow up PR and/or when I validate the assumptions made.

Testing

The current tests are not modified to validate that the wrapper still works on all the existing tests cases.

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)

Checklist

I have read the CONTRIBUTING document
Local run of mvn install succeeds
My code follows the code style of this project
My change requires a change to the Javadoc documentation
I have updated the Javadoc documentation accordingly
I have added tests to cover my changes
All new and existing tests passed
I have added a changelog entry. Adding a new entry must be accomplished by running the scripts/new-change script and following the instructions. Commit the new file created by the script in .changes/next-release with your changes.
My change is to implement 1.11 parity feature and I have updated LaunchChangelog

License

I confirm that this pull request can be released under the Apache 2 license

joviegas · 2023-06-06T20:17:17Z

...ore/src/main/java/software/amazon/awssdk/awscore/client/builder/AwsDefaultClientBuilder.java

@@ -375,6 +378,35 @@ private RetryPolicy resolveAwsRetryPolicy(SdkClientConfiguration config) {
                                       .defaultRetryMode(config.option(SdkClientOption.DEFAULT_RETRY_MODE))
                                       .resolve();
        return AwsRetryPolicy.forRetryMode(retryMode);
+        // fixme This will be changed like this to pick the configured retry strategy


Can we Tag this as TODO :
Since it is identified in our code checks/checkstyles of release

I can make the change but just to clarify, this is going to be removed in the next PR, and as explained in the description, we kept this like this just to validate that the adapter from retry policy to retry strategy still passes all the unit and integration tests. After that, I will switch to use by default a retry strategy if there's no a user defined retry policy, which will validate that it also does pass all the unit / integration tests already present.

joviegas · 2023-06-06T20:22:18Z

...ore/src/main/java/software/amazon/awssdk/awscore/client/builder/AwsDefaultClientBuilder.java

+         */
+    }
+
+    private RetryStrategy<?, ?> resolveAwsRetryStrategy(SdkClientConfiguration config) {


Can we avoid the Generic wildcard types by referring to the parent classes ?

No, we cannot. There's no specific or general enough type that we can use here.

core/aws-core/src/main/java/software/amazon/awssdk/awscore/retry/AwsRetryStrategy.java

core/sdk-core/src/main/java/software/amazon/awssdk/core/client/config/SdkClientOption.java

...src/main/java/software/amazon/awssdk/core/internal/http/pipeline/stages/RetryableStage2.java

sonarqubecloud · 2023-06-07T00:51:11Z

SonarCloud Quality Gate failed.

1 Bug
0 Vulnerabilities
0 Security Hotspots
42 Code Smells

80.5% Coverage
1.7% Duplication

...c/test/java/software/amazon/awssdk/core/internal/http/response/NullErrorResponseHandler.java

joviegas · 2023-06-07T19:43:54Z

...ain/java/software/amazon/awssdk/core/internal/http/pipeline/stages/AsyncRetryableStage2.java

+            if (backoffDelay.isZero()) {
+                attemptExecute(future);
+            } else {
+                retryableStageHelper.logBackingOff(backoffDelay);


I could not find a Junit for this case , how is this tested ?

I will add more tests for these after this is merged in a follow up PR; the idea is to make smaller changes to make it easier to review with self-contained minimal changes that build on top of each other.

...src/main/java/software/amazon/awssdk/core/internal/http/pipeline/stages/RetryableStage2.java

joviegas · 2023-06-07T19:47:19Z

core/sdk-core/src/main/java/software/amazon/awssdk/core/internal/retry/RetryPolicyAdapter.java

+        if (!isRateLimitingEnabled()) {
+            return Duration.ZERO;
+        }
+        OptionalDouble tokenAcquireTimeSeconds = rateLimitingTokenBucket.acquireNonBlocking(1.0, isFastFailRateLimiting());


Can we please add coverage for this case in Junit it currently not shown in coevrage

I will add more tests for these after this is merged in a follow up PR; the idea is to make smaller changes to make it easier to review with self-contained minimal changes that build on top of each other.

joviegas · 2023-06-07T19:48:26Z

...k-core/src/main/java/software/amazon/awssdk/core/internal/retry/SdkDefaultRetryStrategy.java

+ * Retry strategies used by any SDK client.
+ */
+@SdkPublicApi
+public final class SdkDefaultRetryStrategy {


Cane we add a Junit test case for this class ?

I will add more tests for these after this is merged in a follow up PR; the idea is to make smaller changes to make it easier to review with self-contained minimal changes that build on top of each other.

joviegas · 2023-06-07T19:49:37Z

core/aws-core/src/main/java/software/amazon/awssdk/awscore/retry/AwsRetryStrategy.java

+            case LEGACY:
+                return legacyRetryStrategy();
+            default:
+                throw new IllegalArgumentException("unknown retry mode: " + mode);


Junit for AwsRetryStrategy and ADAPTIVE case ?

I will add more tests for these after this is merged in a follow up PR; the idea is to make smaller changes to make it easier to review with self-contained minimal changes that build on top of each other.

joviegas · 2023-06-07T19:52:12Z

Can we please take a look at #4062 (comment) and address code smells which ever possible , or ACK if its already taken care.

sugmanue · 2023-06-07T21:21:01Z

Can we please take a look at #4062 (comment) and address code smells which ever possible , or ACK if its already taken care.

I have no idea how to take care of it, the tool is saying that

        private static final double SCALE_CONSTANT = 0.4; (1)

(1) implies that SCALE_CONSTANT is zero which if you look closely is not, 0.4 > 0 and then saying that therefore:

   this.timeWindow = Math.pow((this.lastMaxRate * (1 - BETA)) / SCALE_CONSTANT, 1.0 / 3); (2)

The use of that constant in (2) will lead to division by zero, which it won't, because is not zero. I think this is a bug in SonarCloud and I don't know how to ACK or silence it, thoughts?

joviegas · 2023-06-07T21:31:52Z

Can we please take a look at #4062 (comment) and address code smells which ever possible , or ACK if its already taken care.

I have no idea how to take care of it, the tool is saying that
        private static final double SCALE_CONSTANT = 0.4; (1)
(1) implies that SCALE_CONSTANT is zero which if you look closely is not, 0.4 > 0 and then saying that therefore:
   this.timeWindow = Math.pow((this.lastMaxRate * (1 - BETA)) / SCALE_CONSTANT, 1.0 / 3); (2)
The use of that constant in (2) will lead to division by zero, which it won't, because is not zero. I think this is a bug in SonarCloud and I don't know how to ACK or silence it, thoughts?

Agree , looks like its a bug in SonarCloud Quality Gate.
Did you get a chance to take a look at the Code smells are there any codesmells which we can eliminate ?

sugmanue · 2023-06-07T21:51:46Z

Agree , looks like its a bug in SonarCloud Quality Gate. Did you get a chance to take a look at the Code smells are there any codesmells which we can eliminate ?

Not really, some are well known such as use of wildcards, others are expected such as catching Throwable and others will go away soon. Let me know if you have identified any worth taking a look into.

@deprecated

* New API for the retries module (#3769) This new module includes the interfaces and classes that will be used to implement the new retry logic within the SDK. * Add default backoff strategies (#3906) * Add default backoff strategies * Moved the backoff strategires to the SPI package * Use AssertJ instead of Hamcrest * Add standard retry strategy (#3931) * Add standard retry strategy * Fix the AcquireInitialTokenRequestImpl API annotation Also add the package to the test/tests-coverage-reporting/pom.xml to get coverage reporting * Add adaptive retry strategy (#3975) * Add adaptive retry strategy * Address pull request comments * Address PR comments * Address PR comments * Update retries and retries-api to snapshot version: 2.20.64-SNAPSHOT * Fix SonarCloud code smells (#3991) * Fix SonarCloud code smells * Move AdaptiveRetryStrategyResourceConstrainedTest to an integration test This change is to workaround the SonarCloud code smell of the Sleep usage in this test * Add legacy retry strategy (#3988) * Add legacy retry strategy * Remove public modifiers from test classes to make SonarCloud happy * Fix another SonarCloud code smell * WIP * Address PR comments * Rename all the strategies to use Default prefix instead of Impl suffix * Address PR comments * Remove those tests that are now part of a different class * Update version after merge from master * Refactor retry strategies (#4039) * Refactor the retry strategies This change uses a single class to implement the core logic of all the retries strategies and adds extension points to tailor the behavior when needed. * Rename to BaseRetryStrategy and make it abstract * Remove previous implementations and rename the new ones * Update sdk version * Fix the retry condition to just look for the initial cause * Add new sync and async retryable stages (#4062) * Add new sync and async retryable stages * Address PR comments * Update sdk version * Change uses of RetryPolicy to RetryStrategy (#4125) * Update sdk version * Deprecate legacy classes and use new when possible (#4154) * Deprecate legacy classes and use new when possible * Fix checkstyle and add some more validation * Add missing @deprecated annotation * Add missing dependency to the retries-api module * Fix minor logging issues * Update sdk version * Add support for retryable trait (#4170) * Merge master * Update to support plugins * Add support for AWS retryable conditions * Use the correct token bucket exception cost value * Add ADAPTIVE_V2 retry mode to support the legacy behavior (#5123) * Add a new ADAPTIVE2 mode to support the legacy behavior * Fix dynamodb test to use adaptive2 mode * Fixes and tests for the expected behaviors * Rename the new adaptive mode to ADAPTIVE_V2 * More fixes related to the rename from adaptive2 to adaptive_v2 * Fix dynamodb retry resolver logic for adaptive mode * Properly clean up the test state * Address PR comments * Remove a small typo * Dumy commit * Dummy commit to kick the internal build * Rename retries-api to retries-spi * Add retry packages to brazil (#5215) * Add retry packages to brazil * Update pom's as per the new module checklist * Remove type params from RetryStrategy, but keep them in RetryStrategy… (#5262) * Remove type params from RetryStrategy, but keep them in RetryStrategy.Builder * Rename from `none` to `doNotRetry` to clarify the behavior * External names used for retry modes only support 'adaptive' (#5265) * Externally named retry modes only support 'adaptive' Behind the scenes this will be mapped to RetryMode.ADAPTIVE_V2 which makes it a non-backwards compatible behavioral change. * Sneak in a fix from the previous PR * Fix a test that expects adaptive to map to `RetryMode.ADAPTIVE` * Fix typos in the comments * Retries release (#5280) * Bump version to 2.26.0-SNAPSHOT * Add retry release changlog entry * Add missing deprecation annotation and javadoc tag * Archive the last changelog from the 2.25 series --------- Co-authored-by: John Viegas <[email protected]>

@deprecated

* New API for the retries module (aws#3769) This new module includes the interfaces and classes that will be used to implement the new retry logic within the SDK. * Add default backoff strategies (aws#3906) * Add default backoff strategies * Moved the backoff strategires to the SPI package * Use AssertJ instead of Hamcrest * Add standard retry strategy (aws#3931) * Add standard retry strategy * Fix the AcquireInitialTokenRequestImpl API annotation Also add the package to the test/tests-coverage-reporting/pom.xml to get coverage reporting * Add adaptive retry strategy (aws#3975) * Add adaptive retry strategy * Address pull request comments * Address PR comments * Address PR comments * Update retries and retries-api to snapshot version: 2.20.64-SNAPSHOT * Fix SonarCloud code smells (aws#3991) * Fix SonarCloud code smells * Move AdaptiveRetryStrategyResourceConstrainedTest to an integration test This change is to workaround the SonarCloud code smell of the Sleep usage in this test * Add legacy retry strategy (aws#3988) * Add legacy retry strategy * Remove public modifiers from test classes to make SonarCloud happy * Fix another SonarCloud code smell * WIP * Address PR comments * Rename all the strategies to use Default prefix instead of Impl suffix * Address PR comments * Remove those tests that are now part of a different class * Update version after merge from master * Refactor retry strategies (aws#4039) * Refactor the retry strategies This change uses a single class to implement the core logic of all the retries strategies and adds extension points to tailor the behavior when needed. * Rename to BaseRetryStrategy and make it abstract * Remove previous implementations and rename the new ones * Update sdk version * Fix the retry condition to just look for the initial cause * Add new sync and async retryable stages (aws#4062) * Add new sync and async retryable stages * Address PR comments * Update sdk version * Change uses of RetryPolicy to RetryStrategy (aws#4125) * Update sdk version * Deprecate legacy classes and use new when possible (aws#4154) * Deprecate legacy classes and use new when possible * Fix checkstyle and add some more validation * Add missing @deprecated annotation * Add missing dependency to the retries-api module * Fix minor logging issues * Update sdk version * Add support for retryable trait (aws#4170) * Merge master * Update to support plugins * Add support for AWS retryable conditions * Use the correct token bucket exception cost value * Add ADAPTIVE_V2 retry mode to support the legacy behavior (aws#5123) * Add a new ADAPTIVE2 mode to support the legacy behavior * Fix dynamodb test to use adaptive2 mode * Fixes and tests for the expected behaviors * Rename the new adaptive mode to ADAPTIVE_V2 * More fixes related to the rename from adaptive2 to adaptive_v2 * Fix dynamodb retry resolver logic for adaptive mode * Properly clean up the test state * Address PR comments * Remove a small typo * Dumy commit * Dummy commit to kick the internal build * Rename retries-api to retries-spi * Add retry packages to brazil (aws#5215) * Add retry packages to brazil * Update pom's as per the new module checklist * Remove type params from RetryStrategy, but keep them in RetryStrategy… (aws#5262) * Remove type params from RetryStrategy, but keep them in RetryStrategy.Builder * Rename from `none` to `doNotRetry` to clarify the behavior * External names used for retry modes only support 'adaptive' (aws#5265) * Externally named retry modes only support 'adaptive' Behind the scenes this will be mapped to RetryMode.ADAPTIVE_V2 which makes it a non-backwards compatible behavioral change. * Sneak in a fix from the previous PR * Fix a test that expects adaptive to map to `RetryMode.ADAPTIVE` * Fix typos in the comments * Retries release (aws#5280) * Bump version to 2.26.0-SNAPSHOT * Add retry release changlog entry * Add missing deprecation annotation and javadoc tag * Archive the last changelog from the 2.25 series --------- Co-authored-by: John Viegas <[email protected]>

sugmanue requested a review from a team as a code owner June 3, 2023 00:10

sugmanue force-pushed the sugmanue/update-retry-executors2 branch from 86b52e2 to 3643006 Compare June 3, 2023 00:29

Add new sync and async retryable stages

56645ac

sugmanue force-pushed the sugmanue/update-retry-executors2 branch from 3643006 to 56645ac Compare June 3, 2023 01:44

joviegas reviewed Jun 6, 2023

View reviewed changes

Address PR comments

9d0ad58

joviegas reviewed Jun 7, 2023

View reviewed changes

joviegas approved these changes Jun 7, 2023

View reviewed changes

sugmanue merged commit 811c9b1 into feature/master/sra-retries Jun 7, 2023

sugmanue deleted the sugmanue/update-retry-executors2 branch June 28, 2023 02:05

Add new sync and async retryable stages #4062

Add new sync and async retryable stages #4062

Uh oh!

Conversation

sugmanue commented Jun 3, 2023

Notes

Testing

Types of changes

Checklist

License

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sugmanue Jun 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sonarqubecloud bot commented Jun 7, 2023

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sugmanue Jun 7, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joviegas commented Jun 7, 2023

Uh oh!

sugmanue commented Jun 7, 2023

Uh oh!

joviegas commented Jun 7, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sugmanue commented Jun 7, 2023

Uh oh!

Uh oh!

sugmanue Jun 6, 2023 •

edited

Loading

sugmanue Jun 7, 2023 •

edited

Loading

joviegas commented Jun 7, 2023 •

edited

Loading