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
Add ADAPTIVE_V2 retry mode to support the legacy behavior #5123
Changes from 8 commits
da88301
7a8518d
b046843
7350a98
d77ed12
76beb59
2354277
1b4d71d
7579bc0
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -73,7 +73,7 @@ public enum RetryMode { | |
STANDARD, | ||
|
||
/** | ||
* Adaptive retry mode builds on {@code STANDARD} mode. | ||
* Adaptive retry mode builds on {@link #STANDARD} mode. | ||
* <p> | ||
* Adaptive retry mode dynamically limits the rate of AWS requests to maximize success rate. This may be at the | ||
* expense of request latency. Adaptive retry mode is not recommended when predictable latency is important. | ||
|
@@ -84,9 +84,32 @@ public enum RetryMode { | |
* the same client. When using adaptive retry mode, we recommend using a single client per resource. | ||
* | ||
* @see RetryPolicy#isFastFailRateLimiting() | ||
* @deprecated As of 2.25.xx, replaced by {@link #ADAPTIVE_V2}. The ADAPTIVE implementation has a bug that prevents it | ||
* from remembering its state across requests which is needed to correctly estimate its sending rate. Given that the | ||
* this bug has been present since its introduction and that correct version might change the traffic patterns of the SDK we | ||
* deemed too risky to fix this implementation.. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. repeated period |
||
*/ | ||
@Deprecated | ||
ADAPTIVE, | ||
|
||
/** | ||
* Adaptive V2 retry mode builds on {@link #STANDARD} mode. | ||
* <p> | ||
* Adaptive retry mode dynamically limits the rate of AWS requests to maximize success rate. This may be at the | ||
* expense of request latency. Adaptive V2 retry mode is not recommended when predictable latency is important. | ||
* <p> | ||
* Adaptive V2 mode differs from {@link #ADAPTIVE} mode in the computed delays between calls, including the fist attempt | ||
* that might be delayed if the algorithm considers that it's needed to increase the odds of a successful response. The | ||
* previous Adaptive mode is also designed to work like so but a bug present since its introduction prevents it from | ||
* preserving its internal state across requests. | ||
* <p> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. s/fist/first/ Also should "Adaptive" be capitalized?
I don't think it's necessary to specify that |
||
* <b>Warning:</b> Adaptive V2 retry mode assumes that the client is working against a single resource (e.g. one | ||
* DynamoDB Table or one S3 Bucket). If you use a single client for multiple resources, throttling or outages | ||
* associated with one resource will result in increased latency and failures when accessing all other resources via | ||
* the same client. When using adaptive retry mode, we recommend using a single client per resource. | ||
*/ | ||
ADAPTIVE_V2, | ||
|
||
; | ||
|
||
/** | ||
|
@@ -176,6 +199,8 @@ private static Optional<RetryMode> fromString(String string) { | |
return Optional.of(STANDARD); | ||
case "adaptive": | ||
return Optional.of(ADAPTIVE); | ||
case "adaptive_v2": | ||
return Optional.of(ADAPTIVE_V2); | ||
default: | ||
throw new IllegalStateException("Unsupported retry policy mode configured: " + string); | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -113,6 +113,10 @@ public static Builder builder(RetryMode retryMode) { | |
Validate.paramNotNull(retryMode, "The retry mode cannot be set as null. If you don't want to set the retry mode," | ||
+ " please use the other builder method without setting retry mode, and the default retry" | ||
+ " mode will be used."); | ||
if (retryMode == RetryMode.ADAPTIVE_V2) { | ||
throw new UnsupportedOperationException("ADAPTIVE_V2 is not supported by retry policies, use a RetryStrategy " | ||
+ "instead"); | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we move this check to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I moved this to the builder, not sure what's the reason behind it but I guess it's the same. |
||
return new BuilderImpl(retryMode); | ||
} | ||
|
||
|
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.
"Given that
the"