From 4cc2e03fbd05638de1777aecc6260fb326dfe642 Mon Sep 17 00:00:00 2001 From: robokaso Date: Thu, 20 Mar 2008 10:04:27 +0000 Subject: [PATCH] RESOLVED - issue BATCH-485: SkippableExceptionClasses should be inverted http://jira.springframework.org/browse/BATCH-485 SkipLimitStepFactoryBean now declares setter for fatal exceptions that are never skipped and immediately rethrown by exception handlerRESOLVED - issue BATCH-485: SkippableExceptionClasses should be inverted http://jira.springframework.org/browse/BATCH-485 SkipLimitStepFactoryBean now declares setter for fatal exceptions that are never skipped and immediately rethrown by exception handler. LimitCheckingItemSkipPolicy and SimpleLimitExceptionHandler were modified to recognize fatal exceptions. --- .../step/item/SkipLimitStepFactoryBean.java | 50 ++++++++++++++----- .../skip/LimitCheckingItemSkipPolicy.java | 46 ++++++++++++++--- .../item/SkipLimitReadFailurePolicyTests.java | 4 +- .../item/SkipLimitStepFactoryBeanTests.java | 43 +++++++++++++--- .../SimpleLimitExceptionHandler.java | 47 +++++++++++++---- 5 files changed, 152 insertions(+), 38 deletions(-) diff --git a/spring-batch-core/src/main/java/org/springframework/batch/core/step/item/SkipLimitStepFactoryBean.java b/spring-batch-core/src/main/java/org/springframework/batch/core/step/item/SkipLimitStepFactoryBean.java index b91e85ce5..ab75b99a8 100644 --- a/spring-batch-core/src/main/java/org/springframework/batch/core/step/item/SkipLimitStepFactoryBean.java +++ b/spring-batch-core/src/main/java/org/springframework/batch/core/step/item/SkipLimitStepFactoryBean.java @@ -7,15 +7,28 @@ import org.springframework.batch.core.step.skip.NeverSkipItemSkipPolicy; import org.springframework.batch.repeat.exception.SimpleLimitExceptionHandler; /** - * Factory bean for step that allows configuring skip limit. + * Factory bean for step that provides options for configuring skip behavior. + * User can set {@link #skipLimit} to set how many exceptions of + * {@link #skippableExceptionClasses} types are tolerated. + * {@link #fatalExceptionClasses} will cause immediate termination of job - they + * are treated as higher priority than {@link #skippableExceptionClasses}, so + * the two lists don't need to be exclusive. + * + * @see SimpleStepFactoryBean + * @see StatefulRetryStepFactoryBean + * + * @author Dave Syer + * @author Robert Kasanicky * */ public class SkipLimitStepFactoryBean extends SimpleStepFactoryBean { private int skipLimit = 0; - - private Class[] skippableExceptionClasses = new Class[]{ Exception.class }; - + + private Class[] skippableExceptionClasses = new Class[] { Exception.class }; + + private Class[] fatalExceptionClasses = new Class[] { Error.class }; + /** * Public setter for a limit that determines skip policy. If this value is * positive then an exception in chunk processing will cause the item to be @@ -28,11 +41,11 @@ public class SkipLimitStepFactoryBean extends SimpleStepFactoryBean { public void setSkipLimit(int skipLimit) { this.skipLimit = skipLimit; } - + /** * Public setter for exception classes that when raised won't crash the job - * but will result in transaction rollback and the item which handling caused - * the exception will be skipped. + * but will result in transaction rollback and the item which handling + * caused the exception will be skipped. * * @param skippableExceptionClasses defaults to Exception */ @@ -41,12 +54,21 @@ public class SkipLimitStepFactoryBean extends SimpleStepFactoryBean { } /** - * Uses the {@link #skipLimit} value to configure item handler and - * and exception handler. + * Public setter for exception classes that should cause immediate failure. + * + * @param fatalExceptionClasses {@link Error} by default + */ + public void setFatalExceptionClasses(Class[] fatalExceptionClasses) { + this.fatalExceptionClasses = fatalExceptionClasses; + } + + /** + * Uses the {@link #skipLimit} value to configure item handler and and + * exception handler. */ protected void applyConfiguration(ItemOrientedStep step) { super.applyConfiguration(step); - + ItemSkipPolicyItemHandler itemHandler = new ItemSkipPolicyItemHandler(getItemReader(), getItemWriter()); if (skipLimit > 0) { @@ -55,10 +77,12 @@ public class SkipLimitStepFactoryBean extends SimpleStepFactoryBean { * to absorb exceptions at the step level because the failed items * will never re-appear after a rollback. */ - itemHandler.setItemSkipPolicy(new LimitCheckingItemSkipPolicy(skipLimit, Arrays.asList(skippableExceptionClasses))); + itemHandler.setItemSkipPolicy(new LimitCheckingItemSkipPolicy(skipLimit, Arrays + .asList(skippableExceptionClasses), Arrays.asList(fatalExceptionClasses))); SimpleLimitExceptionHandler exceptionHandler = new SimpleLimitExceptionHandler(); exceptionHandler.setLimit(skipLimit); exceptionHandler.setExceptionClasses(skippableExceptionClasses); + exceptionHandler.setFatalExceptionClasses(fatalExceptionClasses); setExceptionHandler(exceptionHandler); getStepOperations().setExceptionHandler(getExceptionHandler()); } @@ -66,8 +90,8 @@ public class SkipLimitStepFactoryBean extends SimpleStepFactoryBean { // This is the default in ItemOrientedStep anyway... itemHandler.setItemSkipPolicy(new NeverSkipItemSkipPolicy()); } - + step.setItemHandler(itemHandler); } - + } diff --git a/spring-batch-core/src/main/java/org/springframework/batch/core/step/skip/LimitCheckingItemSkipPolicy.java b/spring-batch-core/src/main/java/org/springframework/batch/core/step/skip/LimitCheckingItemSkipPolicy.java index 4912d3492..361a75500 100644 --- a/spring-batch-core/src/main/java/org/springframework/batch/core/step/skip/LimitCheckingItemSkipPolicy.java +++ b/spring-batch-core/src/main/java/org/springframework/batch/core/step/skip/LimitCheckingItemSkipPolicy.java @@ -45,12 +45,16 @@ import org.springframework.batch.support.SubclassExceptionClassifier; * likely want to skip, but a {@link FileNotFoundException} should cause * immediate termination of the {@link Step}. Because it would be impossible * for a general purpose policy to determine all the types of exceptions that - * should be skipped from those that shouldn't, a list must be passed in, with - * all of the exceptions that are 'skippable' + * should be skipped from those that shouldn't, two lists must be passed in, + * with all of the exceptions that are 'fatal' and 'skippable'. The two lists + * are not enforced to be exclusive, they are prioritized instead - exceptions + * that are fatal will never be skipped, regardless whether the exception can + * also be classified as skippable. *

