iPalash commented on code in PR #4097:
URL: https://github.com/apache/gobblin/pull/4097#discussion_r1955824192
##########
gobblin-temporal/src/main/java/org/apache/gobblin/temporal/joblauncher/GobblinTemporalJobScheduler.java:
##########
@@ -218,7 +218,11 @@ public void
handleNewJobConfigArrival(NewJobConfigArrivalEvent newJobArrival) {
throw new RuntimeException(e);
}
}));
- launcher.launchJob(listener);
+ try {
+ launcher.launchJob(listener);
+ } finally {
+ launcher.close();
Review Comment:
Is there value in adding it to the shutdown hook above as well? or putting
it another way should we add the close functionally added to
`GobblinTemporalJobLauncher.close` to `executeCancellation` instead triggered
via cancelJob. That way we don't need to add the close override there.
##########
gobblin-temporal/src/main/java/org/apache/gobblin/temporal/cluster/GobblinTemporalTaskRunner.java:
##########
@@ -292,6 +293,12 @@ public synchronized void stop() {
this.containerMetrics.get().stopMetricsReporting();
}
+ try {
+ this.workflowServiceStubs.getOptions().getMetricsScope().close();
Review Comment:
Should closing the `this.workflowServiceStubs` handle the metric Scope
closure instead of this class doing it here?
Why does that not happen implicitly?
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
