Add Retries and Retry Policy #307
Conversation
fuatbasik
had a problem deploying
to
integration-tests
GitHub Actions
Failure
fuatbasik
had a problem deploying
to
integration-tests
GitHub Actions
Failure
fuatbasik
temporarily deployed
to
integration-tests
GitHub Actions
Inactive
| this.aggregatingMetrics = aggregatingMetrics; | ||
| this.indexCache = indexCache; | ||
| generateSourceAndData(); | ||
| this.configuration = configuration; |
There was a problem hiding this comment.
is this variable ever used?
think line 111 and 109-110 use class variable vs constructor variable in different ways maybe just remove this config?
...ream/src/main/java/software/amazon/s3/analyticsaccelerator/S3SeekableInputStreamFactory.java
Show resolved
Hide resolved
|
|
||
| /** | ||
| * Executes a supplier with retry logic according to the configured retry policy. | ||
| * |
There was a problem hiding this comment.
these 2 lines are basically saying the same thing about using the retry policy maybe just merge them?
|
|
||
| /** Functional interface for operations that can throw IOException. */ | ||
| @FunctionalInterface | ||
| interface IORunnable { |
There was a problem hiding this comment.
think this interface and the interface below can be handy in the future should we move them to their own classes.
| * @param configuration the physical IO configuration containing retry settings | ||
| * @throws NullPointerException if configuration is null | ||
| */ | ||
| public SeekableInputStreamRetryExecutor(PhysicalIOConfiguration configuration) { |
There was a problem hiding this comment.
I don't think this should take PhysicalIOConfiguration, I can see us wanting to re-use this logic elsewhere and we might not necessarily want to use PhysicalIOConfiguration there. Can't we split this out to it's own thing? even if it just includes the 2 settings we have for now.
...ream/src/test/java/software/amazon/s3/analyticsaccelerator/retry/RetryPolicyBuilderTest.java
Show resolved
Hide resolved
| } | ||
|
|
||
| @Test | ||
| void testConfigurationConstructor() { |
There was a problem hiding this comment.
same thing about using parameterized test to just cover all the constructor tests.
| import java.io.IOException; | ||
|
|
||
| /** | ||
| * Interface for executing operations with retry logic. We do not expect consumers of AAL to |
There was a problem hiding this comment.
Didn't the one design include consumers having access to this?
fuatbasik
had a problem deploying
to
integration-tests
GitHub Actions
Failure
fuatbasik
had a problem deploying
to
integration-tests
GitHub Actions
Failure
fuatbasik
had a problem deploying
to
integration-tests
GitHub Actions
Failure
| * | ||
| * @throws RuntimeException if all retries fails and an error occurs | ||
| */ | ||
| private void generateSourceAndData() { |
There was a problem hiding this comment.
With this PR we are changing the retry behaviour, intentionally. Today if generateSourceAndData timeouts (mostly this is relevant to StreamUtils.toByteArray()), we will retry, even if there is no call to 'read' function of that block. With this change we will only retry if there is a call to 'read' function. This could be when we prefetch a block but never read it, but it does make sense not to retry them anyways.
fuatbasik
had a problem deploying
to
integration-tests
GitHub Actions
Failure
fuatbasik
had a problem deploying
to
integration-tests
GitHub Actions
Failure
fuatbasik
temporarily deployed
to
integration-tests
GitHub Actions
Inactive
|
LGTM TY |
2 participants
Description of change
This is the first PR of a series of 2, that is adding RetryPolicy (with Failsafe as a dependency) and applying these retries with the current defaults. In the next PR, I will add the logic to pass RetryPolicy from library user to block, through Open Stream Information
Relevant issues
NO.
Does this contribution introduce any breaking changes to the existing APIs or behaviors?
No.
Does this contribution introduce any new public APIs or behaviors?
Yes, it introduces a new capability to change/add retries on top of SDK retries.
How was the contribution tested?
Added new unit tests, confirmed existing GrayFailureTests are passing.
Does this contribution need a changelog entry?
Yes. I will update the ChangeLog and Readme in the next PR.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license and I agree to the terms of the Developer Certificate of Origin (DCO).