Rethrow particular not-retryable exceptions

[ttulka]

Although rethrow helps a lot I have use-cases where I need exception handling to be even more flexible.

• Rethrow particular not-retryable exceptions without recovering and wrapping them: rethrowExceptions

Example:
I want to retry server exceptions except for an authorization exception as this should be sent immediately and directly to the caller.

@Retry(include={ServerException.class}, rethrowExceptions={AuthorizationException.class})
public String makeSomeCall() { … }

@Recover
private String recoverThatCall(Throwable t) {
    // not for AuthorizationException
}

[artembilan]

This is OK with me.

@garyrussell ?

[garyrussell]

I don't like mixing "retryability" between the policy and the template; this should be in the policy.

However, the SimpleRetryPolicy should already handle this; just classify the AuthorizationException as not retryable and the ServerException as retryable. i.e. add the ServerException to includes and AuthorizationException to excludes.

The classifier checks for the actual class before traversing up the supers to find their classifications.

[garyrussell]

spring-retry/src/main/java/org/springframework/retry/annotation/AnnotationAwareRetryOperationsInterceptor.java

Lines 353 to 359 in 0c1fa82

	Map<Class<? extends Throwable>, Boolean> policyMap = new HashMap<Class<? extends Throwable>, Boolean>();
	for (Class<? extends Throwable> type : includes) {
	policyMap.put(type, true);
	}
	for (Class<? extends Throwable> type : excludes) {
	policyMap.put(type, false);
	}

[ttulka]

@garyrussell Thanks! I have changed the code according to your comments.

[garyrussell]

@ttulka I don't understand why we need this at all; doesn't excludes do what you want?

[garyrussell]

This is identical to the previous 3 lines.

[ttulka]

it is using rethrowExceptions, the previous for is using excludes to iterate over.

[garyrussell]

Right but why do we need rethrowExceptions when we already have excludes + rethrow?

[ttulka]

Using rethrowExceptions on Retryable sets the noRecoveryForNotRetryable flag of RetryTemplate, so the listed exceptions are not only not-retryable, but also directly rethrown (without recovery and wrapping).

[garyrussell]

So, just set rethrow alongside excludes.

[garyrussell]

Sine 1.3.3 hasn't been built yet, maybe we should revert that commit until this is sorted out?

[artembilan]

That's sounds reasonable.
Looks like that one was requested by @ttulka as well: ec245fd.

So, we are safe so to not release anything on the matter and continue discussion 😄

[garyrussell]

Done - maybe we should move this whole thing to the new 2.0 branch where we can overhaul the entire attribute list?

[ttulka]

Not exactly happy about not getting anything in the next release, but as far as you guys decide on the API attributes, I would love to implement it if you want me to.

All in all, this feature is going to save me a lot of work!

Many thanks

[garyrussell]

I understand you are "not happy", but you realized that the previous commit did not satisfy all your needs and submitted this new PR, that does not use the new attribute from the previous commit.

It made no sense to us to release a version with a new attribute that would immediately become obsolete in the "next" release.

We are happy that you are willing to contribute, but we need to finalize a design that makes sense for the entire community, and provides minimal confusion for users with simpler use cases.

Thanks, again, for your contribution; we will continue to work this through.

[ttulka]

@garyrussell Just using excludes will still search for a recovery method and wrap the exception. I need to rethrow the raw exception immediately.

[artembilan]

I think the premise of this request has been addressed after introducing this attribute to the @Retryable:

	/**
	 * Exception types that are not recoverable; these exceptions are thrown to the caller
	 * without calling any recoverer (immediately if also in {@link #noRetryFor()}).
	 * Defaults to empty.
	 * @return exception types not to retry
	 * @since 2.0
	 */
	Class<? extends Throwable>[] notRecoverable() default {};

So, closing as duplication of: #264