Add AbstractTaskletStepBuilder copy constructor #4471

ilpyoyang · 2023-10-23T14:50:00Z

Description

This PR adds a copy constructor to the AbstractTaskletStepBuilder to address an issue arising during the property setting process with taskExecutor.

Changes

Add AbstractTaskletStepBuilder copy constructor
Remove unnecessary transactionManager in SimpleStepBuilder copy constructor
Add testcode

Resolve #4438

injae-kim · 2023-10-23T14:55:46Z

...re/src/main/java/org/springframework/batch/core/step/builder/AbstractTaskletStepBuilder.java

+	 * copied, so it can be re-used.
+	 * @param parent a parent helper containing common step properties
+	 *
+	 * @author Ilpyo Yang


Maybe @author should be located on L51, not here?

change to L51 completed!

injae-kim · 2023-10-23T14:58:37Z

...batch-test/src/test/java/org/springframework/batch/test/AbstractTaskletStepBuilderTests.java

+
+	@Test
+	void faultTolerantMethodTest() throws ReflectiveOperationException {
+		simpleStepBuilder.taskExecutor(taskExecutor);	// The task executor is set before faultTolerant()


assertEquals(taskExecutor, afterTaskExecutor);

Can we check taskExecutor is copied well after faultTolerant()?

I have added it, thank you for the review.

injae-kim · 2023-12-04T10:10:50Z

...re/src/main/java/org/springframework/batch/core/step/builder/AbstractTaskletStepBuilder.java

+	/**
+	 * Create a new builder initialized with any properties in the parent. The parent is
+	 * copied, so it can be re-used.
+	 * @param parent a parent helper containing common step properties
+	 */
+	public AbstractTaskletStepBuilder(AbstractTaskletStepBuilder<?> parent) {
+		super(parent);
+		this.chunkListeners = parent.chunkListeners;
+		this.stepOperations = parent.stepOperations;
+		this.transactionManager = parent.transactionManager;
+		this.transactionAttribute = parent.transactionAttribute;
+		this.streams.addAll(parent.streams);
+		this.exceptionHandler = parent.exceptionHandler;
+		this.throttleLimit = parent.throttleLimit;
+		this.taskExecutor = parent.taskExecutor;
+	}


TaskletStep step2 = new StepBuilder("step-name", jobRepository) .chunk(10, transactionManager) .taskExecutor(taskExecutor)// The task executor is set before faultTolerant() .reader(itemReader) .processor(itemProcessor) .writer(itemWriter) .faultTolerant() // after faultTolerant(), taskExecutor is not copied // cause there's no copy constructor of AbstractTaskletStepBuilder! .build();

Hi @fmbenhassine , me and @Ilpyo-Yang checked that there's no copy constructor of AbstractTaskletStepBuilder, so SimpleStepBuilder.faultTolerant() doesn't copy member variables of AbstractTaskletStepBuilder well now.

All users who use SimpleStepBuilder.faultTolerant() affected by this bug, so I think it's good to release this bug fix on next release.

So can you review this PR? thanks a lot! 🙇

injae-kim · 2023-12-04T10:17:59Z

...re/src/main/java/org/springframework/batch/core/step/builder/AbstractTaskletStepBuilder.java

+		this.stepOperations = parent.stepOperations;
+		this.transactionManager = parent.transactionManager;
+		this.transactionAttribute = parent.transactionAttribute;
+		this.streams.addAll(parent.streams);


Suggested change

this.streams.addAll(parent.streams);

this.streams = parent.streams;

Hmm maybe we can just use parent.streams instead of addAll()?

Hmm maybe we can just use parent.streams instead of addAll()?

To creating a copy constructor, it seems like the second option, this.streams = parents.streams, is appropriate. Thank you for your comment @injae-kim!

But just curious.. was there any specific reason for initially declaring streams as a final variable?

acktsap · 2024-02-06T17:26:34Z

...re/src/main/java/org/springframework/batch/core/step/builder/AbstractTaskletStepBuilder.java

+	 * copied, so it can be re-used.
+	 * @param parent a parent helper containing common step properties
+	 */
+	public AbstractTaskletStepBuilder(AbstractTaskletStepBuilder<?> parent) {


How about making it protected?

oh I will try thanks!

acktsap · 2024-02-06T17:30:00Z

...batch-test/src/test/java/org/springframework/batch/test/AbstractTaskletStepBuilderTests.java

@@ -0,0 +1,114 @@
+package org.springframework.batch.test;


I think package line should be followed by copyright comments. (see also)

I didn't recognize it.. I will updated it ;)

acktsap · 2024-02-06T17:32:18Z

...batch-test/src/test/java/org/springframework/batch/test/AbstractTaskletStepBuilderTests.java

+ */
+@SpringBatchTest
+@SpringJUnitConfig
+public class AbstractTaskletStepBuilderTests {


I think this test code should be placed somewhere in spring-batch-core/src/test/java....

spring-batch-test module is for providing test utils for spring batch users.

umm that is why this not merged.. I missed few things in contribute regulation. Thank you for kindness review. I will update all in this holiday

@acktsap thanks for the detailed review, and I've incorporated the suggested changes.

I still see this class in the spring-batch-test module. No problem, I will move it to the core module on merge.

fmbenhassine · 2024-02-13T16:34:41Z

Thank you for the PR! It fixes the issue 👍

However, for the test, there is no need to test the copy constructor itself. I think something like the test suggested in #4438 is enough to validate the fix and cover the code path for any future regression.

Could you please update the test and rebase/squash the changes? It should then be good to merge. Thank you upfront!

This commit includes tests for the copy constructor of AbstractTaskletStepBuilder and for the faultTolerant method, specifically after taskExecutor has been set.

ilpyoyang · 2024-02-17T11:05:59Z

Hello @fmbenhassine!
I've updated the commits as per your guidance. The test code now matches the issue details. I've stuck with the TaskletStep approach, finding it most suitable. Please review at your convenience. Thank you!

fmbenhassine · 2024-02-20T10:49:40Z

Thank you for the updates, @Ilpyo-Yang ! I noticed that the tests pass even without the changes in the PR. I think we can simplify tests by checking that the type of the repeat template is correct whether the task executor is set before or after .faultTolerant(). I will take care of that on merge.

- Update tests - Move test class to the core module

- Update tests - Move test class to the core module (cherry picked from commit 4b8e504)

fmbenhassine · 2024-02-20T11:06:28Z

LGTM 👍 Rebased and merged as b9ba8ff. I just updated the tests as mentioned before in 4b8e504. Thank you for your contribution!

- Update tests - Move test class to the core module (cherry picked from commit 4b8e504)

injae-kim reviewed Oct 23, 2023

View reviewed changes

ilpyoyang force-pushed the main branch 2 times, most recently from e281868 to a22d354 Compare October 23, 2023 16:04

injae-kim reviewed Dec 4, 2023

View reviewed changes

acktsap reviewed Feb 6, 2024

View reviewed changes

ilpyoyang force-pushed the main branch 2 times, most recently from a22d354 to f5dd94c Compare February 12, 2024 13:12

fmbenhassine added the status: waiting-for-reporter Issues for which we are waiting for feedback from the reporter label Feb 13, 2024

ilpyoyang force-pushed the main branch from 1e8bab8 to a22d354 Compare February 17, 2024 07:36

Remove unnecessary transactionManager

2eeb101

ilpyoyang force-pushed the main branch from a22d354 to 9bc5c6d Compare February 17, 2024 07:45

Add AbstractTaskletStepBuilder copy constructor and testcode

5bf8a61

This commit includes tests for the copy constructor of AbstractTaskletStepBuilder and for the faultTolerant method, specifically after taskExecutor has been set.

ilpyoyang force-pushed the main branch from 9bc5c6d to 5bf8a61 Compare February 17, 2024 07:51

fmbenhassine added a commit that referenced this pull request Feb 20, 2024

Refine contribution #4471

4b8e504

- Update tests - Move test class to the core module

fmbenhassine added a commit that referenced this pull request Feb 20, 2024

Refine contribution #4471

f53ef73

- Update tests - Move test class to the core module (cherry picked from commit 4b8e504)

fmbenhassine closed this Feb 20, 2024

fmbenhassine added pr-for: bug in: core and removed status: waiting-for-reporter Issues for which we are waiting for feedback from the reporter labels Feb 20, 2024

fmbenhassine added a commit that referenced this pull request Feb 20, 2024

Refine contribution #4471

6549ecc

- Update tests - Move test class to the core module (cherry picked from commit 4b8e504)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add AbstractTaskletStepBuilder copy constructor #4471

Add AbstractTaskletStepBuilder copy constructor #4471

ilpyoyang commented Oct 23, 2023

injae-kim Oct 23, 2023

ilpyoyang Oct 23, 2023

injae-kim Oct 23, 2023

ilpyoyang Oct 23, 2023 •

edited

Loading

injae-kim Dec 4, 2023

injae-kim Dec 4, 2023

ilpyoyang Jan 1, 2024

acktsap Feb 6, 2024

ilpyoyang Feb 8, 2024

acktsap Feb 6, 2024

ilpyoyang Feb 8, 2024

acktsap Feb 6, 2024

ilpyoyang Feb 8, 2024

ilpyoyang Feb 12, 2024

fmbenhassine Feb 20, 2024

fmbenhassine commented Feb 13, 2024

ilpyoyang commented Feb 17, 2024

fmbenhassine commented Feb 20, 2024

fmbenhassine commented Feb 20, 2024

	this.streams.addAll(parent.streams);
	this.streams = parent.streams;

Add AbstractTaskletStepBuilder copy constructor #4471

Add AbstractTaskletStepBuilder copy constructor #4471

Conversation

ilpyoyang commented Oct 23, 2023

Description

Changes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ilpyoyang Oct 23, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fmbenhassine commented Feb 13, 2024

ilpyoyang commented Feb 17, 2024

fmbenhassine commented Feb 20, 2024

fmbenhassine commented Feb 20, 2024

ilpyoyang Oct 23, 2023 •

edited

Loading