prateekm commented on a change in pull request #1437:
URL: https://github.com/apache/samza/pull/1437#discussion_r529787680
##########
File path:
samza-core/src/main/java/org/apache/samza/coordinator/MetadataResourceUtil.java
##########
@@ -55,6 +55,7 @@ public MetadataResourceUtil(JobModel jobModel,
MetricsRegistry metricsRegistry,
public void createResources() {
if (checkpointManager != null) {
checkpointManager.createResources();
+ checkpointManager.stop();
Review comment:
@MabelYC Sorry, don't understand the first part of your reply. What is
that in response to?
Creating the checkpoint manager in the constructor and stopping it after the
first use is :
1. Asymmetrical. Ideally this class should have lifecycle methods
(start/stop) and the checkpoint manager would be stopped there. Also, looks
like we don't even start the checkpoint manager?
2. Makes an unsafe assumption that the method is only called once. If it was
ever called more than once, the second call would fail since the checkpoint
manager is already stopped.
IMHO it will be cleaner to create a new checkpoint manager in
createResources and close it after use. That way there is no additional
overhead as long as it's called once, and its safe if its called more than once.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
