From 5279c0a0368628276beff6eb16f8762efaaa6245 Mon Sep 17 00:00:00 2001 From: dsyer Date: Fri, 10 Jul 2009 11:45:18 +0000 Subject: [PATCH] RESOLVED - issue BATCH-1334: No-rollback for non-skippable exceptions. --- .../item/FaultTolerantChunkProcessor.java | 23 +++++---- .../step/item/FaultTolerantChunkProvider.java | 24 +++++++-- .../item/FaultTolerantStepFactoryBean.java | 30 +++++++---- .../batch/core/AbstractExceptionTests.java | 10 +--- .../core/AbstractExceptionWithCauseTests.java | 33 ++++++++++++ .../FaultTolerantExceptionClassesTests.java | 50 ++++++++++++++----- ...tTolerantStepFactoryBeanRollbackTests.java | 2 +- ...rceRollbackForWriteSkipExceptionTests.java | 35 +++++++++++++ .../skip/NonSkippableReadExceptionTests.java | 35 +++++++++++++ .../skip/NonSkippableWriteExceptionTests.java | 35 +++++++++++++ ...tTolerantExceptionClassesTests-context.xml | 18 ++++++- 11 files changed, 248 insertions(+), 47 deletions(-) create mode 100644 spring-batch-core/src/test/java/org/springframework/batch/core/AbstractExceptionWithCauseTests.java create mode 100644 spring-batch-core/src/test/java/org/springframework/batch/core/step/item/ForceRollbackForWriteSkipExceptionTests.java create mode 100644 spring-batch-core/src/test/java/org/springframework/batch/core/step/skip/NonSkippableReadExceptionTests.java create mode 100644 spring-batch-core/src/test/java/org/springframework/batch/core/step/skip/NonSkippableWriteExceptionTests.java diff --git a/spring-batch-core/src/main/java/org/springframework/batch/core/step/item/FaultTolerantChunkProcessor.java b/spring-batch-core/src/main/java/org/springframework/batch/core/step/item/FaultTolerantChunkProcessor.java index d5742ecf6..6330fb532 100755 --- a/spring-batch-core/src/main/java/org/springframework/batch/core/step/item/FaultTolerantChunkProcessor.java +++ b/spring-batch-core/src/main/java/org/springframework/batch/core/step/item/FaultTolerantChunkProcessor.java @@ -288,15 +288,13 @@ public class FaultTolerantChunkProcessor extends SimpleChunkProcessor extends SimpleChunkProcessor extends SimpleChunkProcessor extends SimpleChunkProvider { private SkipPolicy skipPolicy = new LimitCheckingItemSkipPolicy(0); + private Classifier rollbackClassifier = new BinaryExceptionClassifier(true); + public FaultTolerantChunkProvider(ItemReader itemReader, RepeatOperations repeatOperations) { super(itemReader, repeatOperations); } @@ -46,6 +50,17 @@ public class FaultTolerantChunkProvider extends SimpleChunkProvider { this.skipPolicy = SkipPolicy; } + /** + * Classifier to determine whether exceptions have been marked as + * no-rollback (as opposed to skippable). If ecnounterd they are simply + * ignored, unless also skippable. + * + * @param rollbackClassifier the rollback classifier to set + */ + public void setRollbackClassifier(Classifier rollbackClassifier) { + this.rollbackClassifier = rollbackClassifier; + } + @Override protected I read(StepContribution contribution, Chunk chunk) throws Exception { while (true) { @@ -62,7 +77,10 @@ public class FaultTolerantChunkProvider extends SimpleChunkProvider { logger.debug("Skipping failed input", e); } else { - throw new NonSkippableReadException("Non-skippable exception during read", e); + if (rollbackClassifier.classify(e)) { + throw new NonSkippableReadException("Non-skippable exception during read", e); + } + logger.debug("No-rollback for non-skippable exception (ignored)", e); } } diff --git a/spring-batch-core/src/main/java/org/springframework/batch/core/step/item/FaultTolerantStepFactoryBean.java b/spring-batch-core/src/main/java/org/springframework/batch/core/step/item/FaultTolerantStepFactoryBean.java index a325be1bc..10bfa5f8d 100755 --- a/spring-batch-core/src/main/java/org/springframework/batch/core/step/item/FaultTolerantStepFactoryBean.java +++ b/spring-batch-core/src/main/java/org/springframework/batch/core/step/item/FaultTolerantStepFactoryBean.java @@ -35,6 +35,7 @@ import org.springframework.batch.item.ItemReader; import org.springframework.batch.item.ItemStream; import org.springframework.batch.repeat.RepeatOperations; import org.springframework.batch.repeat.support.RepeatTemplate; +import org.springframework.batch.retry.ExhaustedRetryException; import org.springframework.batch.retry.RetryException; import org.springframework.batch.retry.RetryListener; import org.springframework.batch.retry.RetryPolicy; @@ -246,18 +247,25 @@ public class FaultTolerantStepFactoryBean extends SimpleStepFactoryBean classifier = new BinaryExceptionClassifier(noRollbackExceptionClasses, false); - // Try to avoid pathological cases where we cannot froce a rollback - // where necessary (should be pretty uncommon): - if (!classifier.classify(new ForceRollbackForWriteSkipException("test", new RuntimeException()))) { + // Try to avoid pathological cases where we cannot force a rollback + // (should be pretty uncommon): + if (!classifier.classify(new ForceRollbackForWriteSkipException("test", new RuntimeException())) + || !classifier.classify(new ExhaustedRetryException("test"))) { + final Classifier binary = classifier; + + Collection> types = new HashSet>(); + types.add(ForceRollbackForWriteSkipException.class); + types.add(ExhaustedRetryException.class); + final Classifier panic = new BinaryExceptionClassifier(types, true); + classifier = new Classifier() { public Boolean classify(Throwable classifiable) { - if (ForceRollbackForWriteSkipException.class.isAssignableFrom(classifiable.getClass())) { - return true; - } - return binary.classify(classifiable); + // Rollback if either the user's list or our own applies + return panic.classify(classifiable) || binary.classify(classifiable); } }; + } return classifier; @@ -338,6 +346,7 @@ public class FaultTolerantStepFactoryBean extends SimpleStepFactoryBean chunkProvider = new FaultTolerantChunkProvider(getItemReader(), getChunkOperations()); chunkProvider.setSkipPolicy(readSkipPolicy); + chunkProvider.setRollbackClassifier(getRollbackClassifier()); return chunkProvider; @@ -349,14 +358,14 @@ public class FaultTolerantStepFactoryBean extends SimpleStepFactoryBean configureChunkProcessor() { - SkipPolicy writeSkipPolicy = new LimitCheckingItemSkipPolicy(skipLimit, getSkippableExceptionClasses(), - fatalExceptionClasses); BatchRetryTemplate batchRetryTemplate = configureRetry(); FaultTolerantChunkProcessor chunkProcessor = new FaultTolerantChunkProcessor(getItemProcessor(), getItemWriter(), batchRetryTemplate); chunkProcessor.setBuffering(!isReaderTransactionalQueue()); + + SkipPolicy writeSkipPolicy = new LimitCheckingItemSkipPolicy(skipLimit, getSkippableExceptionClasses(), fatalExceptionClasses); chunkProcessor.setWriteSkipPolicy(writeSkipPolicy); chunkProcessor.setProcessSkipPolicy(writeSkipPolicy); chunkProcessor.setRollbackClassifier(getRollbackClassifier()); @@ -372,7 +381,6 @@ public class FaultTolerantStepFactoryBean extends SimpleStepFactoryBean> getSkippableExceptionClasses() { HashSet> set = new HashSet>(skippableExceptionClasses); - set.addAll(noRollbackExceptionClasses); set.add(ForceRollbackForWriteSkipException.class); return set; } @@ -386,6 +394,8 @@ public class FaultTolerantStepFactoryBean extends SimpleStepFactoryBean> set = new HashSet>(retryableExceptionClasses); set.add(ForceRollbackForWriteSkipException.class); + // set.addAll(noRollbackExceptionClasses); // should only be + // retryable on write simpleRetryPolicy.setRetryableExceptionClasses(set); retryPolicy = simpleRetryPolicy; } diff --git a/spring-batch-core/src/test/java/org/springframework/batch/core/AbstractExceptionTests.java b/spring-batch-core/src/test/java/org/springframework/batch/core/AbstractExceptionTests.java index ee9b02847..a6dce689c 100644 --- a/spring-batch-core/src/test/java/org/springframework/batch/core/AbstractExceptionTests.java +++ b/spring-batch-core/src/test/java/org/springframework/batch/core/AbstractExceptionTests.java @@ -20,7 +20,7 @@ import static org.junit.Assert.assertEquals; import org.junit.Test; -public abstract class AbstractExceptionTests { +public abstract class AbstractExceptionTests extends AbstractExceptionWithCauseTests { @Test public void testExceptionString() throws Exception { @@ -28,14 +28,6 @@ public abstract class AbstractExceptionTests { assertEquals("foo", exception.getMessage()); } - @Test - public void testExceptionStringThrowable() throws Exception { - Exception exception = getException("foo", new IllegalStateException()); - assertEquals("foo", exception.getMessage().substring(0, 3)); - } - public abstract Exception getException(String msg) throws Exception; - public abstract Exception getException(String msg, Throwable t) throws Exception; - } diff --git a/spring-batch-core/src/test/java/org/springframework/batch/core/AbstractExceptionWithCauseTests.java b/spring-batch-core/src/test/java/org/springframework/batch/core/AbstractExceptionWithCauseTests.java new file mode 100644 index 000000000..10ae0e21c --- /dev/null +++ b/spring-batch-core/src/test/java/org/springframework/batch/core/AbstractExceptionWithCauseTests.java @@ -0,0 +1,33 @@ +/* + * 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.core; + +import static org.junit.Assert.assertEquals; + +import org.junit.Test; + +public abstract class AbstractExceptionWithCauseTests { + + @Test + public void testExceptionStringThrowable() throws Exception { + Exception exception = getException("foo", new IllegalStateException()); + assertEquals("foo", exception.getMessage().substring(0, 3)); + } + + public abstract Exception getException(String msg, Throwable t) throws Exception; + +} diff --git a/spring-batch-core/src/test/java/org/springframework/batch/core/step/item/FaultTolerantExceptionClassesTests.java b/spring-batch-core/src/test/java/org/springframework/batch/core/step/item/FaultTolerantExceptionClassesTests.java index 0b1af4555..a4a698dab 100644 --- a/spring-batch-core/src/test/java/org/springframework/batch/core/step/item/FaultTolerantExceptionClassesTests.java +++ b/spring-batch-core/src/test/java/org/springframework/batch/core/step/item/FaultTolerantExceptionClassesTests.java @@ -118,9 +118,9 @@ public class FaultTolerantExceptionClassesTests implements ApplicationContextAwa writer.setExceptionType(Exception.class); StepExecution stepExecution = launchStep("skippableFatalStep"); assertEquals(BatchStatus.FAILED, stepExecution.getStatus()); - // BATCH-1327: + // BATCH-1327: assertEquals("[1, 2, 3]", writer.getWritten().toString()); - // BATCH-1327: + // BATCH-1327: assertEquals("[]", writer.getCommitted().toString()); } @@ -163,10 +163,13 @@ public class FaultTolerantExceptionClassesTests implements ApplicationContextAwa @Test public void testRetryableFatal() throws Exception { + // User wants all exceptions to be retried, but only some are skippable + // FatalRuntimeException is not skippable, but is a subclass of another + // skippable writer.setExceptionType(FatalRuntimeException.class); StepExecution stepExecution = launchStep("retryable"); assertEquals(BatchStatus.FAILED, stepExecution.getStatus()); - // TODO BATCH-1318: assertEquals("[1, 2, 3, 1, 2, 3, 1, 2, 3]", + // TODO BATCH-1333: assertEquals("[1, 2, 3, 1, 2, 3, 1, 2, 3]", // writer.getWritten().toString()); assertEquals("[]", writer.getCommitted().toString()); } @@ -195,32 +198,51 @@ public class FaultTolerantExceptionClassesTests implements ApplicationContextAwa writer.setExceptionType(FatalException.class); StepExecution stepExecution = launchStep("retryable"); assertEquals(BatchStatus.FAILED, stepExecution.getStatus()); - // TODO BATCH-1318: assertEquals("[1, 2, 3, 1, 2, 3, 1, 2, 3]", + // TODO BATCH-1333: assertEquals("[1, 2, 3, 1, 2, 3, 1, 2, 3]", // writer.getWritten().toString()); assertEquals("[]", writer.getCommitted().toString()); } @Test public void testNoRollbackDefaultRollbackException() throws Exception { - writer.setExceptionType(RuntimeException.class); + // Exception is neither no-rollback nor skippable + writer.setExceptionType(Exception.class); StepExecution stepExecution = launchStep("noRollbackDefault"); assertEquals(BatchStatus.FAILED, stepExecution.getStatus()); // BATCH-1318: assertEquals("[1, 2, 3]", writer.getWritten().toString()); // BATCH-1318: assertEquals("[]", writer.getCommitted().toString()); + assertEquals(0, stepExecution.getWriteSkipCount()); } @Test public void testNoRollbackDefaultNoRollbackException() throws Exception { - writer.setExceptionType(SkippableRuntimeException.class); + // Exception is no-rollback and not skippable + writer.setExceptionType(IllegalStateException.class); StepExecution stepExecution = launchStep("noRollbackDefault"); assertNotNull(stepExecution); - // TODO BATCH-1318: assertEquals(BatchStatus.FAILED, stepExecution.getStatus()); - // TODO BATCH-1318: assertEquals("[1, 2, 3]", - // writer.getWritten().toString()); - // TODO BATCH-1318: assertEquals("[1, 2, 3]", - // writer.getCommitted().toString()); + assertEquals(BatchStatus.COMPLETED, stepExecution.getStatus()); + // BATCH-1334: + assertEquals("[1, 2, 3, 1, 2, 3, 4]", writer.getWritten().toString()); + // BATCH-1334: + assertEquals("[1, 2, 3, 4]", writer.getCommitted().toString()); + // BATCH-1334: + assertEquals(0, stepExecution.getWriteSkipCount()); + } + + @Test + public void testNoRollbackPathology() throws Exception { + // Exception is neither no-rollback nor skippable and no-rollback is + // RuntimeException (potentially pathological because other obviously + // rollback signalling Exceptions also extend RuntimeException) + writer.setExceptionType(Exception.class); + StepExecution stepExecution = launchStep("noRollbackPathology"); + assertEquals(BatchStatus.FAILED, stepExecution.getStatus()); + // BATCH-1335: + assertEquals("[1, 2, 3]", writer.getWritten().toString()); + // BATCH-1335: + assertEquals("[]", writer.getCommitted().toString()); } @Test @@ -238,8 +260,10 @@ public class FaultTolerantExceptionClassesTests implements ApplicationContextAwa StepExecution stepExecution = launchStep("noRollbackSkippable"); assertEquals(BatchStatus.COMPLETED, stepExecution.getStatus()); assertEquals("[1, 2, 3, 1, 2, 3, 4]", writer.getWritten().toString()); - // TODO BATCH-1332: assertEquals("[1, 2, 4]", writer.getCommitted().toString()); - // Skipped but also committed! + // BATCH-1332: + assertEquals("[1, 2, 3, 4]", writer.getCommitted().toString()); + // TODO BATCH-1334: + // Not skipped but also committed (because it was marked as no-rollback) assertEquals(1, stepExecution.getWriteSkipCount()); } diff --git a/spring-batch-core/src/test/java/org/springframework/batch/core/step/item/FaultTolerantStepFactoryBeanRollbackTests.java b/spring-batch-core/src/test/java/org/springframework/batch/core/step/item/FaultTolerantStepFactoryBeanRollbackTests.java index 716e256ce..b03ec641e 100644 --- a/spring-batch-core/src/test/java/org/springframework/batch/core/step/item/FaultTolerantStepFactoryBeanRollbackTests.java +++ b/spring-batch-core/src/test/java/org/springframework/batch/core/step/item/FaultTolerantStepFactoryBeanRollbackTests.java @@ -152,7 +152,7 @@ public class FaultTolerantStepFactoryBeanRollbackTests { step.execute(stepExecution); assertEquals(BatchStatus.COMPLETED, stepExecution.getStatus()); - assertEquals(2, stepExecution.getSkipCount()); + assertEquals(0, stepExecution.getSkipCount()); assertEquals(0, stepExecution.getRollbackCount()); } diff --git a/spring-batch-core/src/test/java/org/springframework/batch/core/step/item/ForceRollbackForWriteSkipExceptionTests.java b/spring-batch-core/src/test/java/org/springframework/batch/core/step/item/ForceRollbackForWriteSkipExceptionTests.java new file mode 100644 index 000000000..f8cc0e9f6 --- /dev/null +++ b/spring-batch-core/src/test/java/org/springframework/batch/core/step/item/ForceRollbackForWriteSkipExceptionTests.java @@ -0,0 +1,35 @@ +/* + * 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.core.step.item; + +import org.springframework.batch.core.AbstractExceptionWithCauseTests; + + +/** + * @author Dave Syer + * + */ +public class ForceRollbackForWriteSkipExceptionTests extends AbstractExceptionWithCauseTests { + + /* (non-Javadoc) + * @see org.springframework.batch.core.listener.AbstractDoubleExceptionTests#getException(java.lang.String, java.lang.RuntimeException, java.lang.Throwable) + */ + @Override + public Exception getException(String msg, Throwable e) throws Exception { + return new ForceRollbackForWriteSkipException(msg, e); + } + +} diff --git a/spring-batch-core/src/test/java/org/springframework/batch/core/step/skip/NonSkippableReadExceptionTests.java b/spring-batch-core/src/test/java/org/springframework/batch/core/step/skip/NonSkippableReadExceptionTests.java new file mode 100644 index 000000000..586bd31e7 --- /dev/null +++ b/spring-batch-core/src/test/java/org/springframework/batch/core/step/skip/NonSkippableReadExceptionTests.java @@ -0,0 +1,35 @@ +/* + * 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.core.step.skip; + +import org.springframework.batch.core.AbstractExceptionWithCauseTests; + + +/** + * @author Dave Syer + * + */ +public class NonSkippableReadExceptionTests extends AbstractExceptionWithCauseTests { + + /* (non-Javadoc) + * @see org.springframework.batch.core.listener.AbstractDoubleExceptionTests#getException(java.lang.String, java.lang.RuntimeException, java.lang.Throwable) + */ + @Override + public Exception getException(String msg, Throwable e) throws Exception { + return new NonSkippableReadException(msg, e); + } + +} diff --git a/spring-batch-core/src/test/java/org/springframework/batch/core/step/skip/NonSkippableWriteExceptionTests.java b/spring-batch-core/src/test/java/org/springframework/batch/core/step/skip/NonSkippableWriteExceptionTests.java new file mode 100644 index 000000000..3a2028350 --- /dev/null +++ b/spring-batch-core/src/test/java/org/springframework/batch/core/step/skip/NonSkippableWriteExceptionTests.java @@ -0,0 +1,35 @@ +/* + * 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.core.step.skip; + +import org.springframework.batch.core.AbstractExceptionWithCauseTests; + + +/** + * @author Dave Syer + * + */ +public class NonSkippableWriteExceptionTests extends AbstractExceptionWithCauseTests { + + /* (non-Javadoc) + * @see org.springframework.batch.core.listener.AbstractDoubleExceptionTests#getException(java.lang.String, java.lang.RuntimeException, java.lang.Throwable) + */ + @Override + public Exception getException(String msg, Throwable e) throws Exception { + return new NonSkippableWriteException(msg, e); + } + +} diff --git a/spring-batch-core/src/test/resources/org/springframework/batch/core/step/item/FaultTolerantExceptionClassesTests-context.xml b/spring-batch-core/src/test/resources/org/springframework/batch/core/step/item/FaultTolerantExceptionClassesTests-context.xml index e0ace03f2..718ac5c2b 100644 --- a/spring-batch-core/src/test/resources/org/springframework/batch/core/step/item/FaultTolerantExceptionClassesTests-context.xml +++ b/spring-batch-core/src/test/resources/org/springframework/batch/core/step/item/FaultTolerantExceptionClassesTests-context.xml @@ -59,11 +59,25 @@ - org.springframework.batch.core.step.item.FatalRuntimeException + + java.util.FormatterClosedException - org.springframework.batch.core.step.item.SkippableRuntimeException + java.lang.IllegalStateException + + + + + + + + + java.lang.IllegalStateException + + + + java.lang.RuntimeException