-
Notifications
You must be signed in to change notification settings - Fork 911
Fix overflow bugs in backoff strategy. #1673
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
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1673 +/- ##
============================================
- Coverage 74.48% 74.48% -0.01%
Complexity 909 909
============================================
Files 960 960
Lines 30376 30316 -60
Branches 2390 2371 -19
============================================
- Hits 22626 22580 -46
+ Misses 6615 6598 -17
- Partials 1135 1138 +3
Continue to review full report at Codecov.
|
780e563
to
0b1606c
Compare
core/sdk-core/src/main/java/software/amazon/awssdk/core/retry/backoff/BackoffStrategy.java
Show resolved
Hide resolved
core/sdk-core/src/main/java/software/amazon/awssdk/core/retry/backoff/BackoffStrategy.java
Outdated
Show resolved
Hide resolved
.../src/test/java/software/amazon/awssdk/core/retry/backoff/EqualJitterBackoffStrategyTest.java
Outdated
Show resolved
Hide resolved
3e89194
to
0e723e5
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note in the future if you update branch through GitHub you don't need to rebase the commit locally. Please make sure the other files are not actually being modified before merging
0e723e5
to
cc88c19
Compare
Kudos, SonarCloud Quality Gate passed!
|
…03cf95c22 Pull request: release <- staging/78fb9957-186a-446f-b932-be403cf95c22
Fixed issue #1453 based on the proposed fix code provided by OlegZhukov, set up ceils for
baseDelay
,maxBackoffTime
andretriesAttempted
to prevent potential overflow problems.Description
Added three ceils for
baseDelay
,maxBackoffTime
andretriesAttempted
respectively. Restricted theexponentialDelayMillis
from overflow via applying these ceils in calculation. Added unit tests forEqualJitterBackoffStrategy.java
based on proposed test cases provided by OlegZhukov.In addition, to make it easier to compare different Duration objects, two functions
min
andmax
are added inNumericUtils.java
, corresponding tests are added too.Motivation and Context
Before this modification, the exponentialDelay has a risk of overflow due to its type as Integer. Besides,
baseDelay
andmaxBackoffTime
are not checked if they will meet overflow problems while converting into Millis. So to fix these potential problems, we need to add some restrictions on these variables, as well as the calculation.The open issue corresponding to this pull request is: #1453 .
Testing
Two new test classes are added in this pull request. One is the test class for
EqualJitterBackoffStrategyTest.java
, which includes unit tests for different values ofbaseDelay
andmaxBackoffTime
. The other is forNumericUtils.java
, testing whether the compare functions work well in different situations.Screenshots (if appropriate)
Types of changes
Checklist
mvn install
succeedsLicense