Skip to content

Commit

Permalink
Change the strategy for key generation in stateful retry
Browse files Browse the repository at this point in the history
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
  • Loading branch information
Dave Syer committed Aug 23, 2016
1 parent 08cb981 commit 411d47d
Show file tree
Hide file tree
Showing 4 changed files with 27 additions and 17 deletions.
Expand Up @@ -41,6 +41,7 @@
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;
Expand Down Expand Up @@ -209,6 +210,7 @@ private MethodInterceptor getStatefulInterceptor(Object target, Method method, R
label = method.toGenericString();
}
return RetryInterceptorBuilder.circuitBreaker()
.keyGenerator(new FixedKeyGenerator("circuit"))
.retryOperations(template)
.recoverer(getRecoverer(target, method))
.label(label)
Expand Down
Expand Up @@ -347,6 +347,11 @@ public CircuitBreakerInterceptorBuilder retryPolicy(RetryPolicy policy) {
return this;
}

public CircuitBreakerInterceptorBuilder keyGenerator(MethodArgumentsKeyGenerator keyGenerator) {
this.keyGenerator = keyGenerator;
return this;
}

@Override
public CircuitBreakerInterceptorBuilder recoverer(
MethodInvocationRecoverer<?> recoverer) {
Expand Down
Expand Up @@ -91,8 +91,8 @@ public void setRecoverer(MethodInvocationRecoverer<?> recoverer) {
}

/**
* 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
*/
Expand Down Expand Up @@ -141,24 +141,26 @@ public Object invoke(final MethodInvocation invocation) throws Throwable {
+ ObjectUtils.getIdentityHexString(invocation) + ")");
}

String name = invocation.getMethod().toGenericString();

Object[] args = invocation.getArguments();
Object arg = args;
Object defaultKey = Arrays.asList(args);
if (args.length == 1) {
arg = args[0];
defaultKey = args[0];
}
final Object item = arg;

Object key = this.keyGenerator != null ? this.keyGenerator.getKey(args) : item;
RetryState retryState = new DefaultRetryState(
key,
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: ("
Expand All @@ -181,9 +183,10 @@ private static final class MethodInvocationRetryCallback

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();
}
}
Expand Down
Expand Up @@ -60,12 +60,12 @@ public void close() {

@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,
Expand Down Expand Up @@ -101,7 +101,7 @@ protected static class Service {
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) {
Expand Down

0 comments on commit 411d47d

Please sign in to comment.