From 1b1828d6e1cfd49b86cc620623293a54d5bf6ea6 Mon Sep 17 00:00:00 2001 From: aftab <44019928+aftabshk@users.noreply.github.com> Date: Fri, 10 May 2024 23:06:52 +0530 Subject: [PATCH] GH-427: Fix multiplierExpression & multiplierExpression Fixes: #427 No randomness with configuration like: ``` @Retryable(retryFor = {RuntimeException.class}, maxAttemptsExpression = "${retry.max-attempts}", backoff = @Backoff(delayExpression = "${retry.delay}", multiplierExpression = "${retry.multiplier}", randomExpression = "${retry.random}")) ``` The random logic in the `AnnotationAwareRetryOperationsInterceptor` if `multiplierExpression` is for runtime evaluation. * Fix `AnnotationAwareRetryOperationsInterceptor` to check for `if (multiplier > 0 || parsedMultExp != null) {` before evaluating `random` * Fix `@BackOff(randomExpression)` Javadoc to indicate that it is always evaluated on configuration phase. --- ...tationAwareRetryOperationsInterceptor.java | 4 +- .../retry/annotation/Backoff.java | 7 +- .../retry/backoff/BackOffPolicyBuilder.java | 8 +- .../backoff/BackOffPolicyBuilderTests.java | 74 +++++++++++++++++++ 4 files changed, 89 insertions(+), 4 deletions(-) diff --git a/src/main/java/org/springframework/retry/annotation/AnnotationAwareRetryOperationsInterceptor.java b/src/main/java/org/springframework/retry/annotation/AnnotationAwareRetryOperationsInterceptor.java index 2f906dee..cab4032a 100644 --- a/src/main/java/org/springframework/retry/annotation/AnnotationAwareRetryOperationsInterceptor.java +++ b/src/main/java/org/springframework/retry/annotation/AnnotationAwareRetryOperationsInterceptor.java @@ -76,6 +76,7 @@ * @author Artem Bilan * @author Gary Russell * @author Roman Akentev + * @author Aftab Shaikh * @since 1.1 */ public class AnnotationAwareRetryOperationsInterceptor implements IntroductionInterceptor, BeanFactoryAware { @@ -451,7 +452,8 @@ private BackOffPolicy getBackoffPolicy(Backoff backoff, boolean stateless) { boolean isRandom = false; String randomExpression = (String) attrs.get("randomExpression"); Expression parsedRandomExp = null; - if (multiplier > 0) { + + if (multiplier > 0 || parsedMultExp != null) { isRandom = backoff.random(); if (StringUtils.hasText(randomExpression)) { parsedRandomExp = parse(randomExpression); diff --git a/src/main/java/org/springframework/retry/annotation/Backoff.java b/src/main/java/org/springframework/retry/annotation/Backoff.java index ec891029..82c23cda 100644 --- a/src/main/java/org/springframework/retry/annotation/Backoff.java +++ b/src/main/java/org/springframework/retry/annotation/Backoff.java @@ -41,6 +41,7 @@ * * @author Dave Syer * @author Gary Russell + * @author Aftab Shaikh * @since 1.1 * */ @@ -129,8 +130,10 @@ * Evaluates to a value. In the exponential case ({@link #multiplier()} > 1.0) set * this to true to have the backoff delays randomized, so that the maximum delay is * multiplier times the previous delay and the distribution is uniform between the two - * values. Use {@code #{...}} for one-time evaluation during initialization, omit the - * delimiters for evaluation at runtime. + * values. This expression is always evaluated during initialization. If the + * expression returns true then + * {@link org.springframework.retry.backoff.ExponentialRandomBackOffPolicy} is used + * else {@link org.springframework.retry.backoff.ExponentialBackOffPolicy} is used. * @return the flag to signal randomization is required (default false) */ String randomExpression() default ""; diff --git a/src/main/java/org/springframework/retry/backoff/BackOffPolicyBuilder.java b/src/main/java/org/springframework/retry/backoff/BackOffPolicyBuilder.java index b1e4f8be..f56d89a2 100644 --- a/src/main/java/org/springframework/retry/backoff/BackOffPolicyBuilder.java +++ b/src/main/java/org/springframework/retry/backoff/BackOffPolicyBuilder.java @@ -68,6 +68,7 @@ * load concurrent access. * * @author Tomaz Fernandes + * @author Aftab Shaikh * @since 1.3.3 */ public final class BackOffPolicyBuilder { @@ -217,7 +218,7 @@ public BackOffPolicyBuilder randomSupplier(Supplier randomSupplier) { public BackOffPolicy build() { if (this.multiplier != null && this.multiplier > 0 || this.multiplierSupplier != null) { ExponentialBackOffPolicy policy; - if (Boolean.TRUE.equals(this.random)) { + if (isRandom()) { policy = new ExponentialRandomBackOffPolicy(); } else { @@ -279,4 +280,9 @@ else if (this.delay != null) { return policy; } + private boolean isRandom() { + return (this.randomSupplier != null && Boolean.TRUE.equals(this.randomSupplier.get())) + || Boolean.TRUE.equals(this.random); + } + } diff --git a/src/test/java/org/springframework/retry/backoff/BackOffPolicyBuilderTests.java b/src/test/java/org/springframework/retry/backoff/BackOffPolicyBuilderTests.java index b477f592..3e06ffda 100644 --- a/src/test/java/org/springframework/retry/backoff/BackOffPolicyBuilderTests.java +++ b/src/test/java/org/springframework/retry/backoff/BackOffPolicyBuilderTests.java @@ -26,6 +26,7 @@ /** * @author Tomaz Fernandes * @author Gary Russell + * @author Aftab Shaikh * @since 1.3.3 */ public class BackOffPolicyBuilderTests { @@ -109,4 +110,77 @@ public void shouldCreateExponentialRandomBackOff() { assertThat(new DirectFieldAccessor(policy).getPropertyValue("sleeper")).isEqualTo(mockSleeper); } + @Test + public void shouldCreateExponentialRandomBackOffWhenProvidedRandomSupplier() { + Sleeper mockSleeper = mock(Sleeper.class); + BackOffPolicy backOffPolicy = BackOffPolicyBuilder.newBuilder() + .delay(10000) + .maxDelay(100000) + .multiplier(10) + .randomSupplier(() -> true) + .sleeper(mockSleeper) + .build(); + + assertThat(ExponentialRandomBackOffPolicy.class.isAssignableFrom(backOffPolicy.getClass())).isTrue(); + ExponentialRandomBackOffPolicy policy = (ExponentialRandomBackOffPolicy) backOffPolicy; + assertThat(policy.getInitialInterval()).isEqualTo(10000); + assertThat(policy.getMaxInterval()).isEqualTo(100000); + assertThat(policy.getMultiplier()).isEqualTo(10); + } + + @Test + public void shouldCreateExponentialRandomBackOffWithProvidedSuppliers() { + Sleeper mockSleeper = mock(Sleeper.class); + BackOffPolicy backOffPolicy = BackOffPolicyBuilder.newBuilder() + .delaySupplier(() -> 10000L) + .maxDelaySupplier(() -> 100000L) + .multiplierSupplier(() -> 10d) + .randomSupplier(() -> true) + .sleeper(mockSleeper) + .build(); + + assertThat(ExponentialRandomBackOffPolicy.class.isAssignableFrom(backOffPolicy.getClass())).isTrue(); + ExponentialRandomBackOffPolicy policy = (ExponentialRandomBackOffPolicy) backOffPolicy; + assertThat(policy.getInitialInterval()).isEqualTo(10000); + assertThat(policy.getMaxInterval()).isEqualTo(100000); + assertThat(policy.getMultiplier()).isEqualTo(10); + } + + @Test + public void shouldCreateExponentialBackOffWhenProvidedRandomSupplier() { + Sleeper mockSleeper = mock(Sleeper.class); + BackOffPolicy backOffPolicy = BackOffPolicyBuilder.newBuilder() + .delay(100) + .maxDelay(1000) + .multiplier(2) + .randomSupplier(() -> false) + .sleeper(mockSleeper) + .build(); + assertThat(backOffPolicy instanceof ExponentialRandomBackOffPolicy).isFalse(); + assertThat(ExponentialBackOffPolicy.class.isAssignableFrom(backOffPolicy.getClass())).isTrue(); + ExponentialBackOffPolicy policy = (ExponentialBackOffPolicy) backOffPolicy; + assertThat(policy.getInitialInterval()).isEqualTo(100); + assertThat(policy.getMaxInterval()).isEqualTo(1000); + assertThat(policy.getMultiplier()).isEqualTo(2); + } + + @Test + public void shouldCreateExponentialBackOffWithProvidedSuppliers() { + Sleeper mockSleeper = mock(Sleeper.class); + BackOffPolicy backOffPolicy = BackOffPolicyBuilder.newBuilder() + .delaySupplier(() -> 10000L) + .maxDelaySupplier(() -> 100000L) + .multiplierSupplier(() -> 10d) + .randomSupplier(() -> false) + .sleeper(mockSleeper) + .build(); + + assertThat(backOffPolicy instanceof ExponentialRandomBackOffPolicy).isFalse(); + assertThat(ExponentialBackOffPolicy.class.isAssignableFrom(backOffPolicy.getClass())).isTrue(); + ExponentialBackOffPolicy policy = (ExponentialBackOffPolicy) backOffPolicy; + assertThat(policy.getInitialInterval()).isEqualTo(10000); + assertThat(policy.getMaxInterval()).isEqualTo(100000); + assertThat(policy.getMultiplier()).isEqualTo(10); + } + }