From c914ce74fcaecb7c73f57b87e57decb9cea3d0f1 Mon Sep 17 00:00:00 2001 From: dsyer Date: Wed, 21 May 2008 13:13:50 +0000 Subject: [PATCH] OPEN - issue BATCH-572: Retryable exceptions cannot be skippable Tidied up Javadocs and tests. Throw exception if retry exhausted and no recovery path provided. --- .../retry/callback/RecoveryRetryCallback.java | 27 +++++------ .../StatefulRetryOperationsInterceptor.java | 45 ++++++++++++++----- ...atefulRetryOperationsInterceptorTests.java | 26 ++++++++++- 3 files changed, 71 insertions(+), 27 deletions(-) diff --git a/spring-batch-infrastructure/src/main/java/org/springframework/batch/retry/callback/RecoveryRetryCallback.java b/spring-batch-infrastructure/src/main/java/org/springframework/batch/retry/callback/RecoveryRetryCallback.java index 1d8f3ba85..4985d778f 100644 --- a/spring-batch-infrastructure/src/main/java/org/springframework/batch/retry/callback/RecoveryRetryCallback.java +++ b/spring-batch-infrastructure/src/main/java/org/springframework/batch/retry/callback/RecoveryRetryCallback.java @@ -16,8 +16,6 @@ package org.springframework.batch.retry.callback; -import org.springframework.batch.item.ItemRecoverer; -import org.springframework.batch.item.ItemWriter; import org.springframework.batch.retry.RecoveryCallback; import org.springframework.batch.retry.RetryCallback; import org.springframework.batch.retry.RetryContext; @@ -26,8 +24,8 @@ import org.springframework.batch.retry.policy.RecoveryCallbackRetryPolicy; /** * A {@link RetryCallback} that knows about and caches an item, and attempts to - * process it using an {@link ItemWriter}. Used by the - * {@link RecoveryCallbackRetryPolicy} to enable external retry of the item + * process it using a delegate {@link RetryCallback}. Used by the + * {@link RecoveryCallbackRetryPolicy} to enable stateful retry of the * processing. * * @author Dave Syer @@ -52,12 +50,12 @@ public class RecoveryRetryCallback implements RetryCallback { * Constructor with mandatory properties. The key will be set to the item. * * @param item the item to process - * @param writer the writer to use to process it + * @param callback the delegate to use to process it */ - public RecoveryRetryCallback(Object item, RetryCallback writer) { + public RecoveryRetryCallback(Object item, RetryCallback callback) { super(); this.item = item; - this.callback = writer; + this.callback = callback; this.key = item; } @@ -65,12 +63,12 @@ public class RecoveryRetryCallback implements RetryCallback { * Constructor with mandatory properties. * * @param item the item to process - * @param writer the writer to use to process it + * @param callback the delegate to use to process it */ - public RecoveryRetryCallback(Object item, RetryCallback writer, Object key) { + public RecoveryRetryCallback(Object item, RetryCallback callback, Object key) { super(); this.item = item; - this.callback = writer; + this.callback = callback; this.key = key; } @@ -84,10 +82,7 @@ public class RecoveryRetryCallback implements RetryCallback { } /** - * Setter for injecting optional recovery handler. If it is not injected but - * the reader or writer implement {@link ItemRecoverer}, one of those will - * be used instead (preferring the reader to the writer if both would be - * appropriate). + * Setter for injecting optional recovery handler. * * @param recoverer */ @@ -126,9 +121,9 @@ public class RecoveryRetryCallback implements RetryCallback { } /** - * Accessor for the {@link ItemRecoverer}. + * Accessor for the {@link RecoveryCallback}. * - * @return the {@link ItemRecoverer}. + * @return the {@link RecoveryCallback}. */ public RecoveryCallback getRecoveryCallback() { return recoverer; diff --git a/spring-batch-infrastructure/src/main/java/org/springframework/batch/retry/interceptor/StatefulRetryOperationsInterceptor.java b/spring-batch-infrastructure/src/main/java/org/springframework/batch/retry/interceptor/StatefulRetryOperationsInterceptor.java index 245c306cc..f2fac4317 100644 --- a/spring-batch-infrastructure/src/main/java/org/springframework/batch/retry/interceptor/StatefulRetryOperationsInterceptor.java +++ b/spring-batch-infrastructure/src/main/java/org/springframework/batch/retry/interceptor/StatefulRetryOperationsInterceptor.java @@ -23,17 +23,37 @@ import org.apache.commons.logging.LogFactory; import org.springframework.batch.item.ItemKeyGenerator; import org.springframework.batch.item.ItemRecoverer; import org.springframework.batch.item.NewItemIdentifier; +import org.springframework.batch.retry.ExhaustedRetryException; import org.springframework.batch.retry.RecoveryCallback; import org.springframework.batch.retry.RetryCallback; import org.springframework.batch.retry.RetryContext; +import org.springframework.batch.retry.RetryOperations; import org.springframework.batch.retry.RetryPolicy; import org.springframework.batch.retry.callback.RecoveryRetryCallback; -import org.springframework.batch.retry.policy.RecoveryCallbackRetryPolicy; import org.springframework.batch.retry.policy.NeverRetryPolicy; +import org.springframework.batch.retry.policy.RecoveryCallbackRetryPolicy; import org.springframework.batch.retry.support.RetryTemplate; import org.springframework.util.Assert; import org.springframework.util.ObjectUtils; +/** + * A {@link MethodInterceptor} that can be used to automatically retry calls to + * a method on a service if it fails. The argument to the service method is + * treated as an item to be remembered in case the call fails. So the retry + * operation is stateful, and the item that failed is tracked by its unique key + * (via {@link ItemKeyGenerator}) until the retry is exhausted, at which point + * the {@link ItemRecoverer} is called.
+ * + * The main use case for this is where the service is transactional, via a + * transaction interceptor on the interceptor chain. In this case the retry (and + * recovery on exhausted) always happens in a new transaction.
+ * + * The injected {@link RetryOperations} is used to control the number of + * retries. By default it will retry a fixed number of times, according to the + * defaults in {@link RetryTemplate}.
+ * + * @author Dave Syer + */ public class StatefulRetryOperationsInterceptor implements MethodInterceptor { private transient Log logger = LogFactory.getLog(getClass()); @@ -58,7 +78,10 @@ public class StatefulRetryOperationsInterceptor implements MethodInterceptor { * Public setter for the {@link ItemRecoverer} to use if the retry is * exhausted. The recoverer should be able to return an object of the same * type as the target object because its return value will be used to return - * to the caller in the case of a recovery. + * to the caller in the case of a recovery.
+ * + * If no recoverer is set then an exhausted retry will result in an + * {@link ExhaustedRetryException}. * * @param recoverer the {@link ItemRecoverer} to set */ @@ -71,7 +94,9 @@ public class StatefulRetryOperationsInterceptor implements MethodInterceptor { } /** - * Public setter for the retryPolicy. + * Public setter for the retryPolicy. The value provided should be a normal + * stateless policy, which is wrapped into a stateful policy inside this + * method. * @param retryPolicy the retryPolicy to set */ public void setRetryPolicy(RetryPolicy retryPolicy) { @@ -94,10 +119,14 @@ public class StatefulRetryOperationsInterceptor implements MethodInterceptor { * re-thrown. The only time it is not re-thrown is when retry is exhausted * and the recovery path is taken (though the {@link ItemRecoverer} provided * if there is one). In that case the value returned from the method - * invocation will be null, or if primitive then "0" (e.g. Boolean.FALSE, 0L - * etc.). + * invocation will be the value returned by the recoverer (so the return + * type for that should be the same as the intercepted method). * * @see org.aopalliance.intercept.MethodInterceptor#invoke(org.aopalliance.intercept.MethodInvocation) + * @see ItemRecoverer#recover(Object, Throwable) + * + * @throws ExhaustedRetryException if the retry is exhausted and no + * {@link ItemRecoverer} is provided. */ public Object invoke(final MethodInvocation invocation) throws Throwable { @@ -172,11 +201,7 @@ public class StatefulRetryOperationsInterceptor implements MethodInterceptor { if (recoverer != null) { return recoverer.recover(item, context.getLastThrowable()); } - // TODO: This sucks big time because the method invocation almost - // certainly does not return an object of this type. It would be - // better to return null (but then method invocations that return - // primitive values would barf). - return item; + throw new ExhaustedRetryException("Retry was exhausted but there was no recovery path."); } } diff --git a/spring-batch-infrastructure/src/test/java/org/springframework/batch/retry/interceptor/StatefulRetryOperationsInterceptorTests.java b/spring-batch-infrastructure/src/test/java/org/springframework/batch/retry/interceptor/StatefulRetryOperationsInterceptorTests.java index 7fe6b3f32..7a7fa3cd5 100644 --- a/spring-batch-infrastructure/src/test/java/org/springframework/batch/retry/interceptor/StatefulRetryOperationsInterceptorTests.java +++ b/spring-batch-infrastructure/src/test/java/org/springframework/batch/retry/interceptor/StatefulRetryOperationsInterceptorTests.java @@ -28,6 +28,7 @@ import org.springframework.aop.framework.Advised; import org.springframework.aop.framework.ProxyFactory; import org.springframework.aop.target.SingletonTargetSource; import org.springframework.batch.item.ItemRecoverer; +import org.springframework.batch.retry.ExhaustedRetryException; import org.springframework.batch.retry.policy.AlwaysRetryPolicy; import org.springframework.batch.retry.policy.NeverRetryPolicy; import org.springframework.batch.retry.policy.SimpleRetryPolicy; @@ -134,7 +135,30 @@ public class StatefulRetryOperationsInterceptorTests extends TestCase { assertEquals(1, result.size()); } - public void testRetryExceptionAfterTooManyAttempts() throws Exception { + public void testRetryExceptionAfterTooManyAttemptsWithNoRecovery() throws Exception { + ((Advised) service).addAdvice(interceptor); + interceptor.setRetryPolicy(new NeverRetryPolicy()); + try { + service.service("foo"); + fail("Expected Exception."); + } + catch (Exception e) { + String message = e.getMessage(); + assertTrue("Wrong message: " + message, message.startsWith("Not enough calls")); + } + assertEquals(1, count); + try { + service.service("foo"); + fail("Expected ExhaustedRetryException"); + } catch (ExhaustedRetryException e) { + // expected + String message = e.getMessage(); + assertTrue("Wrong message: " + message, message.startsWith("Retry was exhausted but there was no recover")); + } + assertEquals(1, count); + } + + public void testRecoveryAfterTooManyAttempts() throws Exception { ((Advised) service).addAdvice(interceptor); interceptor.setRetryPolicy(new NeverRetryPolicy()); try {