From fac1c2b22b484b23d8efa3b11c17157c37660c36 Mon Sep 17 00:00:00 2001 From: dsyer Date: Fri, 22 Aug 2008 14:31:10 +0000 Subject: [PATCH] Retry policy needed to know if the callback was successful. --- .../batch/retry/RetryPolicy.java | 3 +- .../retry/policy/CompositeRetryPolicy.java | 8 +- .../ExceptionClassifierRetryPolicy.java | 10 +- .../batch/retry/policy/NeverRetryPolicy.java | 4 +- .../policy/RecoveryCallbackRetryPolicy.java | 13 +- .../batch/retry/policy/SimpleRetryPolicy.java | 4 +- .../retry/policy/TimeoutRetryPolicy.java | 2 +- .../batch/retry/support/RetryTemplate.java | 2 +- .../retry/policy/AlwaysRetryPolicyTests.java | 2 +- .../policy/CompositeRetryPolicyTests.java | 12 +- .../ExceptionClassifierRetryPolicyTests.java | 6 +- .../ExternalRetryIntergrationTests.java | 133 ++++++++++++++++++ .../policy/ExternalRetryPolicyTests.java | 3 +- .../retry/policy/NeverRetryPolicyTests.java | 2 +- .../policy/RecoveryRetryPolicyTests.java | 8 +- .../retry/policy/TimeoutRetryPolicyTests.java | 2 +- 16 files changed, 175 insertions(+), 39 deletions(-) create mode 100644 spring-batch-infrastructure/src/test/java/org/springframework/batch/retry/policy/ExternalRetryIntergrationTests.java diff --git a/spring-batch-infrastructure/src/main/java/org/springframework/batch/retry/RetryPolicy.java b/spring-batch-infrastructure/src/main/java/org/springframework/batch/retry/RetryPolicy.java index 18b234977..77fe1caf0 100644 --- a/spring-batch-infrastructure/src/main/java/org/springframework/batch/retry/RetryPolicy.java +++ b/spring-batch-infrastructure/src/main/java/org/springframework/batch/retry/RetryPolicy.java @@ -59,8 +59,9 @@ public interface RetryPolicy { /** * @param context a retry status created by the {@link #open(RetryCallback, RetryContext)} * method of this manager. + * @param succeeded true if the retry callback succeeded */ - void close(RetryContext context); + void close(RetryContext context, boolean succeeded); /** * Called once per retry attempt, after the callback fails. diff --git a/spring-batch-infrastructure/src/main/java/org/springframework/batch/retry/policy/CompositeRetryPolicy.java b/spring-batch-infrastructure/src/main/java/org/springframework/batch/retry/policy/CompositeRetryPolicy.java index 432013586..4138f9c6c 100644 --- a/spring-batch-infrastructure/src/main/java/org/springframework/batch/retry/policy/CompositeRetryPolicy.java +++ b/spring-batch-infrastructure/src/main/java/org/springframework/batch/retry/policy/CompositeRetryPolicy.java @@ -68,15 +68,15 @@ public class CompositeRetryPolicy extends AbstractStatelessRetryPolicy { * created. If any of them fails to close the exception is propagated (and * those later in the chain are closed before re-throwing). * - * @see org.springframework.batch.retry.RetryPolicy#close(org.springframework.batch.retry.RetryContext) + * @see org.springframework.batch.retry.RetryPolicy#close(org.springframework.batch.retry.RetryContext, boolean) */ - public void close(RetryContext context) { + public void close(RetryContext context, boolean succeeded) { RetryContext[] contexts = ((CompositeRetryContext) context).contexts; RetryPolicy[] policies = ((CompositeRetryContext) context).policies; RuntimeException exception = null; for (int i = 0; i < contexts.length; i++) { try { - policies[i].close(contexts[i]); + policies[i].close(contexts[i], succeeded); } catch (RuntimeException e) { if (exception==null) { @@ -108,7 +108,7 @@ public class CompositeRetryPolicy extends AbstractStatelessRetryPolicy { * Delegate to the policies that were in operation when the context was * created. * - * @see org.springframework.batch.retry.RetryPolicy#close(org.springframework.batch.retry.RetryContext) + * @see org.springframework.batch.retry.RetryPolicy#close(org.springframework.batch.retry.RetryContext, boolean) */ public void registerThrowable(RetryContext context, Throwable throwable) throws TerminatedRetryException { RetryContext[] contexts = ((CompositeRetryContext) context).contexts; diff --git a/spring-batch-infrastructure/src/main/java/org/springframework/batch/retry/policy/ExceptionClassifierRetryPolicy.java b/spring-batch-infrastructure/src/main/java/org/springframework/batch/retry/policy/ExceptionClassifierRetryPolicy.java index df6666c4e..4c7a3feb6 100644 --- a/spring-batch-infrastructure/src/main/java/org/springframework/batch/retry/policy/ExceptionClassifierRetryPolicy.java +++ b/spring-batch-infrastructure/src/main/java/org/springframework/batch/retry/policy/ExceptionClassifierRetryPolicy.java @@ -81,11 +81,11 @@ public class ExceptionClassifierRetryPolicy extends AbstractStatelessRetryPolicy /** * Delegate to the policy currently activated in the context. * - * @see org.springframework.batch.retry.RetryPolicy#close(org.springframework.batch.retry.RetryContext) + * @see org.springframework.batch.retry.RetryPolicy#close(org.springframework.batch.retry.RetryContext, boolean) */ - public void close(RetryContext context) { + public void close(RetryContext context, boolean succeeded) { RetryPolicy policy = (RetryPolicy) context; - policy.close(context); + policy.close(context, succeeded); } /** @@ -146,10 +146,10 @@ public class ExceptionClassifierRetryPolicy extends AbstractStatelessRetryPolicy return policy.shouldRethrow(context); } - public void close(RetryContext context) { + public void close(RetryContext context, boolean succeeded) { // Only close those policies that have been used (opened): for (RetryPolicy policy : contexts.keySet()) { - policy.close(getContext(policy)); + policy.close(getContext(policy), succeeded); } } diff --git a/spring-batch-infrastructure/src/main/java/org/springframework/batch/retry/policy/NeverRetryPolicy.java b/spring-batch-infrastructure/src/main/java/org/springframework/batch/retry/policy/NeverRetryPolicy.java index d661bfead..b18119183 100644 --- a/spring-batch-infrastructure/src/main/java/org/springframework/batch/retry/policy/NeverRetryPolicy.java +++ b/spring-batch-infrastructure/src/main/java/org/springframework/batch/retry/policy/NeverRetryPolicy.java @@ -45,9 +45,9 @@ public class NeverRetryPolicy extends AbstractStatelessRetryPolicy { /** * Do nothing. * - * @see org.springframework.batch.retry.RetryPolicy#close(org.springframework.batch.retry.RetryContext) + * @see org.springframework.batch.retry.RetryPolicy#close(org.springframework.batch.retry.RetryContext, boolean) */ - public void close(RetryContext context) { + public void close(RetryContext context, boolean succeeded) { // no-op } diff --git a/spring-batch-infrastructure/src/main/java/org/springframework/batch/retry/policy/RecoveryCallbackRetryPolicy.java b/spring-batch-infrastructure/src/main/java/org/springframework/batch/retry/policy/RecoveryCallbackRetryPolicy.java index 518f0ba94..0cddeef89 100644 --- a/spring-batch-infrastructure/src/main/java/org/springframework/batch/retry/policy/RecoveryCallbackRetryPolicy.java +++ b/spring-batch-infrastructure/src/main/java/org/springframework/batch/retry/policy/RecoveryCallbackRetryPolicy.java @@ -87,10 +87,10 @@ public class RecoveryCallbackRetryPolicy extends AbstractStatefulRetryPolicy { /** * Delegates to the delegate context. * - * @see org.springframework.batch.retry.RetryPolicy#close(org.springframework.batch.retry.RetryContext) + * @see org.springframework.batch.retry.RetryPolicy#close(org.springframework.batch.retry.RetryContext, boolean) */ - public void close(RetryContext context) { - ((RetryPolicy) context).close(context); + public void close(RetryContext context, boolean succeeded) { + ((RetryPolicy) context).close(context, succeeded); } /** @@ -156,8 +156,11 @@ public class RecoveryCallbackRetryPolicy extends AbstractStatefulRetryPolicy { return delegate.canRetry(this.delegateContext); } - public void close(RetryContext context) { - delegate.close(this.delegateContext); + public void close(RetryContext context, boolean succeeded) { + if (succeeded) { + retryContextCache.remove(key); + delegate.close(this.delegateContext, succeeded); + } } public RetryContext open(RetryCallback callback, RetryContext parent) { diff --git a/spring-batch-infrastructure/src/main/java/org/springframework/batch/retry/policy/SimpleRetryPolicy.java b/spring-batch-infrastructure/src/main/java/org/springframework/batch/retry/policy/SimpleRetryPolicy.java index 703033f53..22383f74d 100644 --- a/spring-batch-infrastructure/src/main/java/org/springframework/batch/retry/policy/SimpleRetryPolicy.java +++ b/spring-batch-infrastructure/src/main/java/org/springframework/batch/retry/policy/SimpleRetryPolicy.java @@ -119,9 +119,9 @@ public class SimpleRetryPolicy extends AbstractStatelessRetryPolicy { } /** - * @see org.springframework.batch.retry.RetryPolicy#close(RetryContext) + * @see org.springframework.batch.retry.RetryPolicy#close(RetryContext, boolean) */ - public void close(RetryContext status) { + public void close(RetryContext status, boolean succeeded) { } /** diff --git a/spring-batch-infrastructure/src/main/java/org/springframework/batch/retry/policy/TimeoutRetryPolicy.java b/spring-batch-infrastructure/src/main/java/org/springframework/batch/retry/policy/TimeoutRetryPolicy.java index 41641cd90..3cc56c64e 100644 --- a/spring-batch-infrastructure/src/main/java/org/springframework/batch/retry/policy/TimeoutRetryPolicy.java +++ b/spring-batch-infrastructure/src/main/java/org/springframework/batch/retry/policy/TimeoutRetryPolicy.java @@ -56,7 +56,7 @@ public class TimeoutRetryPolicy extends AbstractStatelessRetryPolicy { return ((TimeoutRetryContext) context).isAlive(); } - public void close(RetryContext context) { + public void close(RetryContext context, boolean succeeded) { } public RetryContext open(RetryCallback callback, RetryContext parent) { diff --git a/spring-batch-infrastructure/src/main/java/org/springframework/batch/retry/support/RetryTemplate.java b/spring-batch-infrastructure/src/main/java/org/springframework/batch/retry/support/RetryTemplate.java index 27120fc4e..d763fa9aa 100644 --- a/spring-batch-infrastructure/src/main/java/org/springframework/batch/retry/support/RetryTemplate.java +++ b/spring-batch-infrastructure/src/main/java/org/springframework/batch/retry/support/RetryTemplate.java @@ -205,7 +205,7 @@ public class RetryTemplate implements RetryOperations { } finally { - retryPolicy.close(context); + retryPolicy.close(context, lastException==null); doCloseInterceptors(callback, context, lastException); RetrySynchronizationManager.clear(); } diff --git a/spring-batch-infrastructure/src/test/java/org/springframework/batch/retry/policy/AlwaysRetryPolicyTests.java b/spring-batch-infrastructure/src/test/java/org/springframework/batch/retry/policy/AlwaysRetryPolicyTests.java index 48ae1615e..91e913025 100644 --- a/spring-batch-infrastructure/src/test/java/org/springframework/batch/retry/policy/AlwaysRetryPolicyTests.java +++ b/spring-batch-infrastructure/src/test/java/org/springframework/batch/retry/policy/AlwaysRetryPolicyTests.java @@ -29,7 +29,7 @@ public class AlwaysRetryPolicyTests extends TestCase { assertTrue(policy.canRetry(context)); policy.registerThrowable(context, null); assertTrue(policy.canRetry(context)); - policy.close(context); + policy.close(context, true); assertTrue(policy.canRetry(context)); } diff --git a/spring-batch-infrastructure/src/test/java/org/springframework/batch/retry/policy/CompositeRetryPolicyTests.java b/spring-batch-infrastructure/src/test/java/org/springframework/batch/retry/policy/CompositeRetryPolicyTests.java index ea315c9df..b2b8464c8 100644 --- a/spring-batch-infrastructure/src/test/java/org/springframework/batch/retry/policy/CompositeRetryPolicyTests.java +++ b/spring-batch-infrastructure/src/test/java/org/springframework/batch/retry/policy/CompositeRetryPolicyTests.java @@ -78,17 +78,17 @@ public class CompositeRetryPolicyTests extends TestCase { final List list = new ArrayList(); CompositeRetryPolicy policy = new CompositeRetryPolicy(); policy.setPolicies(new RetryPolicy[] { new MockRetryPolicySupport() { - public void close(RetryContext context) { + public void close(RetryContext context, boolean succeeded) { list.add("1"); } }, new MockRetryPolicySupport() { - public void close(RetryContext context) { + public void close(RetryContext context, boolean succeeded) { list.add("2"); } } }); RetryContext context = policy.open(null, null); assertNotNull(context); - policy.close(context); + policy.close(context, true); assertEquals(2, list.size()); } @@ -96,19 +96,19 @@ public class CompositeRetryPolicyTests extends TestCase { final List list = new ArrayList(); CompositeRetryPolicy policy = new CompositeRetryPolicy(); policy.setPolicies(new RetryPolicy[] { new MockRetryPolicySupport() { - public void close(RetryContext context) { + public void close(RetryContext context, boolean succeeded) { list.add("1"); throw new RuntimeException("Pah!"); } }, new MockRetryPolicySupport() { - public void close(RetryContext context) { + public void close(RetryContext context, boolean succeeded) { list.add("2"); } } }); RetryContext context = policy.open(null, null); assertNotNull(context); try { - policy.close(context); + policy.close(context, true); fail("Expected RuntimeException"); } catch (RuntimeException e) { assertEquals("Pah!", e.getMessage()); diff --git a/spring-batch-infrastructure/src/test/java/org/springframework/batch/retry/policy/ExceptionClassifierRetryPolicyTests.java b/spring-batch-infrastructure/src/test/java/org/springframework/batch/retry/policy/ExceptionClassifierRetryPolicyTests.java index 81ae4e98c..b88f81b53 100644 --- a/spring-batch-infrastructure/src/test/java/org/springframework/batch/retry/policy/ExceptionClassifierRetryPolicyTests.java +++ b/spring-batch-infrastructure/src/test/java/org/springframework/batch/retry/policy/ExceptionClassifierRetryPolicyTests.java @@ -106,7 +106,7 @@ public class ExceptionClassifierRetryPolicyTests extends TestCase { public void testClose() throws Exception { policy.setPolicyMap(Collections.singletonMap(ExceptionClassifierSupport.DEFAULT, (RetryPolicy) new MockRetryPolicySupport() { - public void close(RetryContext context) { + public void close(RetryContext context, boolean succeeded) { count++; } })); @@ -114,12 +114,12 @@ public class ExceptionClassifierRetryPolicyTests extends TestCase { // The mapped (child) policy hasn't been used yet, so if we close now // we don't incur the possible expense of ceating the child context. - policy.close(context); + policy.close(context, true); assertEquals(0, count); // not classified yet // This forces a child context to be created and the child policy is // then closed policy.registerThrowable(context, new IllegalStateException()); - policy.close(context); + policy.close(context, true); assertEquals(1, count); // now classified } diff --git a/spring-batch-infrastructure/src/test/java/org/springframework/batch/retry/policy/ExternalRetryIntergrationTests.java b/spring-batch-infrastructure/src/test/java/org/springframework/batch/retry/policy/ExternalRetryIntergrationTests.java new file mode 100644 index 000000000..3dee5c5e4 --- /dev/null +++ b/spring-batch-infrastructure/src/test/java/org/springframework/batch/retry/policy/ExternalRetryIntergrationTests.java @@ -0,0 +1,133 @@ +/* + * Copyright 2006-2007 the original author or authors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.springframework.batch.retry.policy; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertNotNull; +import static org.junit.Assert.assertNull; +import static org.junit.Assert.assertTrue; +import static org.junit.Assert.fail; + +import org.junit.Test; +import org.springframework.batch.retry.RetryCallback; +import org.springframework.batch.retry.RetryContext; +import org.springframework.batch.retry.callback.RecoveryRetryCallback; +import org.springframework.batch.retry.support.RetryTemplate; + +/** + * @author Dave Syer + * + */ +public class ExternalRetryIntergrationTests { + + @Test + public void testExternalRetryWithFailAndNoRetry() throws Exception { + MockRetryCallback callback = new MockRetryCallback(); + + RecoveryRetryCallback recoveryCallback = new RecoveryRetryCallback("foo", callback); + + RetryTemplate retryTemplate = new RetryTemplate(); + RecoveryCallbackRetryPolicy retryPolicy = new RecoveryCallbackRetryPolicy(new SimpleRetryPolicy(1)); + MapRetryContextCache cache = new MapRetryContextCache(); + retryPolicy.setRetryContextCache(cache); + retryTemplate.setRetryPolicy(retryPolicy); + + assertFalse(cache.containsKey("foo")); + + Object result = "start_foo"; + try { + result = retryTemplate.execute(recoveryCallback); + // The first failed attempt we expect to retry... + fail("Expected RuntimeException"); + } + catch (RuntimeException e) { + assertNull(e.getMessage()); + } + + assertTrue(cache.containsKey("foo")); + + try { + result = retryTemplate.execute(recoveryCallback); + // We always get a second attempt... + } + catch (IllegalArgumentException e) { + // This is now the "exhausted" message: + assertNotNull(e.getMessage()); + // But if template is external we should + // swallow the exception when retry is impossible. + fail("Did not expect IllegalArgumentException"); + } + + assertFalse(cache.containsKey("foo")); + + // Callback is called once: the recovery path should be called in + // handleRetryExhausted (so not in this test)... + assertEquals(1, callback.attempts); + assertEquals(null, result); + } + + @Test + public void testExternalRetryWithSuccessOnRetry() throws Exception { + MockRetryCallback callback = new MockRetryCallback(); + + RecoveryRetryCallback recoveryCallback = new RecoveryRetryCallback("foo", callback); + + RetryTemplate retryTemplate = new RetryTemplate(); + RecoveryCallbackRetryPolicy retryPolicy = new RecoveryCallbackRetryPolicy(new SimpleRetryPolicy(2)); + MapRetryContextCache cache = new MapRetryContextCache(); + retryPolicy.setRetryContextCache(cache); + retryTemplate.setRetryPolicy(retryPolicy); + + assertFalse(cache.containsKey("foo")); + + Object result = "start_foo"; + try { + result = retryTemplate.execute(recoveryCallback); + // The first failed attempt we expect to retry... + fail("Expected RuntimeException"); + } + catch (RuntimeException e) { + assertNull(e.getMessage()); + } + + assertTrue(cache.containsKey("foo")); + + result = retryTemplate.execute(recoveryCallback); + + assertFalse(cache.containsKey("foo")); + + assertEquals(2, callback.attempts); + assertEquals("bar", result); + } + + /** + * @author Dave Syer + * + */ + private final class MockRetryCallback implements RetryCallback { + int attempts = 0; + + public Object doWithRetry(RetryContext context) throws Throwable { + attempts++; + if (attempts < 2) { + throw new RuntimeException(); + } + return "bar"; + } + } + +} diff --git a/spring-batch-infrastructure/src/test/java/org/springframework/batch/retry/policy/ExternalRetryPolicyTests.java b/spring-batch-infrastructure/src/test/java/org/springframework/batch/retry/policy/ExternalRetryPolicyTests.java index aedc22e03..e71fa57bf 100644 --- a/spring-batch-infrastructure/src/test/java/org/springframework/batch/retry/policy/ExternalRetryPolicyTests.java +++ b/spring-batch-infrastructure/src/test/java/org/springframework/batch/retry/policy/ExternalRetryPolicyTests.java @@ -94,7 +94,6 @@ public class ExternalRetryPolicyTests extends TestCase { } catch (IllegalArgumentException e) { // Expected - System.err.println(e.getMessage()); assertTrue(Pattern.matches(".*not.*Throwable.*", e.getMessage())); } @@ -192,7 +191,7 @@ public class ExternalRetryPolicyTests extends TestCase { return attempts < retryLimit; } - public void close(RetryContext context) { + public void close(RetryContext context, boolean succeeded) { // do nothing } diff --git a/spring-batch-infrastructure/src/test/java/org/springframework/batch/retry/policy/NeverRetryPolicyTests.java b/spring-batch-infrastructure/src/test/java/org/springframework/batch/retry/policy/NeverRetryPolicyTests.java index 2c1be6d30..221ea71d6 100644 --- a/spring-batch-infrastructure/src/test/java/org/springframework/batch/retry/policy/NeverRetryPolicyTests.java +++ b/spring-batch-infrastructure/src/test/java/org/springframework/batch/retry/policy/NeverRetryPolicyTests.java @@ -31,7 +31,7 @@ public class NeverRetryPolicyTests extends TestCase { assertTrue(policy.canRetry(context)); policy.registerThrowable(context, null); assertFalse(policy.canRetry(context)); - policy.close(context); + policy.close(context, true); assertFalse(policy.canRetry(context)); } diff --git a/spring-batch-infrastructure/src/test/java/org/springframework/batch/retry/policy/RecoveryRetryPolicyTests.java b/spring-batch-infrastructure/src/test/java/org/springframework/batch/retry/policy/RecoveryRetryPolicyTests.java index ac62ab0e5..2fc26c4b0 100644 --- a/spring-batch-infrastructure/src/test/java/org/springframework/batch/retry/policy/RecoveryRetryPolicyTests.java +++ b/spring-batch-infrastructure/src/test/java/org/springframework/batch/retry/policy/RecoveryRetryPolicyTests.java @@ -109,7 +109,7 @@ public class RecoveryRetryPolicyTests extends TestCase { assertNotNull(context); policy.registerThrowable(context, new Exception()); assertFalse(policy.canRetry(context)); - policy.close(context); + policy.close(context, true); // still can't retry, even if policy is closed // (not that this would happen in practice)... assertFalse(policy.canRetry(context)); @@ -129,14 +129,14 @@ public class RecoveryRetryPolicyTests extends TestCase { assertNotNull(context); policy.registerThrowable(context, new Exception()); assertTrue(policy.canRetry(context)); - policy.close(context); + policy.close(context, false); // Second call... context = policy.open(callback, null); assertNotNull(context); policy.registerThrowable(context, new Exception()); assertFalse(policy.canRetry(context)); - policy.close(context); + policy.close(context, true); } @@ -229,7 +229,7 @@ public class RecoveryRetryPolicyTests extends TestCase { // False before close... assertFalse(policy.canRetry(context)); - policy.close(context); + policy.close(context, true); Object result = policy.handleRetryExhausted(context); assertNull(result); // default result is null diff --git a/spring-batch-infrastructure/src/test/java/org/springframework/batch/retry/policy/TimeoutRetryPolicyTests.java b/spring-batch-infrastructure/src/test/java/org/springframework/batch/retry/policy/TimeoutRetryPolicyTests.java index ac1f2f91f..6b3196f8f 100644 --- a/spring-batch-infrastructure/src/test/java/org/springframework/batch/retry/policy/TimeoutRetryPolicyTests.java +++ b/spring-batch-infrastructure/src/test/java/org/springframework/batch/retry/policy/TimeoutRetryPolicyTests.java @@ -30,7 +30,7 @@ public class TimeoutRetryPolicyTests extends TestCase { assertTrue(policy.canRetry(context)); Thread.sleep(200); assertFalse(policy.canRetry(context)); - policy.close(context); + policy.close(context, true); } public void testRetryCount() throws Exception {