* * @author Ben Hale * @author Lucas Ward + * @author Robert Kasanicky */ public class LimitCheckingItemSkipPolicy implements ItemSkipPolicy { @@ -59,15 +63,33 @@ public class LimitCheckingItemSkipPolicy implements ItemSkipPolicy { */ private static final String SKIP = "skip"; + /** + * Label for classifying fatal exceptions - these are never skipped. + */ + private static final String NEVER_SKIP = "neverSkip"; + private final int skipLimit; private ExceptionClassifier exceptionClassifier; + /** + * Convenience constructor that assumes all exception types are skippable + * and none are fatal. + * @param skipLimit the number of exceptions allowed to skip + */ public LimitCheckingItemSkipPolicy(int skipLimit) { - this(skipLimit, Collections.singletonList(Exception.class)); + this(skipLimit, Collections.singletonList(Exception.class), Collections.EMPTY_LIST); } - public LimitCheckingItemSkipPolicy(int skipLimit, List skippableExceptions) { + /** + * + * @param skipLimit the number of skippable exceptions that are allowed to + * be skipped + * @param skippableExceptions exception classes that can be skipped + * (non-critical) + * @param fatalExceptions exception classes that should never be skipped + */ + public LimitCheckingItemSkipPolicy(int skipLimit, List skippableExceptions, List fatalExceptions) { this.skipLimit = skipLimit; SubclassExceptionClassifier exceptionClassifier = new SubclassExceptionClassifier(); Map typeMap = new HashMap(); @@ -75,6 +97,10 @@ public class LimitCheckingItemSkipPolicy implements ItemSkipPolicy { Class throwable = (Class) iterator.next(); typeMap.put(throwable, SKIP); } + for (Iterator iterator = fatalExceptions.iterator(); iterator.hasNext();) { + Class throwable = (Class) iterator.next(); + typeMap.put(throwable, NEVER_SKIP); + } exceptionClassifier.setTypeMap(typeMap); this.exceptionClassifier = exceptionClassifier; } @@ -82,12 +108,16 @@ public class LimitCheckingItemSkipPolicy implements ItemSkipPolicy { /** * Given the provided exception and skip count, determine whether or not * processing should continue for the given exception. If the exception is - * not within the list of 'skippable exceptions', false will be returned. If - * the exception is within the list, and {@link StepExecution} skipCount is - * greater than the skipLimit, then a {@link SkipLimitExceededException} - * will be thrown. + * not within the list of 'skippable exceptions' or belongs to the list of + * 'fatal exceptions', false will be returned. If the exception is within + * the skippable list (and not in the fatal list), and {@link StepExecution} + * skipCount is greater than the skipLimit, then a + * {@link SkipLimitExceededException} will be thrown. */ public boolean shouldSkip(Throwable t, int skipCount) { + if (exceptionClassifier.classify(t).equals(NEVER_SKIP)) { + return false; + } if (exceptionClassifier.classify(t).equals(SKIP)) { if (skipCount < skipLimit) { return true; diff --git a/spring-batch-core/src/test/java/org/springframework/batch/core/step/item/SkipLimitReadFailurePolicyTests.java b/spring-batch-core/src/test/java/org/springframework/batch/core/step/item/SkipLimitReadFailurePolicyTests.java index ba6325ab4..8fa66572c 100644 --- a/spring-batch-core/src/test/java/org/springframework/batch/core/step/item/SkipLimitReadFailurePolicyTests.java +++ b/spring-batch-core/src/test/java/org/springframework/batch/core/step/item/SkipLimitReadFailurePolicyTests.java @@ -17,6 +17,7 @@ package org.springframework.batch.core.step.item; import java.io.FileNotFoundException; import java.util.ArrayList; +import java.util.Collections; import java.util.List; import junit.framework.TestCase; @@ -38,8 +39,9 @@ public class SkipLimitReadFailurePolicyTests extends TestCase { List skippableExceptions = new ArrayList(); skippableExceptions.add(FlatFileParseException.class); + List fatalExceptions = Collections.EMPTY_LIST; - failurePolicy = new LimitCheckingItemSkipPolicy(1, skippableExceptions); + failurePolicy = new LimitCheckingItemSkipPolicy(1, skippableExceptions, fatalExceptions); } public void testLimitExceed(){ diff --git a/spring-batch-core/src/test/java/org/springframework/batch/core/step/item/SkipLimitStepFactoryBeanTests.java b/spring-batch-core/src/test/java/org/springframework/batch/core/step/item/SkipLimitStepFactoryBeanTests.java index 09b1ecf1e..8d4874a87 100644 --- a/spring-batch-core/src/test/java/org/springframework/batch/core/step/item/SkipLimitStepFactoryBeanTests.java +++ b/spring-batch-core/src/test/java/org/springframework/batch/core/step/item/SkipLimitStepFactoryBeanTests.java @@ -34,8 +34,10 @@ public class SkipLimitStepFactoryBeanTests extends TestCase { SkipLimitStepFactoryBean tested = new SkipLimitStepFactoryBean(); - // TODO checked exceptions are wrapped as RepeatExceptions by the chunkOperations#exceptionHandler - Class[] skippableExceptions = new Class[] { RepeatException.class, SkippableException.class, SkippableRuntimeException.class }; + // TODO checked exceptions are wrapped as RepeatExceptions by the + // chunkOperations#exceptionHandler + Class[] skippableExceptions = new Class[] { RepeatException.class, SkippableException.class, + SkippableRuntimeException.class }; final int SKIP_LIMIT = 2; @@ -61,8 +63,7 @@ public class SkipLimitStepFactoryBeanTests extends TestCase { } /** - * - * @throws Exception + * Check items causing errors are skipped as expected. */ public void testSkip() throws Exception { ItemOrientedStep step = (ItemOrientedStep) tested.getObject(); @@ -71,7 +72,7 @@ public class SkipLimitStepFactoryBeanTests extends TestCase { step.execute(stepExecution); assertEquals(2, stepExecution.getSkipCount()); - + assertTrue(reader.skipped.contains("2")); assertTrue(reader.skipped.contains("4")); // writer did not skip "2" as it never made it to writer, only "4" did @@ -84,7 +85,31 @@ public class SkipLimitStepFactoryBeanTests extends TestCase { .contains(expectedOutput[i])); } assertTrue(writer.written.size() == expectedOutput.length); - + + } + + /** + * Fatal exception should cause immediate termination regardless of other + * skip settings (note the fatal exception is also classified as skippable). + */ + public void testFatalException() throws Exception { + tested.setFatalExceptionClasses(new Class[] { FatalRuntimeException.class }); + tested.setItemWriter(new SkipWriterStub() { + public void write(Object item) { + throw new FatalRuntimeException("Ouch!"); + } + }); + + ItemOrientedStep step = (ItemOrientedStep) tested.getObject(); + StepExecution stepExecution = new StepExecution(step, jobExecution); + + try { + step.execute(stepExecution); + fail(); + } + catch (FatalRuntimeException expected) { + assertTrue(expected.getMessage().equals("Ouch!")); + } } /** @@ -175,4 +200,10 @@ public class SkipLimitStepFactoryBeanTests extends TestCase { } } + private static class FatalRuntimeException extends SkippableRuntimeException { + public FatalRuntimeException(String message) { + super(message); + } + } + } diff --git a/spring-batch-infrastructure/src/main/java/org/springframework/batch/repeat/exception/SimpleLimitExceptionHandler.java b/spring-batch-infrastructure/src/main/java/org/springframework/batch/repeat/exception/SimpleLimitExceptionHandler.java index ab64586ab..9f45bbbec 100644 --- a/spring-batch-infrastructure/src/main/java/org/springframework/batch/repeat/exception/SimpleLimitExceptionHandler.java +++ b/spring-batch-infrastructure/src/main/java/org/springframework/batch/repeat/exception/SimpleLimitExceptionHandler.java @@ -22,23 +22,35 @@ import org.springframework.batch.repeat.RepeatContext; import org.springframework.batch.support.ExceptionClassifierSupport; /** - * Simple implementation of exception handler which looks for one exception - * type. If it is found then a counter is incremented and the a limit is checked - * to determine if it has been exceeded and the Throwable should be re-thrown. + * Simple implementation of exception handler which looks for given exception + * types. If one of the types is found then a counter is incremented and the + * limit is checked to determine if it has been exceeded and the Throwable + * should be re-thrown. Also allows to specify list of 'fatal' exceptions that + * are never subject to counting, but are immediately re-thrown. The fatal list + * has higher priority so the two lists needn't be exclusive. * * @author Dave Syer + * @author Robert Kasanicky */ public class SimpleLimitExceptionHandler implements ExceptionHandler { /** - * Name of exception classifier key for the nominated exception type. + * Name of exception classifier key for the nominated exception types. */ private static final String TX_INVALID = "TX_INVALID"; + /** + * Name of exception classifier key for the fatal exception types (not + * counted, immediately rethrown). + */ + private static final String FATAL = "FATAL"; + private RethrowOnThresholdExceptionHandler delegate = new RethrowOnThresholdExceptionHandler(); private Class[] exceptionClasses = new Class[] { Exception.class }; + private Class[] fatalExceptionClasses = new Class[] { Error.class }; + /** * Flag to indicate the the exception counters should be shared between * sibling contexts in a nested batch (i.e. inner loop). Default is false. @@ -68,6 +80,11 @@ public class SimpleLimitExceptionHandler implements ExceptionHandler { super(); delegate.setExceptionClassifier(new ExceptionClassifierSupport() { public Object classify(Throwable throwable) { + for (int i = 0; i < fatalExceptionClasses.length; i++) { + if (fatalExceptionClasses[i].isAssignableFrom(throwable.getClass())) { + return FATAL; + } + } for (int i = 0; i < exceptionClasses.length; i++) { if (exceptionClasses[i].isAssignableFrom(throwable.getClass())) { return TX_INVALID; @@ -103,20 +120,30 @@ public class SimpleLimitExceptionHandler implements ExceptionHandler { { put(ExceptionClassifierSupport.DEFAULT, new Integer(0)); put(TX_INVALID, new Integer(limit)); + put(FATAL, new Integer(0)); } }); } /** - * Setter for the Throwable exceptionClasses that this handler counts. Defaults to - * {@link Exception}. If more exceptionClasses are specified handler uses single - * counter that is incremented when one of the recognized exception - * exceptionClasses is handled. - * - * @param type + * Setter for the Throwable exceptionClasses that this handler counts. + * Defaults to {@link Exception}. If more exceptionClasses are specified + * handler uses single counter that is incremented when one of the + * recognized exception exceptionClasses is handled. */ public void setExceptionClasses(Class[] classes) { this.exceptionClasses = classes; } + /** + * Setter for the Throwable exceptionClasses that shouldn't be counted, but + * rethrown immediately. This list has higher priority than + * {@link #exceptionClasses}. + * + * @param fatalExceptionClasses defaults to {@link Error} + */ + public void setFatalExceptionClasses(Class[] fatalExceptionClasses) { + this.fatalExceptionClasses = fatalExceptionClasses; + } + }