-
Notifications
You must be signed in to change notification settings - Fork 2.4k
BATCH-2108 #292
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
BATCH-2108 #292
Conversation
@@ -58,10 +58,6 @@ public ChunkMonitorData(int offset, int chunkSize) { | |||
|
|||
private ItemReader<?> reader; | |||
|
|||
public ChunkMonitor() { | |||
this.setExecutionContextName(ChunkMonitor.class.getName()); | |||
} |
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.
This is problematic. While (from what I can see) all the other places we use the short name (MyClass for example), in this case, we use the full class name (org.springframework.batch.MyClass). Changing this would break restarts of this job.
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.
It will indeed break job restarts, but this will be the case for the other components as well, because with this PR the default execution context name will be overwritten with the bean name (unless the user has explicitly configured the name property).
Is it not acceptable that job executions started with spring-batch 2.2.x runtime cannot be restarted on spring-batch 3.x runtime?
While it would be acceptable to have breaking changes on a major version like 3.0, I question the benefit of breaking that for this feature. If we were already breaking it, merging this would be a no-brainer. However since 3.0 is backwards compatible from a restart perspective without this, I'm hesitant to merge this right now. |
From 2.1 to 2.2 there was also at least one change that broke backwards compatibility for restarts in JdbcPagingItemReader: https://jira.spring.io/browse/BATCH-1749. But this pull requests would indeed break job restartability of 2.2.x job instances on a larger scale, so I can understand your objections. I do want to mention that the problem that this PR solves has been a frequent source of restartibility bugs in the batch projects I have been involved in (using more than one reader/writer of the same type in the same step, forgetting to explicitly configure them with a unique name). |
I'm wondering if there is a more delicate way to handle this than clobbering restart capabilities, and yet still bring to a developer's attention the scenario you're attempting to fix. For now, I'm going to close this PR so that we can move this conversation to Jira. If we come back to this approach, we can always re-open it. |
spring-projects#292. Instead of trying to prevent ExecutionContext key collisions using BeanNameAware, this PR throws an exception on update() when a duplicate component name is detected.
Change ItemStreamSupport to implement BeanNameAware. The default name (in case the object is instantiated outside of a spring bean factory) is still the short form of the class name. The name property can still be explicitly configured by the user. When the user didn't configure the name, the BeanNameAware.setBeanName callback will initialize the name property to the bean name.