GH-184: Expression Evaluation at Runtime #298

garyrussell · 2022-05-03T17:45:21Z

Resolves #184

artembilan

I'm not fully buying this evaluation mode and would live with just a runtime one like we do in many other places. Plus it might turn in some use-case that end-user would like to have a mix of modes for different expressions. Also it looks a bit clumsy for nested annotations...

artembilan · 2022-05-03T19:19:10Z

src/main/java/org/springframework/retry/policy/SimpleRetryPolicy.java

@@ -218,7 +240,7 @@ private boolean retryForException(Throwable ex) {

 	@Override
 	public String toString() {
-		return ClassUtils.getShortName(getClass()) + "[maxAttempts=" + this.maxAttempts + "]";
+		return ClassUtils.getShortName(getClass()) + "[maxAttempts=" + this.maxAttemptsSupplier + "]";


Suggested change

return ClassUtils.getShortName(getClass()) + "[maxAttempts=" + this.maxAttemptsSupplier + "]";

return ClassUtils.getShortName(getClass()) + "[maxAttempts=" + getMaxAttempts() + "]";

?

Looks like has not been addressed.

artembilan · 2022-05-03T19:28:35Z

src/main/java/org/springframework/retry/backoff/UniformRandomBackOffPolicy.java

-					: random.nextInt((int) (maxBackOffPeriod - minBackOffPeriod));
-			sleeper.sleep(minBackOffPeriod + delta);
+			Long min = this.minBackOffPeriod.get();
+			long delta = this.maxBackOffPeriod == this.minBackOffPeriod ? 0


I think this is something wrong: you compare Supplier instances - not their values.

garyrussell · 2022-05-03T20:53:03Z

I don't think we should add the overhead of runtime evaluation for all users; when most won't need it; it also causes problems because the SimpleRetryPolicy must be serializable (used in the circuit breaker retry context. See the note in the README and javadocs.

If you have a better suggestion for the toggle, I am open to hearing it.

garyrussell · 2022-05-03T23:18:36Z

One alternative I considered was template expressions #{...} are evaluated at initialization and non-template expressions are evaluated at runtime.

This would be a behavior change, but it's a major release, so I could agree to it.

artembilan · 2022-05-04T13:54:05Z

Yeah...

If we would like to be consistent with an XML configuration and its SpEL support, then indeed that @Retryable(maxAttempts) has to be a string and support a "templated" expression evaluation during configuration phase.
Meanwhile a maxAttemptsExpression would really lead to the runtime expression evaluation.
Just similar to what we have so far in Spring Integration.

Looking to the same @Retryable right now we have:

	int maxAttempts() default 3;

	String maxAttemptsExpression() default "";

	String exceptionExpression() default "";

Where it is not clear that maxAttemptsExpression is going to be evaluated at configuration phase or at runtime, but exceptionExpression states it clearly that it is for runtime.
That's another argument from me against a common evaluation mode option for the whole annotation.

So, if we cannot change simple value attributes (like that maxAttempts to String), then your suggestion for templating on companion SpEL attributes doesn't sound bad to distinguish runtime and configuration phase evaluations.

garyrussell · 2022-05-04T13:58:37Z

I think changing the types would be too much of a breaking change - even for a major release (I suspect many users don't even use expressions); I'll go with templating idea.

…cider.

src/main/java/org/springframework/retry/policy/SimpleRetryPolicy.java

artembilan · 2022-05-04T16:34:52Z

src/main/java/org/springframework/retry/policy/SimpleRetryPolicy.java

@@ -218,7 +240,7 @@ private boolean retryForException(Throwable ex) {

 	@Override
 	public String toString() {
-		return ClassUtils.getShortName(getClass()) + "[maxAttempts=" + this.maxAttempts + "]";
+		return ClassUtils.getShortName(getClass()) + "[maxAttempts=" + this.maxAttemptsSupplier + "]";


Looks like has not been addressed.

artembilan · 2022-05-04T16:40:50Z

README.md

+With earlier versions, evaluation was always performed during initialization (except for `Retryable.exceptionExpression` which is always evaluated at runtime).
+When evaluating at runtime, a root object containing the method arguments is passed to the evaluation context.
+
+**Note:** The arguments are not available until the method has been called at least once; they will be null initially, which means, for example, you can't set the initial `maxAttempts` using an argument value, you can, however, change the `maxAttempts` after the first failure and before any retries are performed.


Is it only a limitation for the maxAttempts option?

I see open() is done far before a doWithRetry(), so perhaps those args does not make sense for circuit breaker...

Looks like BackOff expressions don't suffer from this limitation: it is indeed called in the catch from the doWithRetry().

Do I miss anything?

Thanks

Right - the back off is not evaluated until after the method is called so, yes, the limitation is only on the maxAttempts.

Good point about the CB.

It turns out that we can't use args at all for any stateful retry (including circuit breakers) because you would get the args from the previous call (which may be different).

Other polishing from review.

artembilan · 2022-05-04T18:57:42Z

...ain/java/org/springframework/retry/annotation/AnnotationAwareRetryOperationsInterceptor.java

+			}
+			if (args == null) {
+				args = Args.NO_ARGS;
+			}
 		}
 		return expression.getValue(this.evaluationContext, args, type);


Is it expected to propagate an args var as null for statefull unlike for stateless with an Args.NO_ARGS?

Yes; I want them to get a hard error EL1007E: Property or field 'args' cannot be found on null.

I failed to save the doc change for this; pushing now...

spring-projectsGH-184: Expression Evaluation at Runtime

b6a7632

Resolves spring-projects#184

artembilan requested changes May 3, 2022

View reviewed changes

Remove expressionEvaluation properties; use template or not as the de…

5ded394

…cider.

artembilan requested changes May 4, 2022

View reviewed changes

Remove args property when using stateful (including CircuitBreaker).

79e93d9

Other polishing from review.

artembilan requested changes May 4, 2022

View reviewed changes

Docs for stateful.

7077a70

artembilan merged commit 47c1e52 into spring-projects:main May 4, 2022

garyrussell deleted the GH-184 branch May 4, 2022 19:28

	return ClassUtils.getShortName(getClass()) + "[maxAttempts=" + this.maxAttemptsSupplier + "]";
	return ClassUtils.getShortName(getClass()) + "[maxAttempts=" + getMaxAttempts() + "]";

GH-184: Expression Evaluation at Runtime #298

GH-184: Expression Evaluation at Runtime #298

Uh oh!

Conversation

garyrussell commented May 3, 2022

Uh oh!

artembilan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

garyrussell commented May 3, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

garyrussell commented May 3, 2022

Uh oh!

artembilan commented May 4, 2022

Uh oh!

garyrussell commented May 4, 2022

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

garyrussell commented May 3, 2022 •

edited

Loading