Refactoring retry logic out to separate class #177
Conversation
|
Decided this was a good middle ground towards making the S3 retries changes well integrated and not refactoring too much of the retry policy stuff yet. Will include S3 retry logic in other PR. |
| import org.slf4j.LoggerFactory; | ||
| import software.amazon.awssdk.http.HttpResponse; | ||
|
|
||
| public class ClockSkewUtil { |
There was a problem hiding this comment.
Needs final and private ctor
There was a problem hiding this comment.
To be REALLY petty, *Utils is what we've been using elsewhere. I don't mind Util, but better to be consistent.
| .map(DateUtils::parseRfc1123Date) | ||
| .orElseThrow(() -> new RuntimeException( | ||
| "Unable to parse clock skew offset from response. Server Date header missing")); | ||
| long diff = System.currentTimeMillis() - serverDate.toEpochMilli(); |
There was a problem hiding this comment.
Would be cleaner to useDuration#between() https://docs.oracle.com/javase/8/docs/api/java/time/Duration.html#between-java.time.temporal.Temporal-java.time.temporal.Temporal-
| import software.amazon.awssdk.retry.v2.RetryPolicyContext; | ||
| import software.amazon.awssdk.util.CapacityManager; | ||
|
|
||
| public class RetryHandler { |
There was a problem hiding this comment.
Is this class meant to be shared between threads?
There was a problem hiding this comment.
Not really. This class is going to be instantiated on each request / retry and its state is created newly based on the data at hand.
|
I was thinking that we could have a single RetryableStage that returns a completable future and has a pluggable re-submit layer. so something like this Sync // Uses a blocking Thread.sleep with delay then executes the request. Returns an already
// fulfulled CompletableFuture
new RetryableStage(new ThreadSleepStrategy())
// Would probably need something in the next stage to join on the future for syncAsync // Submits to a ScheduledExecutor with the delay amount
new RetryableStage(new ScheduledExecutorStrategy()) |
|
@shorea I'll look into that. I think I'd still pull out the core retry logic into a separate class but handling exceptions and the sleep can probably all be combined into one stage. |
| * @return long value of how long to wait | ||
| */ | ||
| public long computeDelayBeforeNextRetry() { | ||
| long delay = retryPolicy.computeDelayBeforeNextRetry(retryPolicyContext); |
There was a problem hiding this comment.
we don't need local variable 'delay' here. why not directly assign to lastBackoffDelay and return it?
spfink
force-pushed
the
finks/retry
branch
2 times, most recently
from
025f882 to
4a7265c
Compare
All tests (unit and integration) pass and provide good coverage over this logic.