Skip to content

Override SimpleStepBuilder.faultTolerant() in FaultTolerantStepBuilder #3840

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

Override SimpleStepBuilder.faultTolerant() in FaultTolerantStepBuilder #3840

wants to merge 1 commit into from

Conversation

marbon87
Copy link
Contributor

@marbon87 marbon87 commented Jan 28, 2021 •
edited
Loading 

		@Bean
		public Step faultTolerantStep(StepBuilderFactory stepBuilderFactory) throws Exception {
			return stepBuilderFactory.get("faultTolerantStep")
					.chunk(2)
					.reader(reader())
					.processor(processor())
					.writer(writer())
					.faultTolerant()
					.listener(chunkListener)
					.skipLimit(1)
					.faultTolerant()
                                        .build()

I had a failure in a step configuration when calling .faultTolerant() twice. That led to the problem that the chunkListener was not registered.
The reason is that calling .faultTolerant() on an existing FaultTolerantStepBuilder creates a new FaultTolerantStepBuilder but not all configurations, like ChunkListeners, are kept.
Although it was a mistake in my job config, i think it would be better to return the current FaultTolerantStepBuilder to prevent such mistakes.

@fmbenhassine fmbenhassine added the status: waiting-for-triage Issues that we did not analyse yet label Jan 29, 2021
@fmbenhassine
Copy link
Contributor

Good catch! Thank you for your PR. This will make the FaultTolerantStepBuilder more tolerant to configuration faults 😉

However, could you please add a failing test that passes with this PR? We need to get this covered by a test. Thank you upfront.

@fmbenhassine fmbenhassine added in: core pr-for: bug status: waiting-for-reporter Issues for which we are waiting for feedback from the reporter and removed status: waiting-for-triage Issues that we did not analyse yet labels Mar 15, 2021
@marbon87 marbon87 closed this Mar 23, 2021
@marbon87
Override SimpleStepBuilder.faultTolerant() in FaultTolerantStepBuilder
5c8ea1c 
Override SimpleStepBuilder.faultTolerant() in FaultTolerantStepBuilder to prevent creation of a new FaultTolerantStepBuilder when calling faultTolerant() on an existing FaultTolerantStepBuilder. Otherwise configuration, like chunkListeners, are lost.
@marbon87 marbon87 reopened this Mar 23, 2021
@marbon87
Copy link
Contributor Author

Hi @benas, thanks for the feedback. I added a test.

@marbon87
Copy link
Contributor Author

Hey @benas , can you merge this pr please?

@fmbenhassine fmbenhassine removed the status: waiting-for-reporter Issues for which we are waiting for feedback from the reporter label Apr 27, 2021
@fmbenhassine fmbenhassine added this to the 4.3.3 milestone Apr 27, 2021
@fmbenhassine
Copy link
Contributor

@marbon87 Thank you for updating the PR by adding a test. I planned to merge the fix in the upcoming 4.3.3.

@fmbenhassine fmbenhassine added the for: backport-to-4.2.x Issues that will be back-ported to the 4.2.x line label May 11, 2021
fmbenhassine
fmbenhassine reviewed May 11, 2021
View reviewed changes
...src/test/java/org/springframework/batch/core/step/builder/FaultTolerantStepBuilderTests.java
import org.junit.Test;

import static org.junit.Assert.assertEquals;
import static org.mockito.Mockito.mock;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This import is not used, I will remove it on merge.

fmbenhassine pushed a commit that referenced this pull request May 11, 2021
@marbon87 @fmbenhassine
Override SimpleStepBuilder.faultTolerant() in FaultTolerantStepBuilder
a43d3a5 
Override SimpleStepBuilder.faultTolerant() in FaultTolerantStepBuilder
to prevent creation of a new FaultTolerantStepBuilder when calling
faultTolerant() on an existing FaultTolerantStepBuilder. Otherwise
configuration, like chunkListeners, are lost.

Issue #3840
@fmbenhassine
Copy link
Contributor

LGTM. Rebased and merged as a43d3a5. Thank you for your contribution!

fmbenhassine pushed a commit that referenced this pull request May 11, 2021
@marbon87 @fmbenhassine
Override SimpleStepBuilder.faultTolerant() in FaultTolerantStepBuilder
0c4c660 
Override SimpleStepBuilder.faultTolerant() in FaultTolerantStepBuilder
to prevent creation of a new FaultTolerantStepBuilder when calling
faultTolerant() on an existing FaultTolerantStepBuilder. Otherwise
configuration, like chunkListeners, are lost.

Issue #3840
fmbenhassine pushed a commit that referenced this pull request May 11, 2021
@marbon87 @fmbenhassine
Override SimpleStepBuilder.faultTolerant() in FaultTolerantStepBuilder
df7f831 
Override SimpleStepBuilder.faultTolerant() in FaultTolerantStepBuilder
to prevent creation of a new FaultTolerantStepBuilder when calling
faultTolerant() on an existing FaultTolerantStepBuilder. Otherwise
configuration, like chunkListeners, are lost.

Issue #3840
@fmbenhassine fmbenhassine mentioned this pull request May 11, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fmbenhassine fmbenhassine fmbenhassine left review comments

Assignees
No one assigned
Labels
for: backport-to-4.2.x Issues that will be back-ported to the 4.2.x line in: core pr-for: bug
Projects
None yet
Milestone
4.3.3
Development

Successfully merging this pull request may close these issues.
2 participants
@marbon87 @fmbenhassine