New API for the retries module #3769

sugmanue · 2023-02-17T19:11:18Z

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

Motivation and Context

As part of the Smithy Reference Architecture this change adds a new module for retries. This change adds only the API part.

Modifications

No modifications to existing code were made. We will make those changes in follow up pull requests.

Testing

Added unit tests for all the default methods in the RetryStrategy.Builder interface that create lambdas to be used to decide if an exception should be retried.
Added unit tests for ExponentialDelayWithJitter and FixedDelayWithJitterTest similar to the tests present for its equivalent class here

Screenshots (if appropriate)

N/A

Types of changes

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

Checklist

[ x] I have read the CONTRIBUTING document
[ x] Local run of mvn install succeeds
[ x] My code follows the code style of this project
[ x] My change requires a change to the Javadoc documentation
[ x] I have updated the Javadoc documentation accordingly
[ x] I have added tests to cover my changes
[ x] 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

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

zoewangg

Left some initial comments, still going through the PR.

zoewangg · 2023-02-25T02:22:27Z

core/retries-api/pom.xml

+            <artifactId>assertj-core</artifactId>
+            <scope>test</scope>
+        </dependency>
+        <dependency>


Let's remove junit4 and use junit5 for all tests in this module.

Sure, I will change it.

zoewangg · 2023-02-25T02:27:17Z

core/retries-api/src/main/java/software/amazon/awssdk/retriesapi/BackoffStrategy.java

+    /**
+     * Strategy that waits for a period of time equal to the provided delay.
+     */
+    final class FixedDelayWithoutJitter implements BackoffStrategy {


Can we create separate classes for them?

Sure, I can move those into their own files.

zoewangg · 2023-02-25T02:52:48Z

core/retries-api/src/main/java/software/amazon/awssdk/retriesapi/RetryStrategy.java

+         * Configure the strategy to retry the root cause of a failure (the final cause) a failure exception is an instance of to
+         * the provided class (includes subtypes).
+         */
+        default Builder retryOnRootCauseInstanceOf(Class<? extends Throwable> throwable) {


I wonder if we should expose so many retryXXX methods. Too many APIs could make this harder to understand and use.

The number of APIs will be doubled if we add varargs overloads.

I guess that some of these conditions are subtle and error prone, having these spell out with proper names make the API easier to use and understand, but I can remove some of these. Which ones do you think it will be worth keeping and which ones do you think we can go without?

zoewangg · 2023-02-25T02:59:25Z

core/retries-api/src/main/java/software/amazon/awssdk/retriesapi/RetryStrategy.java

+         * Configure the strategy to retry when a failure exception class is an instance of the provided class (includes
+         * subtypes).
+         */
+        default Builder retryOnExceptionInstanceOf(Class<? extends Throwable> throwable) {


Should we create APIs to allow users to pass a list of exceptions?

I guess the argument in favor would be that it will make some expressions easier but the same can be accomplish by calling this method for each. Since I'm not quite sure that this will add much I think it would be better to do that when and if the need araises.

zoewangg · 2023-02-25T03:03:55Z

.../retries-api/src/main/java/software/amazon/awssdk/retriesapi/AcquireInitialTokenRequest.java

+ * permissions and limitations under the License.
+ */
+
+package software.amazon.awssdk.retriesapi;


Should it be software.amazon.awssdk.retries.api?

I just realized that the original plan was to use instead software.amazon.awssdk.api.retries, I can go either way but I like the original idea better, thoughts?

zoewangg · 2023-02-25T03:12:56Z

core/retries-api/src/main/java/software/amazon/awssdk/retriesapi/BackoffStrategy.java

+@SdkPublicApi
+@ThreadSafe
+public interface BackoffStrategy {
+    Duration BASE_DELAY_CEILING = Duration.ofMillis(Integer.MAX_VALUE); // Around ~24.8 days


These constants seem to be implementation details, should we move them to the classes that use them? If they are shared between classes, should we move them to a constant class?

zoewangg · 2023-02-25T03:14:09Z

...ies-api/src/main/java/software/amazon/awssdk/retriesapi/TokenAcquisitionFailedException.java

+ * Exception thrown by {@link RetryStrategy} when a new token cannot be acquired.
+ */
+@SdkPublicApi
+public final class TokenAcquisitionFailedException extends RuntimeException {


I wonder if it should extend SdkClientException to make it easier for customers to handle

I guess we could depend on core, but not sure if we want to complect those. This package for now only depends on annotations and utils and I feel that adding sdk-core will make it a lot less easier to reuse outside the SDK.

Also, moved the backoff strategies out of the interface class an into their own clasess and remove the constants from the interface into its own class.

* Move the namespace from api.retries to retries.api to make it consistent with the identity module * Remove the backoff implementations and static methods from the interface, those classes will be living in the implementation package.

zoewangg · 2023-03-07T00:49:26Z

core/retries-api/pom.xml

+                <configuration>
+                    <archive>
+                        <manifestEntries>
+                            <Automatic-Module-Name>software.amazon.awssdk.retriesapi</Automatic-Module-Name>


This needs to match with the top-level package name.

sonarcloud · 2023-03-07T20:37:12Z

SonarCloud Quality Gate failed.

0 Bugs
0 Vulnerabilities
0 Security Hotspots
2 Code Smells

64.2% Coverage
0.0% Duplication

@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 <70235430+joviegas@users.noreply.github.com>

@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 <70235430+joviegas@users.noreply.github.com>

New API for the retries module

24947c8

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

sugmanue requested a review from a team as a code owner February 17, 2023 19:11

sugmanue assigned zoewangg Feb 17, 2023

zoewangg reviewed Feb 25, 2023

View reviewed changes

sugmanue added 4 commits March 3, 2023 18:32

Move the namespace from retriesapi to api.retries

5207c86

Also, moved the backoff strategies out of the interface class an into their own clasess and remove the constants from the interface into its own class.

Update the wording in the feature json file

272758a

Minor stylistic changes to add javadoc and remove extra space

52c5526

zoewangg reviewed Mar 7, 2023

View reviewed changes

Fix the Automatic-Module-Name to match the package changes

e28fadf

zoewangg approved these changes Mar 7, 2023

View reviewed changes

sugmanue merged commit 7ef8830 into feature/master/sra-retries Mar 8, 2023

sugmanue deleted the sugmanue/add-retry-interfaces branch March 8, 2023 18:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New API for the retries module #3769

New API for the retries module #3769

sugmanue commented Feb 17, 2023

zoewangg left a comment •

edited

Loading

zoewangg Feb 25, 2023

sugmanue Mar 3, 2023

zoewangg Feb 25, 2023

sugmanue Mar 3, 2023

zoewangg Feb 25, 2023

sugmanue Mar 3, 2023

zoewangg Feb 25, 2023

sugmanue Mar 3, 2023

zoewangg Feb 25, 2023

sugmanue Mar 3, 2023

zoewangg Feb 25, 2023

zoewangg Feb 25, 2023

sugmanue Mar 3, 2023

zoewangg Mar 7, 2023

sonarcloud bot commented Mar 7, 2023

New API for the retries module #3769

New API for the retries module #3769

Conversation

sugmanue commented Feb 17, 2023

Motivation and Context

Modifications

Testing

Screenshots (if appropriate)

Types of changes

Checklist

License

zoewangg left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarcloud bot commented Mar 7, 2023

zoewangg left a comment •

edited

Loading