-
Notifications
You must be signed in to change notification settings - Fork 41.6k
Add option to allow Spring Batch custom isolation levels #28859
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.
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
Conversation
| public class BasicBatchConfigurer implements BatchConfigurer, InitializingBean { | ||
|
|
||
| private final BatchProperties properties; | ||
| protected final BatchProperties properties; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally we prefer to keep member variables private. Is there a reason you changed these?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From the design of this class it looks like it it's open for extension (see protected methods). However, subclasses cannot access the members those protected methods operate on, even though it's passed to their constructor. I'd suggest either extend the visibility to protected or provide protected getters to these. Since the members are final I think it's safe and would improve extensibility. What do you think?
For this concrete member, the new JpaBatchConfigurer implementation uses the properties too in determineIsolationLevel
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given that the fields don't need to be made protected to implement the configurable isolation level, I think they should be considered via a separate issue please. Even properties could remain private as JpaBatchConfigurer could capture what it needs in its constructor. This would minimise the changes to the public API.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the delay. Sure, I'll roll this change back and will consider a separate PR for this.
|
Thanks for the follow-up @tiborsulyan. Spring Batch has added an enum in the next major release so we'd like to create a local enum to improve user experience in 2.7.x. No need to act on it, we'll do it as part of merging it. |
|
@tiborsulyan thank you for making your first contribution to Spring Boot. |
Fixes #28802
Create
spring.batch.jdbc.isolation-level-for-createproperty with the following behavior:null