Retry policy refactor #190

spfink · 2017-10-03T23:18:30Z

No description provided.

millems · 2017-10-09T22:28:06Z

...rc/main/java/software/amazon/awssdk/config/defaults/DynamoDbClientConfigurationDefaults.java

- * {@link GlobalClientConfigurationDefaults}, and a lower-priority configuration than the customer-provided configuration.
- */
-@SdkInternalApi
-public class DynamoDbClientConfigurationDefaults extends ClientConfigurationDefaults {


Should we wire this up instead of deleting it?

millems · 2017-10-09T22:32:51Z

core/src/main/java/software/amazon/awssdk/retry/RetryPolicy.java

+         * @param retryCondition The retry condition
+         * @return This builder for method chaining
+         */
+        Builder retryCondition(RetryCondition retryCondition);


You have maxNumberRetries and RetryCondition as separate things. I'd expect the maximum number of retries be part of the condition.

It actually builds down to the RetryCondition but I do think it should be a separate field. Customers may want to just modify the number of retries without changing the other conditions that we retry on. This gives them an easy way to do that. If we make it required as part of the retry condition and force customers to use the AndRetryCondition its not really user friendly.

I have gone back and forth on this myself. On the one hand it bothers me from an abstraction point of view but I totally get that it's common for customers to want to tweak the number of retries without messing too much with the other conditions on which it retries. I wonder if we can make this easier by returning a builder similiar to https://github.com/aws/aws-sdk-java/blob/master/aws-java-sdk-opensdk/src/main/java/com/amazonaws/opensdk/retry/RetryPolicyBuilder.java with all the service defaults filled in and customers can tweak that as they need it. Perhaps we can expose a method on the builder that provides them the builder to modify.

Matt and I talked more about this offline yesterday.

Basically numRetries is the maximum amount of retries per a given request. Its specified at the top level to allow for easy configuration as it is a value that is common to change. It also allows a easy way to specify an overall max for a given request.

Within each condition, you are given the number of attempts and additional context so you can still have a condition that does less retries. You would just do that within the shouldRetry method.

We already kinda have a method for returning a builder with the defaults specified. If you just do RetryPolicy.builder().build() it will return a retry policy with all defaults and you then have a toBuilder().

That doesn't work with service defaults though, ideally core should define no service specific default retry (outside of connection/io related failures) and all service retryable exceptions should be wired up by the service builder. Now we may not be able to get fully there by GA but our interfaces should be forward thinking with that in mind.

We can sync offline on this further. I think we are on the same page.

Sure thing. I'm free all day today.

millems · 2017-10-09T22:36:03Z

core/src/main/java/software/amazon/awssdk/retry/SdkDefaultRetryPolicies.java

+import software.amazon.awssdk.RetryableException;
+import software.amazon.awssdk.http.HttpStatusCodes;
+
+public class SdkDefaultRetryPolicies {


Is this a public or private class? From the name, I would have expected it to have retry policies in it.

I'll rename it to something. Couldn't really figure out a good name.

This will be public as I don't see any reason not to expose these values to customers.

millems · 2017-10-09T22:36:58Z

core/src/main/java/software/amazon/awssdk/retry/SdkDefaultRetryPolicies.java

+    public static final Set<Class> RETRYABLE_EXCEPTIONS;
+
+    static {
+        THROTTLING_ERROR_CODES = new HashSet<>();


Can these be unmodifiable sets instead?

Yep. Wanted to wait until we are JDK9 compatible though to make it easy. Can add a TODO.

+1 I just know someone is going to modify them to define their own global retryable error codes....

millems · 2017-10-09T22:38:14Z

core/src/main/java/software/amazon/awssdk/retry/SdkDefaultRetryPolicies.java

+        CLOCK_SKEW_ERROR_CODES.add("InvalidSignatureException");
+        CLOCK_SKEW_ERROR_CODES.add("SignatureDoesNotMatch");
+        CLOCK_SKEW_ERROR_CODES.add("AuthFailure");
+        CLOCK_SKEW_ERROR_CODES.add("RequestInTheFuture");


These are kind of AWS specific. Do we have a plan for pulling them out of core?

+1 I know this would expand the scope quite a bit but long term I think we want to track these via traits and generate service specific retry conditions.

Generating retry policies per service is definitely something that we should do and I think this PR makes this easy. Each service would just build its own retry policy while taking the defaults for things that the service does not override.

I imagine always having some base set like this but we should generate custom policies per service based on some model and then wire those up in the default client builder. That's why I removed the DynamoDB one from here. They should be generated within the service module and wired up on generation rather than being predefined in core.

We should take another look at the proposed JSON retry schema and try and get that in. We don't have to do that now though and I don't believe we will be blocked from doing so. It might mean the retry policy changes for a service but I don't think that is not backwards compatible as it should be additive from the default policy.

millems · 2017-10-09T22:38:31Z

core/src/main/java/software/amazon/awssdk/retry/SimpleRetryPolicy.java

+import software.amazon.awssdk.retry.conditions.MaxNumberOfRetriesCondition;
+import software.amazon.awssdk.retry.conditions.RetryCondition;
+
+public class SimpleRetryPolicy implements RetryPolicy {


internal or public?

Public. Will add.

millems · 2017-10-09T22:40:04Z

core/src/main/java/software/amazon/awssdk/retry/backoff/BackoffStrategy.java

+                                                            SdkDefaultRetryPolicies.DEFAULT_NUM_RETRIES);
+
+    BackoffStrategy NONE = new FixedDelayBackoffStrategy(1);
+
    /**
     * Compute the delay before the next retry request. This strategy is only consulted when there will be a next retry.
     *
     * @param context Context about the state of the last request and information about the number of requests made.
     * @return Amount of time in milliseconds to wait before the next attempt. Must be non-negative (can be zero).
     */
    long computeDelayBeforeNextRetry(RetryPolicyContext context);


Would Duration be better than long here?

Yeah that should be fine. Will update.

millems · 2017-10-09T22:41:01Z

core/src/main/java/software/amazon/awssdk/retry/backoff/EqualJitterBackoffStrategy.java

+import java.util.Random;
+import software.amazon.awssdk.retry.RetryPolicyContext;
+
+public class EqualJitterBackoffStrategy implements BackoffStrategy {


Can we comment what these different strategies do? Customers might want to use them, but have a bit more of an explanation.

millems · 2017-10-09T22:41:10Z

core/src/main/java/software/amazon/awssdk/retry/backoff/EqualJitterBackoffStrategy.java

+    private final Random random = new Random();
+
+    public EqualJitterBackoffStrategy(final int baseDelay,
+                                      final int maxBackoffTime,


Should this have a builder. Hard to know what these ints are without reading documentation.

millems · 2017-10-09T22:43:57Z

core/src/main/java/software/amazon/awssdk/retry/conditions/MaxNumberOfRetriesCondition.java

@@ -25,7 +25,7 @@
    private final int maxNumberOfRetries;

    public MaxNumberOfRetriesCondition(int maxNumberOfRetries) {
-        this.maxNumberOfRetries = assertIsPositive(maxNumberOfRetries, "maxNumberOfRetries");
+        this.maxNumberOfRetries = maxNumberOfRetries;


Are we okay with negatives now?

Yes. We can add this back but not sure we need to. Anything <= 0 will just be 0 retries.

meh I'd rather not do that. Lots of contracts use -1 or something to mean infinite and I'd rather not open ourselves up to that confusion. We can always relax constraints in the future if there is a need.

shorea

Overall looks really good :)

shorea · 2017-10-10T22:42:46Z

core/src/main/java/software/amazon/awssdk/retry/RetryPolicyContext.java

@@ -59,8 +58,7 @@ public Object originalRequest() {
    }

    /**
-     * @return The marshalled request. See {@link Request#addHandlerContext(HandlerContextKey, Object)} for a mechanism to store
-     *     request level state across invocations of the retry policy.
+     * @return The marshalled request.


Should we include ExecutionContext in here so they have somewhere to store mutable state?

shorea · 2017-10-10T22:46:58Z

core/src/main/java/software/amazon/awssdk/retry/SdkDefaultRetryPolicies.java

+        CLOCK_SKEW_ERROR_CODES.add("InvalidSignatureException");
+        CLOCK_SKEW_ERROR_CODES.add("SignatureDoesNotMatch");
+        CLOCK_SKEW_ERROR_CODES.add("AuthFailure");
+        CLOCK_SKEW_ERROR_CODES.add("RequestInTheFuture");


+1 I know this would expand the scope quite a bit but long term I think we want to track these via traits and generate service specific retry conditions.

shorea · 2017-10-10T22:48:12Z

core/src/main/java/software/amazon/awssdk/retry/SdkDefaultRetryPolicies.java

+    public static final Set<Class> RETRYABLE_EXCEPTIONS;
+
+    static {
+        THROTTLING_ERROR_CODES = new HashSet<>();


+1 I just know someone is going to modify them to define their own global retryable error codes....

shorea · 2017-10-10T22:48:41Z

core/src/main/java/software/amazon/awssdk/retry/SimpleRetryPolicy.java

+import software.amazon.awssdk.retry.conditions.MaxNumberOfRetriesCondition;
+import software.amazon.awssdk.retry.conditions.RetryCondition;
+
+public class SimpleRetryPolicy implements RetryPolicy {


shorea · 2017-10-10T22:50:11Z

core/src/main/java/software/amazon/awssdk/retry/backoff/EqualJitterBackoffStrategy.java

+    private final Random random = new Random();
+
+    public EqualJitterBackoffStrategy(final int baseDelay,
+                                      final int maxBackoffTime,


Should this have a builder. Hard to know what these ints are without reading documentation.

shorea · 2017-10-10T22:52:01Z

core/src/main/java/software/amazon/awssdk/retry/conditions/MaxNumberOfRetriesCondition.java

@@ -25,7 +25,7 @@
    private final int maxNumberOfRetries;

    public MaxNumberOfRetriesCondition(int maxNumberOfRetries) {
-        this.maxNumberOfRetries = assertIsPositive(maxNumberOfRetries, "maxNumberOfRetries");
+        this.maxNumberOfRetries = maxNumberOfRetries;


meh I'd rather not do that. Lots of contracts use -1 or something to mean infinite and I'd rather not open ourselves up to that confusion. We can always relax constraints in the future if there is a need.

shorea · 2017-10-10T22:52:59Z

core/src/main/java/software/amazon/awssdk/retry/conditions/RetryOnErrorCodeCondition.java

+
+    private final Set<String> retryableErrorCodes;
+
+    public RetryOnErrorCodeCondition(Set<String> retryableErrorCodes) {


If this is public perhaps a var arg for convenience.

shorea · 2017-10-10T22:55:41Z

core/src/test/java/software/amazon/awssdk/retry/RetryPolicyContexts.java

+/**
+ * Precanned instances of {@link RetryPolicyContext} and factory methods for creating contexts.
+ */
+public class RetryPolicyContexts {


Feels internal

This is in the test directory.

shorea · 2017-10-10T22:55:47Z

core/src/test/java/software/amazon/awssdk/retry/RetryPolicyContexts.java

+     */
+    public static final RetryPolicyContext EMPTY = RetryPolicyContext.builder().build();
+
+    public static final RetryPolicyContext LEGACY = RetryPolicyContexts.fromLegacy(


What's this for?

shorea · 2017-10-10T23:02:18Z

core/src/main/java/software/amazon/awssdk/retry/RetryPolicy.java

+         * @param retryCondition The retry condition
+         * @return This builder for method chaining
+         */
+        Builder retryCondition(RetryCondition retryCondition);


I have gone back and forth on this myself. On the one hand it bothers me from an abstraction point of view but I totally get that it's common for customers to want to tweak the number of retries without messing too much with the other conditions on which it retries. I wonder if we can make this easier by returning a builder similiar to https://github.com/aws/aws-sdk-java/blob/master/aws-java-sdk-opensdk/src/main/java/com/amazonaws/opensdk/retry/RetryPolicyBuilder.java with all the service defaults filled in and customers can tweak that as they need it. Perhaps we can expose a method on the builder that provides them the builder to modify.

spfink · 2017-10-11T20:53:54Z

Updated per feedback.

Only thing that I haven't done yet from the feedback is the builders on the backoff strategies. Typically you would see the builder on the interface but that doesn't work here. I guess I can add one specific to each strategy...

@shorea

millems · 2017-10-17T23:04:10Z

core/src/main/java/software/amazon/awssdk/retry/RetryHandler.java

+import software.amazon.awssdk.http.SdkHttpFullRequest;
+import software.amazon.awssdk.util.CapacityManager;
+
+public class RetryHandler {


Annotate internal?

I'd lean more towards public. The logic here shouldn't ever change as it mostly delegates out to the retry policy. If customers want to use this to determine whether or not a given request should be retried I think that's fine.

How would the customer use this?

millems · 2017-10-17T23:05:34Z

core/src/main/java/software/amazon/awssdk/retry/RetryHandler.java

+    public RetryHandler(RetryPolicy retryPolicy,
+                        CapacityManager retryCapacity) {
+        this.retryPolicy = retryPolicy;
+        this.retryCapacity = retryCapacity;


Would it be possible for the capacity manager to be part of the retry condition as well? Essentially, you mix-in throttling, the same way we're mixing in the max-retry?

We'd probably need the extra hooks to release the capacity, but it seems weird that an SDK customer can't implement retry throttling using our condition interface.

It could but I'm not sure it's worth the extra complexity. This is always the first condition that should be checked as you'd be wasting time if it wasn't. We'd also have to help a lot through our builder as the customer shouldn't need to know which exceptions are throttling if they want to override the defaults.

The complexity argument is legit - I'm not sure how much extra it would be. Would it be more than 1 method?

The wasting-time argument I don't follow so much. What's another few nanoseconds added to the case that the throttle condition will fail?

In terms of the customer not knowing about which exceptions are throttling exceptions, the same thing applies to the customer not knowing which exceptions are retryable exceptions. If that's a problem here, it's a problem there as well.

In terms of the customer not knowing about which exceptions are throttling exceptions, the same thing applies to the customer not knowing which exceptions are retryable exceptions.

I don't think that's equivalent. In the retryable exception case, there is one dimension, exceptions. They either take ours or set their own.

In a condition dealing with throttling they are more likely interested in modifying the number of throttled retries and the cost while using our throttled retries. In that case they need to know what our throttled exceptions are to be able to produce a complete retry condition.

The wasting-time argument I don't follow so much. What's another few nanoseconds added to the case that the throttle condition will fail?

Ultimately I guess it could apply in any situation where you have a set of conditions but I would typically see a priority to retry conditions in which throttling is the highest. Evaluating any condition before it would be a waste of time.

It just feels like our retry policy interface isn't powerful enough to express what we need, so we're working around it to add extra features. It feels like LESS complexity to have it under just one abstraction. I suppose it's always something we could add later, assuming we don't expose the concept of capacity manager outside of the core.

Would a reasonable compromise be to hide all traces of it from our public interfaces?

I'm on the fence. I see where you're coming from Matt and I agree it would make sense in the retry condition. We would need some way to notify the retry condition that a request has succeeded though and I feel like that would pollute the interface a bit (no longer a simple predicate). Not exactly sure how many customers would actually want to implement their own throttling so I'd say lean towards leaving it out for now.

millems · 2017-10-17T23:08:13Z

core/src/main/java/software/amazon/awssdk/retry/RetryPolicy.java

-    public BackoffStrategy getBackoffStrategy() {
-        return backoffStrategy;
+@SdkPublicApi
+public interface RetryPolicy extends BackoffStrategy, RetryCondition, ToCopyableBuilder<RetryPolicy.Builder, RetryPolicy> {


It seems weird that a retry policy is-a backoff strategy and a retry condition. That seems like a has-a relationship. Any particular reason to extend the interfaces here?

I like it extending the interfaces, otherwise we have to write awkward code in the runtime to get the individual strategies. Also let's you implement the condition and backoff strategy easily in one class.

We could always implement both in the same class if we wanted by making the subclass implement the condition and backoff strategy interfaces. What's the awkward code we'd have to write in the runtime?

Here's my complaint about the way it currently works:

Given:

class MyRetryPolicy implements RetryPolicy { ... }

We can create a retry policy that delegates to other retry policies, which to me is a bit odd:

RetryPolicy myRetryStrategy = new MyRetryPolicy(); RetryPolicy letsGetMeta = RetryPolicy.builder() .retryCondition(myRetryStrategy) .backoffStrategy(myRetryStrategy) .build();

Even if that's considered a "feature", it implies a retry policy must always delegate to a retry condition and backoff strategy, but it also IS a retry condition and backoff strategy? That has really weird implications.

For example, I can't even wrap my head around what this code does:

RetryPolicy whatDoesThisEvenDoPolicy = myRetryStrategy.toBuilder() .retryCondition(myRetryStrategy) .backoffStrategy(myRetryStrategy) .build();

MyRetryPolicy must implement toBuilder(), because it extends ToCopyableBuilder, but it also must HAVE a condition and a backoff strategy because it can be converted to and from a builder that HAS one? So RetryPolicy always delegates to a RetryCondition and a BackoffStrategy?

My only guess at how to implement by own retry policy is to either throw a UnsupportedOperationException from toBuilder (meaning we've given up on the L in SOLID) or just make my retry policy always delegate to a condition and backoff strategy, at which point every RetryPolicy is a decorator (meaning we've given up on the I in SOLID). We will have already given up on the S in SOLID by having the class implement both interfaces.

So RetryPolicy always delegates to a RetryCondition and a BackoffStrategy?

Not necesarily. The simple implementation takes a condition and backoff strategy separately and delegates to them but you could just as easily implement it all in one class and combine the logic. I think the weirdness here stems from forcing the builder on the retry policy interface, do we even need a builder? Would a factory method suffice for the common case of combining condition and strategy?

If we don't make it ToCopyableBuilder, it's better, but we're still breaking the S and I in SOLID and I'm not sure what benefit it's bringing us.

retryPolicy.retryCondition().shouldRetry(context) is pretty much identical to retryPolicy.shouldRetry(context).

How does it break SOLID?

S: We're defining an interface RetryPolicy with two potential responsibilities: defining a condition and defining a backoff strategy.
I: We have an interface RetryPolicy that has two methods that can (and already are in this case) be separated into two separate interfaces.

I don't really agree with all that. In certainly can be broken up into two separate concepts but from a customer's perspective its one logical thing, how the client handles retry situations. Does any aggregate interface break SOLID? I wouldn't think so.

millems · 2017-10-17T23:11:14Z

core/src/main/java/software/amazon/awssdk/retry/RetryPolicyContext.java

@@ -121,6 +131,11 @@ public Builder exception(SdkBaseException exception) {
            return this;
        }

+        public Builder executionContext(ExecutionContext executionContext) {


ExecutionContext feels like a big, internal thing to expose to a public interface like the retry policy. Would it be better to expose the ExecutionAttributes, which is what we currently expose to the interceptors for the same reason?

+1, sorry that was my suggestion and got the names mixed up.

millems · 2017-10-17T23:12:01Z

core/src/main/java/software/amazon/awssdk/retry/SdkDefaultRetrySettings.java

+import software.amazon.awssdk.http.HttpStatusCodes;
+
+// TODO: Add some JDK9 sweetness to this class when ready
+public class SdkDefaultRetrySettings {


Annotate as internal or public? It feels like something we might want to change, so I'd vote for internal...

I'm fine with internal in that we can change it but I don't like the full description of internal for this. I'm fine with customers using / depending on this to get at what we are retrying as long as they understand it will change.

Internal for now and make it public in the future if we need to? Less surface area to worry about...

millems · 2017-10-17T23:14:44Z

core/src/main/java/software/amazon/awssdk/retry/backoff/BackoffStrategy.java


-/**
- * Super interface for {@link RetryPolicy} that defines a strategy for backing off between retries.
- */
 public interface BackoffStrategy {


Annotate as public?

millems · 2017-10-17T23:17:11Z

core/src/main/java/software/amazon/awssdk/retry/conditions/MaxNumberOfRetriesCondition.java


-import static software.amazon.awssdk.util.ValidationUtils.assertIsPositive;
+import static software.amazon.awssdk.utils.NumericUtils.assertIsPositive;


Should this be in Validate with our other validation methods?

Maybe? We had this NumericUtils class that already had a few validations for number types. Makes sense to me that NumericUtils should be used here but not a big deal to me either way.

Validate already has some stuff for different types. We should decide on one place. Should we move string validation to StringUtils?

millems · 2017-10-17T23:17:30Z

core/src/main/java/software/amazon/awssdk/retry/conditions/OrRetryCondition.java


 import java.util.ArrayList;
 import java.util.Collections;
 import java.util.List;
+import software.amazon.awssdk.retry.RetryPolicyContext;


Sorry, it was just an unchanged file with an added import.

millems · 2017-10-17T23:17:52Z

core/src/main/java/software/amazon/awssdk/retry/conditions/RetryOnErrorCodeCondition.java

+import software.amazon.awssdk.AmazonServiceException;
+import software.amazon.awssdk.retry.RetryPolicyContext;
+
+public class RetryOnErrorCodeCondition implements RetryCondition {


Internal or public?

millems · 2017-10-17T23:20:12Z

core/src/main/java/software/amazon/awssdk/retry/conditions/RetryOnErrorCodeCondition.java

+    // TODO: Switch to varargs and Set.of()
+    public RetryOnErrorCodeCondition(Set<String> retryableErrorCodes) {
+        this.retryableErrorCodes = new HashSet<>(retryableErrorCodes);
+    }


With Java 9 will we need varargs? Set.of(...) isn't much harder, and it makes it clear that there's no duplicates allowed. I don't have a strong opinion, but there are a few other conditions that take Sets in this review, so we should consider moving this TODO to them as well.

A little less burden on the customer. Fine either way though.

shorea · 2017-10-18T15:09:04Z

core/src/main/java/software/amazon/awssdk/retry/RetryHandler.java

+        // Do not use retry capacity for throttling exceptions
+        if (!RetryUtils.isThrottlingException(exception)) {
+            // See if we have enough available retry capacity to be able to execute this retry attempt.
+            if (!retryCapacity.acquire(SdkDefaultRetrySettings.THROTTLED_RETRY_COST)) {


This confused me at first. I was thinking the cost was specific to throttling exceptions but we don't acquire for throttling exceptions. I see now it's just the default cost for "retry throttling" but the naming really confused me. How about RETRY_THROTTLING_COST?

shorea · 2017-10-18T15:18:05Z

core/src/main/java/software/amazon/awssdk/retry/SdkDefaultRetrySettings.java

+
+    public static final Duration BASE_DELAY = Duration.ofMillis(100);
+
+    public static final Duration MAX_BACKOFF_IN_MILLIS = Duration.ofMillis(20_000);


IN_MILLIS doesn't make sense in the name now.

shorea · 2017-10-18T15:19:02Z

core/src/main/java/software/amazon/awssdk/retry/SdkDefaultRetrySettings.java

+    public static final int THROTTLED_RETRY_COST = 5;
+
+    /**
+     * When throttled retries are enabled, this is the total number of subsequent failed retries


This comment seems misleading. If each retry consumes 5 cost then there would be only 20 retries before capacity is drained right?

Capacity is THROTTLED_RETRIES * THROTTLED_RETRY_COST

Ahhh makes sense.

shorea · 2017-10-18T15:23:56Z

core/src/main/java/software/amazon/awssdk/retry/conditions/RetryCondition.java

 public interface RetryCondition {

+    RetryCondition DEFAULT = new OrRetryCondition(new RetryOnStatusCodeCondition(SdkDefaultRetrySettings.RETRYABLE_STATUS_CODES),


Thoughts on making these default methods like Predicate does?

https://docs.oracle.com/javase/8/docs/api/java/util/function/Predicate.html#or-java.util.function.Predicate-

Fine with adding and/or default methods for any given two retry conditions but not sure it makes sense in the line you are highlighting here.

Think it makes sense to do one condition and another condition. Don't think it makes sense to do a condition and a list of others. You can do retryCondition and RetryCondition and RetryCondition but then you are needlessly adding layers.

shorea · 2017-10-18T15:26:17Z

utils/src/main/java/software/amazon/awssdk/utils/NumericUtils.java

+        return num;
+    }
+
+    public static int assertIsPositive(int num, String fieldName, boolean zeroIsPositive) {


Can we just have another method name assertIsNonNegative? Not a fan of flags to change method behavior.

millems · 2017-10-23T18:49:15Z

core/src/main/java/software/amazon/awssdk/core/retry/RetryPolicy.java

-@Immutable
-public final class RetryPolicy {
+@SdkPublicApi
+public final class RetryPolicy implements ToCopyableBuilder<RetryPolicy.Builder, RetryPolicy> {


millems · 2017-10-23T18:53:11Z

core/src/main/java/software/amazon/awssdk/core/retry/RetryPolicy.java

+
+    @Override
+    public Builder toBuilder() {
+        return builder().numRetries(numRetries).retryCondition(retryCondition).backoffStrategy(backoffStrategy);


Should this be the original retry condition?

// 3 Retries RetryPolicy foo = RetryPolicy.builder().maxRetries(3).build(); // Still 3 retries? RetryPolicy bar = foo.toBuilder().maxRetries(10).build();

millems · 2017-10-23T18:55:33Z

core/src/main/java/software/amazon/awssdk/core/retry/RetryPolicy.java

-        long delayBeforeNextRetry(AmazonWebServiceRequest originalRequest,
-                                  AmazonClientException exception,
-                                  int retriesAttempted);
+        private Integer numRetries;


Nitpick: Should these start off assigned with the default values instead of binding them in the constructor?

Right now:

RetryPolicy.Builder foo = RetryPolicy.builder(); // null foo.numRetries(); // 3 foo.build().toBuilder().numRetries();

millems · 2017-10-23T18:55:59Z

core/src/main/java/software/amazon/awssdk/core/retry/RetryPolicyContext.java

+     */
+    public ExecutionContext executionContext() {
+        return this.executionContext;
+    }


Execution attributes? Execution context is pretty internal.

Weird, I swear I had already done this. Must have gotten lost somewhere.

millems · 2017-10-23T18:56:48Z

core/src/main/java/software/amazon/awssdk/core/retry/RetryPolicyContext.java

    private final int retriesAttempted;
    private final Integer httpStatusCode;

    private RetryPolicyContext(Object originalRequest,
                               SdkHttpFullRequest request,
                               SdkBaseException exception,
+                               ExecutionContext executionContext,
                               int retriesAttempted,


Should RetryPolicyContext be final?

millems · 2017-10-23T19:40:39Z

core/src/main/java/software/amazon/awssdk/core/retry/backoff/EqualJitterBackoffStrategy.java

+
+package software.amazon.awssdk.core.retry.backoff;
+
+import static software.amazon.awssdk.utils.NumericUtils.assertIsNotNegative;


Where did we land on the location for this? Validate already has some for numbers, so we should unify the location. I vote for Validate as all of our validation stuff.

millems · 2017-10-23T19:41:22Z

core/src/main/java/software/amazon/awssdk/core/retry/backoff/FullJitterBackoffStrategy.java

+
+    public FullJitterBackoffStrategy(final Duration baseDelay,
+                                     final Duration maxBackoffTime,
+                                     final int numRetries) {


Same comment here. Builder?

millems · 2017-10-23T19:42:08Z

core/src/main/java/software/amazon/awssdk/core/retry/conditions/RetryCondition.java

+import software.amazon.awssdk.core.retry.RetryPolicyContext;
+import software.amazon.awssdk.core.retry.SdkDefaultRetrySettings;
+
+@SdkPublicApi
 public interface RetryCondition {


FunctionalInterface

millems · 2017-10-23T19:43:22Z

core/src/main/java/software/amazon/awssdk/core/retry/conditions/RetryOnExceptionsCondition.java

 public class RetryOnExceptionsCondition implements RetryCondition {

-    private final List<Class<? extends Exception>> exceptionsToRetryOn;
+    private final Set<Class> exceptionsToRetryOn;


Class<?>? Raw use of generic types is gross...

millems · 2017-10-23T19:45:20Z

...dtrail/src/it/java/software/amazon/awssdk/services/cloudtrail/CloudTrailIntegrationTest.java

@@ -24,6 +24,7 @@
 import java.io.IOException;
 import java.util.Iterator;
 import org.apache.commons.io.IOUtils;
+import org.apache.log4j.BasicConfigurator;


Is this used here? There was no content changes.

varunnvs92 · 2017-11-09T00:11:14Z

core/src/main/java/software/amazon/awssdk/core/retry/SdkDefaultRetrySettings.java

+
+    public static final Duration MAX_BACKOFF = Duration.ofMillis(20_000);
+
+    public static final Integer DEFAULT_NUM_RETRIES = 3;


DEFAULT_RETRY_COUNT or DEFAULT_MAX_RETRIES?

Fine with DEFAULT_MAX_RETRIES

varunnvs92 · 2017-11-09T00:18:39Z

core/src/main/java/software/amazon/awssdk/core/retry/backoff/BackoffStrategy.java

+
+    default int calculateExponentialDelay(int retriesAttempted, Duration baseDelay, Duration maxBackoffTime, int maxRetries) {
+        int retries = Math.min(retriesAttempted, maxRetries);
+        return (int) Math.min((1L << retries) * baseDelay.toMillis(), maxBackoffTime.toMillis());


I don't remember how this conversion works. If the computed value can always be fit in an int, we can use Math.toIntExact(long).

varunnvs92 · 2017-11-09T02:42:59Z

core/src/test/java/software/amazon/awssdk/core/config/ImmutableClientConfigurationTest.java

-    };
+    private static final RetryPolicy RETRY_POLICY = RetryPolicy.builder()
+                                                               .retryCondition((c) -> false)
+                                                               .backoffStrategy((c) -> Duration.ZERO)


Do we need the extra paranthesis around c? Also better naming for c?

Nope and sure

varunnvs92 · 2017-11-09T02:51:31Z

utils/src/main/java/software/amazon/awssdk/utils/NumericUtils.java

+    public static int assertIsNotNegative(int num, String fieldName) {
+
+        if (num < 0) {
+            throw new IllegalArgumentException(String.format("%s must be positive", fieldName));


must not be negative?

varunnvs92 · 2017-11-09T03:01:02Z

utils/src/main/java/software/amazon/awssdk/utils/NumericUtils.java

+        }
+
+        if (duration.isNegative()) {
+            throw new IllegalArgumentException(String.format("%s must be positive", fieldName));


must not be negative

millems reviewed Oct 9, 2017

View reviewed changes

shorea reviewed Oct 10, 2017

View reviewed changes

millems reviewed Oct 17, 2017

View reviewed changes

shorea reviewed Oct 18, 2017

View reviewed changes

spfink force-pushed the finks/retry-policy branch from d7f2b85 to dd3c878 Compare October 20, 2017 17:30

millems reviewed Oct 23, 2017

View reviewed changes

spfink force-pushed the finks/retry-policy branch from dd3c878 to 0979960 Compare November 3, 2017 17:04

varunnvs92 reviewed Nov 9, 2017

View reviewed changes

spfink force-pushed the finks/retry-policy branch from 0979960 to 17fb95e Compare November 10, 2017 21:17

varunnvs92 approved these changes Nov 13, 2017

View reviewed changes

Refactoring retry policy

c9679db

spfink force-pushed the finks/retry-policy branch from 17fb95e to c9679db Compare November 13, 2017 23:11

Merge branch 'master' into finks/retry-policy

de82cae

spfink merged commit 65b6517 into master Nov 13, 2017

spfink deleted the finks/retry-policy branch December 6, 2017 18:00


		private final Set<String> retryableErrorCodes;

		public RetryOnErrorCodeCondition(Set<String> retryableErrorCodes) {


		import static software.amazon.awssdk.util.ValidationUtils.assertIsPositive;
		import static software.amazon.awssdk.utils.NumericUtils.assertIsPositive;


		public static final Duration BASE_DELAY = Duration.ofMillis(100);

		public static final Duration MAX_BACKOFF_IN_MILLIS = Duration.ofMillis(20_000);

		public interface RetryCondition {

		RetryCondition DEFAULT = new OrRetryCondition(new RetryOnStatusCodeCondition(SdkDefaultRetrySettings.RETRYABLE_STATUS_CODES),


		package software.amazon.awssdk.core.retry.backoff;

		import static software.amazon.awssdk.utils.NumericUtils.assertIsNotNegative;


		public static final Duration MAX_BACKOFF = Duration.ofMillis(20_000);

		public static final Integer DEFAULT_NUM_RETRIES = 3;

Retry policy refactor #190

Retry policy refactor #190

Conversation

spfink commented Oct 3, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spfink Oct 11, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spfink Oct 10, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shorea left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spfink commented Oct 11, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

millems Oct 18, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spfink Oct 11, 2017 •

edited

Loading

spfink Oct 10, 2017 •

edited

Loading

millems Oct 18, 2017 •

edited

Loading