GOBBLIN-2175] proceed with commit in case of exception from Temporal workload execution

[pratapaditya04]

Dear Gobblin maintainers,
Please accept this PR. I understand that it will not be reviewed until I have checked off all the steps below!

JIRA

• My PR addresses the following Gobblin JIRA issues and references them in the PR title. For example, "[GOBBLIN-2175] My Gobblin PR"
  • https://issues.apache.org/jira/browse/GOBBLIN-2175

Description

• Here are some details about my PR, including screenshots (if applicable):
  Partial commit support was introduced in this PR [GOBBLIN-2165] Support partial commit semantic from CopyDataPublisher #4066 , This feature is not working as expected in temporal execution.
  When a ProcessWorkUnit activity encounters an Exception and exhausts the configured number of retries, it causes the parent NestingExecWorkUnits workflow to fail. This failure propagates upwards, resulting in a ChildWorkflowExecutionFailed for the next enclosing parent, ProcessWorkUnitsImpl. Consequently, the CommitStepWorkflow, which is crucial for file-level commit, does not execute. The file rename into the final destination occurs during the commit step, not within the task itself. This issue disrupts the workflow, as the commit step is essential for completing the file-level commit process.

To address this, we modify ProcessWorkUnitsImpl to catch the child workflow failure and still perform the CommitStepWorkflow before exiting.

Tests

• My PR adds the following unit tests OR does not need testing for this extremely good reason:

Commits

• My commits all reference JIRA issues in their subject lines, and I have squashed multiple commits if they address the same issue. In addition, my commits follow the guidelines from "How to write a good git commit message":
  1. Subject is separated from body by a blank line
  2. Subject is limited to 50 characters
  3. Subject does not end with a period
  4. Subject uses the imperative mood ("add", not "adding")
  5. Body wraps at 72 characters
  6. Body explains "what" and "why", not "how"

[phet]

I was about to suggest you omit this line and "fall through", but the timing is actually crucial. what you're doing here results in sending a failure GTE before doing the commit. that would lead GaaS in turn to emit the job observability event and complete the flow execution before commit even runs. if that case, the datasetsMetrics definitely won't be filled in the job observability event.

much better would be to wait to send the JobFailed GTE only after the JobSummary GTE is sent

hence:

performCommitIfAnyWorkUnitsProcessed(workSpec, searchAttributes, workunitsProcessed);
throw e;  // [see comment for L90 about why we throw rather than sending a GTE directly]

[codecov-commenter]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 55.43%. Comparing base (e5d897e) to head (3db4063).
Report is 1 commits behind head on master.

Additional details and impacted files

@@             Coverage Diff              @@
##             master    #4078      +/-   ##
============================================
+ Coverage     51.52%   55.43%   +3.90%     
+ Complexity     7563     1595    -5968     
============================================
  Files          1387      307    -1080     
  Lines         52132    10598   -41534     
  Branches       5724     1065    -4659     
============================================
- Hits          26863     5875   -20988     
+ Misses        22979     4222   -18757     
+ Partials       2290      501    -1789     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[phet]

need space after // and also could streamline comment:

throw e; // re-throw after any partial commit, to fail the parent workflow

but anyway, that throw on L84 won't run will it? the partial commit (CommitStepWorkflow) will throw an exception that unwinds the stack beforehand. am I correct that at best this throw is a "should never happen" fallback?

ITO which we'd prefer to surface (possibly both) - what specific info is in the orig exception relative to the one currently being throw? would it make sense to combine messages from the two or simply throw one or the other?

[pratapaditya04]

addressed, caught exception from commit step and threw exception with both the messages combined

[phet]

nice work - there are certainly several tricky nuances here you did a great job of getting under control!

[phet]

NBD, but no need for String.format, when a string literal would do

[phet]

needs space before //

[phet]

I may even have suggested this text, but reading again, it's ambiguous (e.g. was the failure while attempting partial commit?)

this would be clearer:

"ProcessWorkUnits failure - will attempt partial commit..."

[phet]

1. maybe add intro context plus a newline to separate the msgs. e.g.

"ProcessWorkUnits failure (as expected) led to failure during partial commit attempt -\n  ProcessWorkUnits failure: %s\n  CommitStep failure: %s"

2. also, can't you reuse combinedMessage on L96? or is more of commitException than just the msg getting used the second time?

3. e will not lose its stack trace, so no need to wrap it as new Exception(e), unless you want someone to know you rethrew it from this particular place. that said, I'd avoid wrapping, since that just adds more layers to peel back while debugging