From 411d47d7b2937777e77bd75953b294c46b049e46 Mon Sep 17 00:00:00 2001 From: Dave Syer Date: Tue, 23 Aug 2016 12:06:44 +0100 Subject: [PATCH] Change the strategy for key generation in stateful retry There were a couple of issues to fix here. The first was that only the method arguments and not the method signature (or label) were being used in the key generator. Plus the arguments were being used as an array, which has a different hashcode on each invocation (a Collection would be better). Plus the interceptor builder didn't set the key generator in a circuit breaker, so all the method calls with different args are unique and they are supposed to be the same. See gh-49 --- ...tationAwareRetryOperationsInterceptor.java | 2 + .../interceptor/RetryInterceptorBuilder.java | 5 +++ .../StatefulRetryOperationsInterceptor.java | 37 ++++++++++--------- ...cuitBreakerInterceptorStatisticsTests.java | 6 +-- 4 files changed, 30 insertions(+), 20 deletions(-) diff --git a/src/main/java/org/springframework/retry/annotation/AnnotationAwareRetryOperationsInterceptor.java b/src/main/java/org/springframework/retry/annotation/AnnotationAwareRetryOperationsInterceptor.java index c9c1d1e..7b7f973 100644 --- a/src/main/java/org/springframework/retry/annotation/AnnotationAwareRetryOperationsInterceptor.java +++ b/src/main/java/org/springframework/retry/annotation/AnnotationAwareRetryOperationsInterceptor.java @@ -41,6 +41,7 @@ import org.springframework.retry.backoff.FixedBackOffPolicy; import org.springframework.retry.backoff.NoBackOffPolicy; import org.springframework.retry.backoff.Sleeper; import org.springframework.retry.backoff.UniformRandomBackOffPolicy; +import org.springframework.retry.interceptor.FixedKeyGenerator; import org.springframework.retry.interceptor.MethodArgumentsKeyGenerator; import org.springframework.retry.interceptor.MethodInvocationRecoverer; import org.springframework.retry.interceptor.NewMethodArgumentsIdentifier; @@ -209,6 +210,7 @@ public class AnnotationAwareRetryOperationsInterceptor implements IntroductionIn label = method.toGenericString(); } return RetryInterceptorBuilder.circuitBreaker() + .keyGenerator(new FixedKeyGenerator("circuit")) .retryOperations(template) .recoverer(getRecoverer(target, method)) .label(label) diff --git a/src/main/java/org/springframework/retry/interceptor/RetryInterceptorBuilder.java b/src/main/java/org/springframework/retry/interceptor/RetryInterceptorBuilder.java index 5159a57..411bdc0 100644 --- a/src/main/java/org/springframework/retry/interceptor/RetryInterceptorBuilder.java +++ b/src/main/java/org/springframework/retry/interceptor/RetryInterceptorBuilder.java @@ -347,6 +347,11 @@ public abstract class RetryInterceptorBuilder { return this; } + public CircuitBreakerInterceptorBuilder keyGenerator(MethodArgumentsKeyGenerator keyGenerator) { + this.keyGenerator = keyGenerator; + return this; + } + @Override public CircuitBreakerInterceptorBuilder recoverer( MethodInvocationRecoverer recoverer) { diff --git a/src/main/java/org/springframework/retry/interceptor/StatefulRetryOperationsInterceptor.java b/src/main/java/org/springframework/retry/interceptor/StatefulRetryOperationsInterceptor.java index 02899e2..c0cd2b6 100644 --- a/src/main/java/org/springframework/retry/interceptor/StatefulRetryOperationsInterceptor.java +++ b/src/main/java/org/springframework/retry/interceptor/StatefulRetryOperationsInterceptor.java @@ -91,8 +91,8 @@ public class StatefulRetryOperationsInterceptor implements MethodInterceptor { } /** - * Rollback classifier for the retry state. Default to null (meaning rollback - * for all). + * Rollback classifier for the retry state. Default to null (meaning rollback for + * all). * * @param rollbackClassifier the rollbackClassifier to set */ @@ -141,24 +141,26 @@ public class StatefulRetryOperationsInterceptor implements MethodInterceptor { + ObjectUtils.getIdentityHexString(invocation) + ")"); } - Object[] args = invocation.getArguments(); - Object arg = args; - if (args.length == 1) { - arg = args[0]; - } - final Object item = arg; + String name = invocation.getMethod().toGenericString(); - Object key = this.keyGenerator != null ? this.keyGenerator.getKey(args) : item; - RetryState retryState = new DefaultRetryState( - key, + Object[] args = invocation.getArguments(); + Object defaultKey = Arrays.asList(args); + if (args.length == 1) { + defaultKey = args[0]; + } + + Object key = Arrays.asList(name, this.keyGenerator != null + ? this.keyGenerator.getKey(invocation.getArguments()) : defaultKey ); + RetryState retryState = new DefaultRetryState(key, this.newMethodArgumentsIdentifier != null && this.newMethodArgumentsIdentifier.isNew(args), this.rollbackClassifier); - Object result = this.retryOperations.execute( - new MethodInvocationRetryCallback(invocation, label), this.recoverer != null - ? new ItemRecovererCallback(args, this.recoverer) : null, - retryState); + Object result = this.retryOperations + .execute(new MethodInvocationRetryCallback(invocation, label), + this.recoverer != null + ? new ItemRecovererCallback(args, this.recoverer) : null, + retryState); if (this.logger.isDebugEnabled()) { this.logger.debug("Exiting proxied method in stateful retry with result: (" @@ -181,9 +183,10 @@ public class StatefulRetryOperationsInterceptor implements MethodInterceptor { private MethodInvocationRetryCallback(MethodInvocation invocation, String label) { this.invocation = invocation; - if (label!=null) { + if (label != null) { this.label = label; - } else { + } + else { this.label = invocation.getMethod().toGenericString(); } } diff --git a/src/test/java/org/springframework/retry/stats/CircuitBreakerInterceptorStatisticsTests.java b/src/test/java/org/springframework/retry/stats/CircuitBreakerInterceptorStatisticsTests.java index 1148372..8eae3aa 100644 --- a/src/test/java/org/springframework/retry/stats/CircuitBreakerInterceptorStatisticsTests.java +++ b/src/test/java/org/springframework/retry/stats/CircuitBreakerInterceptorStatisticsTests.java @@ -60,12 +60,12 @@ public class CircuitBreakerInterceptorStatisticsTests { @Test public void testCircuitOpenWhenNotRetryable() throws Throwable { - Object result = callback.service(); + Object result = callback.service("one"); RetryStatistics stats = repository.findOne("test"); // System.err.println(stats); assertEquals(1, stats.getStartedCount()); assertEquals(RECOVERED, result); - result = callback.service(); + result = callback.service("two"); assertEquals(RECOVERED, result); assertEquals("There should be two recoveries", 2, stats.getRecoveryCount()); assertEquals("There should only be one error because the circuit is now open", 1, @@ -101,7 +101,7 @@ public class CircuitBreakerInterceptorStatisticsTests { private RetryContext status; @CircuitBreaker(label = "test", maxAttempts = 1) - public Object service() throws Exception { + public Object service(String input) throws Exception { this.status = RetrySynchronizationManager.getContext(); Integer attempts = (Integer) status.getAttribute("attempts"); if (attempts == null) {