Add default retry predicates to user provided ones

[L-Applin]

Add default retry predicates to retry strategies on top of user provided predicates.

Motivation and Context

Currently, the SDK will only add default retry predicates when user do not specified any, effectively overriding defaults whenever a single other predicate is defined by user.

Modifications

• Added an opt out configuration: The boolean useClientDefaults() method has been added to the RetryStrategy and builder interface to check whether this strategy should add the default predicates or not.
• Keep track of which defaults have been added to prevent adding them multiple time at client build time.
• Moved DefaultAwsClientBuilderTest to junit 5

Testing

Added unit tests

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)

[millems]

Another option for design might be to model "defaults" explicitly, and make this addDefaults(RetryStrategyDefaults), so that we enforce that defaults are always added before they are marked as added. We may not need shouldAddDefaults at that point, because the "should I add this?" can be done within the retry strategy and not rely on the caller to make the proper decision.

The drawback for that is that we can't avoid the round-trip to a builder if there are no defaults to be added. Not sure what the cost of that is.

[L-Applin]

right I see what you mean. The caller would just attempt to add the default every time, which would just be a noop within the retry strategy if those defaults were already added. It's not a big change, I can see the benefit of doing it this way.

[sonarqubecloud]

Quality Gate passed

Issues
15 New issues
0 Accepted issues

Measures
0 Security Hotspots
95.1% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[millems]

Is this method needed anymore?

[L-Applin]

It is if we want to avoid having the core retry classes reference BaseRetryStrategy like you proposed eariler, which I think is the better approach. For example, AwsRetryStrategy needs to be able to mark defaults as being added when instantiated directly, to not duplicate them when the default client builder tries to add them later at client build time.

[millems]

Got it, I imagined AwsRetryStrategy using addDefaults on instantiation to ensure they're marked as already included. I can see why that's weird now. For some reason I also imagined addDefaults being on the builder, not on the retry strategy itself.

[millems]

If we implement equals() and hashCode(), we don't even need name().

[L-Applin]

mmmhh, Implementations of this interface have no fields, there is nothing to compare to for equals and hash code. We could make the implementations not anonymous and used their name, is that what you're proposing? For SdkDefaultRetryStrategy it would be something like:

    private static final class SdkRetryStrategyDefaults implements RetryStrategyDefaults {
        @Override
        public void applyDefaults(RetryStrategy.Builder<?, ?> builder) {
            configureStrategy(builder);
            markDefaultsAdded(builder);
        }

        @Override
        public boolean equals(Object o) {
            if (this == o) {
                return true;
            }
            return o != null && getClass() == o.getClass();
        }

        @Override
        public int hashCode() {
            return getClass().getName().hashCode();
        }
    }

[millems]

That or just use identity equality and singletons, but I suppose that names are less opaque.

[L-Applin]

I think I slightly prefer singleton and identity, lets got with that. We wouldn't need to implement hashCode and equals then.

[L-Applin]

changed my mind, lets just keep name(), it's simpler

[sugmanue]

You need to copy the set here instead of using the same as the builder, or, better yet, copy and pass it trough Collections.unmodifiableSet. Otherwise you end up sharing state that you shouldn't. For instance:

RetryStategy.Builder builder = standardRetryStrategy().toBuilder();
addSomeFooDefaults(builder);
RetryStategy strategyA = builder.build();
addSomeBarDefaults(builder);
RetryStategy strategyB = builder.build();

After this, both, strategyA and strategyB think they have both, Foo and Bar defaults whereas only strategyA does.