arjun4084346 commented on a change in pull request #3141:
URL: https://github.com/apache/incubator-gobblin/pull/3141#discussion_r514634673
##########
File path:
gobblin-runtime/src/main/java/org/apache/gobblin/runtime/AbstractJobLauncher.java
##########
@@ -535,7 +535,11 @@ public WorkUnit apply(@Nullable WorkUnit input) {
} finally {
try {
TimingEvent jobCleanupTimer =
this.eventSubmitter.getTimingEvent(TimingEvent.LauncherTimings.JOB_CLEANUP);
- cleanupStagingData(jobState);
+ try {
+ cleanupStagingData(jobState);
+ } catch (Throwable t) {
+ LOG.error("Failed to clean up staging data", t);
Review comment:
Okay, so the current behavior is do not fail the job (just mark the
gobblin job as FAILED) when workunit creation fails, or anything in the main
try block fails. However, according to the current behavior any exception in
`cleanupStagingData` and `notifyListeners` are thrown (i.e. making job fail).
So, if we want to preserve the same behavior, we should move the code form
L565-L599 to finally block at 542, right?
Why the current behavior treats `cleanupStagingData` failures differently,
and whether we should change this behavior is a different question that I am
not fully sure about.
----------------------------------------------------------------
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]
