Skip to content

Commit

Permalink
spring-projectsGH-427: Fix multiplierExpression & multiplierExpression
Browse files Browse the repository at this point in the history
Fixes: spring-projects#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.
  • Loading branch information
aftabshk authored and app committed May 20, 2024
1 parent 7e9e216 commit 1b1828d
Show file tree
Hide file tree
Showing 4 changed files with 89 additions and 4 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@
*
* @author Dave Syer
* @author Gary Russell
* @author Aftab Shaikh
* @since 1.1
*
*/
Expand Down Expand Up @@ -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 "";
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,7 @@
* load concurrent access.
*
* @author Tomaz Fernandes
* @author Aftab Shaikh
* @since 1.3.3
*/
public final class BackOffPolicyBuilder {
Expand Down Expand Up @@ -217,7 +218,7 @@ public BackOffPolicyBuilder randomSupplier(Supplier<Boolean> 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 {
Expand Down Expand Up @@ -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);
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
/**
* @author Tomaz Fernandes
* @author Gary Russell
* @author Aftab Shaikh
* @since 1.3.3
*/
public class BackOffPolicyBuilderTests {
Expand Down Expand Up @@ -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);
}

}

0 comments on commit 1b1828d

Please sign in to comment.