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

Change STANDARD mode retry count for DynamoDB to match LEGACY as V1 #2324

Merged
merged 1 commit into from
Mar 15, 2021
Merged

Change STANDARD mode retry count for DynamoDB to match LEGACY as V1 #2324

merged 1 commit into from
Mar 15, 2021

Conversation

joviegas
Copy link
Contributor

@joviegas joviegas commented Mar 10, 2021

Description

Change STANDARD mode retry count for DynamoDB to match LEGACY same as in V1

Motivation and Context

  • DynamoDB is concerned of the lower value in STANDARD which was a lower value (2 in case of V2) and asked to set it to higher value as legacy mode.
  • In-order to keep this consistent with V1 setting the retry count to 8 same as LEGACY_RETRY_COUNT.

Testing

Screenshots (if appropriate)

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 read the README document
  • I have added tests to cover my changes
  • All new and existing tests passed
  • A short description of the change has been added to the CHANGELOG
  • 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

@codecov-io
Copy link

Codecov Report

Merging #2324 (172f30f) into master (b9bc77d) will increase coverage by 0.02%.
The diff coverage is 84.61%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #2324      +/-   ##
============================================
+ Coverage     77.64%   77.66%   +0.02%     
  Complexity      366      366              
============================================
  Files          1239     1239              
  Lines         39033    39035       +2     
  Branches       3081     3081              
============================================
+ Hits          30307    30318      +11     
+ Misses         7256     7248       -8     
+ Partials       1470     1469       -1     
Flag Coverage Δ Complexity Δ
unittests 77.66% <84.61%> (+0.02%) 0.00 <0.00> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ Complexity Δ
.../awssdk/services/dynamodb/DynamoDbRetryPolicy.java 92.30% <84.61%> (+13.14%) 0.00 <0.00> (ø)
...ty/internal/IdleConnectionCountingChannelPool.java 85.36% <0.00%> (-2.44%) 0.00% <0.00%> (ø%)
...mazon/awssdk/utils/internal/MappingSubscriber.java 89.65% <0.00%> (+6.89%) 0.00% <0.00%> (ø%)
...a/software/amazon/awssdk/core/retry/RetryMode.java 92.00% <0.00%> (+8.00%) 0.00% <0.00%> (ø%)
...k/core/pagination/async/ResponsesSubscription.java 88.00% <0.00%> (+16.00%) 0.00% <0.00%> (ø%)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b9bc77d...172f30f. Read the comment docs.

/**
* Default max retry count for DynamoDB client, when using the STANDARD retry mode.
**/
private static final int STANDARD_MAX_ERROR_RETRY = 10;
Copy link
Contributor

Choose a reason for hiding this comment

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

The legacy max error retry is 8 in v2, so the standard mode should be the same. We can probably just reuse the existing LEGACY_MAX_ERROR_RETRY

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated STANDARD_MAX_ERROR_RETRY as 8 , I would like to keep a separate Constant for Standard mode to avoid confusions

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 make it private static final int STANDARD_MAX_ERROR_RETRY = LEGACY_MAX_ERROR_RETRY? My concern is that if we ever change the legacy max error retry (although not likely), we might forget to change both places.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@@ -0,0 +1,6 @@
{
"category": "AWS SDK for Java v2",
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor, the category should be Amazon DynamoDB

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

"category": "AWS SDK for Java v2",
"contributor": "",
"type": "feature",
"description": "Change STANDARD mode retry count which was 8 for DynamoDB to match LEGACY retry count which is 10."
Copy link
Contributor

Choose a reason for hiding this comment

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

The description can be updated, as the LEGACY retry count is 8.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

81.8% 81.8% Coverage
0.0% 0.0% Duplication

@joviegas joviegas merged commit e5fb519 into aws:master Mar 15, 2021
aws-sdk-java-automation added a commit that referenced this pull request Jan 12, 2023
…66c26af80

Pull request: release <- staging/8474511c-601d-4baa-b8f5-2f366c26af80
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants