Wrong implementation of noRetry(...) and noSkip(...) in FaultTolerantStepBuilder #1199
Comments
|
Michael Minella commented What makes you think this? Is there a use case that is failing for you? |
|
Albert Strasser commented Thanks for responding so fast. Unfortunately yes. I am working in jdbc batch mode with chunks in fault tolerant mode (cause I need it to produce read skips). If the commit of the chunk transaction produces an UncategorizedSQLException, it becomes "swallowed" as it is not one of the hardcoded 11 fatal exceptions and the job goes to COMPLETE without having written anything (very dangerous mode for production!). To be more concise, the exceptionHandler of the RetryTemplate swallows it. Then i tried to explicetely turn off retry for the UncategorizedSQLException by using noRetry. No effect. Then I manipuated the nonRetryableExceptions field in the FaultTolerantStepBuilder by using reflection to add the UncategorizedSQLException (ugly workaround), and now it works. The exception during chunk commit makes the job fail. |
|
Michael Minella commented Can you share your configuration? |
|
Daniel Guggi commented As Albert already stated, I also don't see an easy way to configure/modify "nonSkippableExceptionClasses" and/or "nonRetryableExceptionClasses" properties on FaultTolerantStepBuilder instances. There is afaik no (obvious) way to add elements to that collections. Those collections are only filled via "addSpecialExceptions" (private) method (maybe we're not supposed to add elements to those collections? - or am I missing something here?) Upon build() the FaultTolerantStepBuilder's createRetryOperations() method is invoked: RepeatOperations stepOperations = getStepOperations();
if (stepOperations instanceof RepeatTemplate) {
SimpleRetryExceptionHandler exceptionHandler = new SimpleRetryExceptionHandler(retryPolicyWrapper,
getExceptionHandler(), nonRetryableExceptionClasses);
((RepeatTemplate) stepOperations).setExceptionHandler(exceptionHandler);
}SimpleRetryExceptionHandler instance is initialized with (hardcoded) non-retryable exception classes. Which results in (handleException() method): //....
else {
logger.debug("Handled non-fatal exception", throwable);
}
//...Although the appropriate exception was set on the builder using "noRetry" method (probably noRetry() should "just" add the given exceptions to "nonRetryableExceptionClasses" collection? - same for noSkip()?). This is the step configuration we used (without success): stepBuilders.get(PROCESSING_STEP_NAME)
.<ObjectA, ObjectB>chunk(jobProperties().getChunkSize())
.reader(someReader())
.processor(someProcessor())
.writer(someWriter())
.faultTolerant()
.skip(Skip1Exception.class)
.skip(Skip2Exception.class)
.noRetry(DataAccessException.class)
.skipLimit(Integer.MAX_VALUE)
.listener(someListener())
.listener(anotherListener())
.build();I think we could set a custom/explicit JobOperations-Implementation (actually not an instanceof RepeatTemplate): .stepOperations(somthingThatDoesNotExtendFromRepeatTemplate())in order to ensure that the following (default) code is not executed (FaultTolerantJobBuilder.createRetryOperations()): RepeatOperations stepOperations = getStepOperations();
if (stepOperations instanceof RepeatTemplate) {
SimpleRetryExceptionHandler exceptionHandler = new SimpleRetryExceptionHandler(retryPolicyWrapper,
getExceptionHandler(), nonRetryableExceptionClasses);
((RepeatTemplate) stepOperations).setExceptionHandler(exceptionHandler);
}Is this expected behaviour? Thanks! |
|
Albert Strasser commented Thanks Daniel. You are right about the stepOperations. I experimented with it but this is no easy way because by not extending the RetryTemplate we loose everything that RetryTemplate has and does. It is not trivial to set everything manually that the step builders set on the stepOperations during build(). I decided against continuing this because it seemed very dangerous to remodel spring batch code in a customized class. This will probably lead to big issues when upgrading to newer batch versions. There should be another way. |
|
Erwin Vervaet commented
We're facing very similar issues on version 2.2.7. The SimpleRetryExceptionHandler incorrectly categorizes this exception as non-fatal since it is not one of the hard-coded nonRetryableExceptionClasses the FaultTolerantStepBuilder sets up:
This is also evidenced by the logging: Consequently, the batch processing does not fail and the batch just continues processing, ultimately finishing in what appears to be a success for what was actually a failure! Very dangerous indeed! |
|
Erwin Vervaet commented
I think there are deeper bugs here than just the FaultTolerantStepBuild issues. I've created a new JIRA ticket: BATCH-2415 |
|
Mahmoud Ben Hassine commented Skip/Retry features are applicable to exceptions thrown from the processor or writer. This should be clearly mentioned in the documentation. When the commit of the chunk’s transaction fails, there is no retry logic applied (which explains why using Note there is no retry/skip configuration in this test and the step is still completing (but should fail). This is because by default, the Even if we change the implementation of |
|
Erwin Vervaet commented
Correct.
That kind of makes sense to me. |
|
Mahmoud Ben Hassine commented
I do confirm (with the failing test). It should be noted that the issue happens only with fault tolerant steps, simple steps fail as expected when the commit fails.
Indeed, even if technically it will fix the issue, I think it is not the correct way to do it. Requiring the user to call Currently, all stack traces provided contain the debug message "Handled non-fatal exception". There should be another way to tell Spring Batch which exception is fatal or not, something like: In a previous version, there was a public method called Probably we can consider keeping the current split of semantics but also add a configurable collection of fatal exceptions. Fatal semantics should be clearly defined and documented. A fatal exception could be defined as:
Points 1 and 2 are for backward compatibility. Point 3 is what is missing today I think.
Yes, thank you very much for the elaborate analysis!! The whole issue is when the commit [B] fails. |
|
Erwin Vervaet commented Having a set of "fatal exception" classes of course can do the trick, but it might be a bit drastic. I would argue that the exceptions that only occur at transaction commit time are often transient, so a retry might be more appropriate. |
fmbenhassine
added
the
status: waiting-for-triage
fmbenhassine
added
in: core
related-to: transaction-management
and removed
in: retry
labels
fmbenhassine
changed the title
Albert Strasser opened BATCH-2403 and commented
In org.springframework.batch.core.step.builder.FaultTolerantStepBuilder.java
This implementation:
should be changed to:
A similar problem concerns the noSkip() method in the same class.
Affects: 3.0.3, 3.0.4
3 votes, 8 watchers
The text was updated successfully, but these errors were encountered: