[GOBBLIN-2193] Fail Azkaban job on Temporal Job Failure #4096

iPalash · 2025-02-06T06:35:44Z

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.
- https://issues.apache.org/jira/browse/GOBBLIN-2193

Description

Here are some details about my PR, including screenshots (if applicable):
Currently when the temporal job running on Yarn fails, we don't propagate the error back to Azkaban job which launches the Yarn Application.

The change here bubbles the issues encountered when the job fails upto the GobblinYarnAppLaucher run by the Azkaban job and fails with a RuntimeException after logging the issues summary.

Tests

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

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"

Copilot

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (1)

gobblin-temporal/src/test/java/org/apache/gobblin/temporal/yarn/YarnServiceTest.java:148

The testHandleJobFailureEvent method should also test the scenario where the job state is successful.

...mporal/src/main/java/org/apache/gobblin/temporal/joblauncher/GobblinTemporalJobLauncher.java

codecov-commenter · 2025-02-06T07:18:08Z

Codecov Report

Attention: Patch coverage is 13.33333% with 13 lines in your changes missing coverage. Please review.

Project coverage is 51.49%. Comparing base (adef734) to head (ba88388).

Files with missing lines	Patch %	Lines
...rg/apache/gobblin/yarn/GobblinYarnAppLauncher.java	13.33%	13 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##             master    #4096      +/-   ##
============================================
+ Coverage     45.38%   51.49%   +6.10%     
- Complexity     3192     7566    +4374     
============================================
  Files           696     1388     +692     
  Lines         26628    52198   +25570     
  Branches       2655     5733    +3078     
============================================
+ Hits          12085    26878   +14793     
- Misses        13542    23026    +9484     
- Partials       1001     2294    +1293

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

gobblin-cluster/src/main/java/org/apache/gobblin/cluster/event/JobFailureEvent.java

gobblin-temporal/src/main/java/org/apache/gobblin/temporal/yarn/YarnService.java

gobblin-yarn/src/main/java/org/apache/gobblin/yarn/GobblinYarnAppLauncher.java

...mporal/src/main/java/org/apache/gobblin/temporal/joblauncher/GobblinTemporalJobLauncher.java

gobblin-temporal/src/main/java/org/apache/gobblin/temporal/yarn/YarnService.java

gobblin-temporal/src/test/java/org/apache/gobblin/temporal/yarn/YarnServiceTest.java

gobblin-yarn/src/main/java/org/apache/gobblin/yarn/GobblinYarnAppLauncher.java

gobblin-temporal/src/main/java/org/apache/gobblin/temporal/yarn/YarnService.java

khandelwal-prateek · 2025-02-16T11:34:33Z

gobblin-temporal/src/main/java/org/apache/gobblin/temporal/yarn/YarnService.java

+      if (this.jobSummaryEvent.getJobState() != null && !this.jobSummaryEvent.getJobState().getState().isSuccess()) {
+        this.amrmClientAsync.unregisterApplicationMaster(FinalApplicationStatus.FAILED, this.jobSummaryEvent.getIssuesSummary(), null);
+      } else {
+        this.amrmClientAsync.unregisterApplicationMaster(FinalApplicationStatus.SUCCEEDED, StringUtils.defaultString(this.jobSummaryEvent.getIssuesSummary()), null);


this.jobSummaryEvent.getIssuesSummary() wouldn't be null, right? since getIssuesSummary() returns an empty string as default. if yes, it is fine to use StringUtils.defaultString but we should use for both statuses or not use it at all

khandelwal-prateek · 2025-02-16T11:36:34Z

...mporal/src/main/java/org/apache/gobblin/temporal/joblauncher/GobblinTemporalJobLauncher.java

+    TextStringBuilder sb = new TextStringBuilder();
+    try {
+      List<Issue> issues = this.getIssueRepository().getAll();
+      if (issues.size() == 0) {


please use issues.isEmpty()

khandelwal-prateek · 2025-02-16T13:10:24Z

gobblin-yarn/src/main/java/org/apache/gobblin/yarn/GobblinYarnAppLauncher.java

-        LOGGER.error("Gobblin Yarn application failed for the following reason: " + applicationReport.getDiagnostics());
+        applicationFailed = true;
+        LOGGER.error("Gobblin Yarn application failed because of the following issues: " + applicationReport.getDiagnostics());
+      } else if (StringUtils.isNotBlank(applicationReport.getDiagnostics())) {


I think this should be removed as it's not useful to have diagnostics for success cases, these are mostly task failures which have already been retried

It will be useful in case where no work units were generated as in that cases job always succeeds so it will be easier to know directly.

khandelwal-prateek · 2025-02-16T13:29:39Z

gobblin-yarn/src/main/java/org/apache/gobblin/yarn/GobblinYarnAppLauncher.java

+    synchronized (this.applicationDone) {
+      while (!this.applicationCompleted) {
+        try {
+          this.applicationDone.wait();


it might be simpler and cleaner to use CountDownLatch instead of explicit synchronization with synchronized, wait(), and notify()

khandelwal-prateek · 2025-02-16T13:29:57Z

gobblin-yarn/src/main/java/org/apache/gobblin/yarn/GobblinYarnAppLauncher.java

+            throw new RuntimeException("Gobblin Yarn application failed");
+          }
+        } catch (InterruptedException ie) {
+          LOGGER.error("Interrupted while waiting for the Gobblin Yarn application to finish", ie);


throw exception?

Fail Azkaban job on GaaS Job Failure

b17f3dd

iPalash changed the title ~~[GOBBLIN-2193] Fail Azkaban job on GaaS Job (running on temporal) Failure~~ [GOBBLIN-2193] Fail Azkaban job on Temporal Job Failure Feb 6, 2025

Swapnil added 2 commits February 6, 2025 12:11

update test for YarnService

3bac719

Add docstring for JobFailureEvent

ba88388

Blazer-007 requested a review from Copilot February 6, 2025 07:02

Copilot AI reviewed Feb 6, 2025

View reviewed changes

...mporal/src/main/java/org/apache/gobblin/temporal/joblauncher/GobblinTemporalJobLauncher.java Outdated Show resolved Hide resolved

Checkstyle errors and redundant line remove

4fd615d

Blazer-007 reviewed Feb 6, 2025

View reviewed changes

Swapnil added 2 commits February 6, 2025 15:49

review comments

a449483

review changes

d898960

Blazer-007 reviewed Feb 6, 2025

View reviewed changes

import ordeR

17d12ae

Blazer-007 reviewed Feb 7, 2025

View reviewed changes

gobblin-temporal/src/main/java/org/apache/gobblin/temporal/yarn/YarnService.java Outdated Show resolved Hide resolved

replace fields with object

849991f

iPalash requested a review from Blazer-007 February 13, 2025 04:48

Blazer-007 approved these changes Feb 13, 2025

View reviewed changes

Swapnil added 2 commits February 13, 2025 10:52

update test

bb1a8f3

remove unused import

d23ef07

khandelwal-prateek reviewed Feb 16, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[GOBBLIN-2193] Fail Azkaban job on Temporal Job Failure #4096

[GOBBLIN-2193] Fail Azkaban job on Temporal Job Failure #4096

iPalash commented Feb 6, 2025 •

edited

Loading

Copilot AI left a comment

codecov-commenter commented Feb 6, 2025 •

edited

Loading

khandelwal-prateek Feb 16, 2025

khandelwal-prateek Feb 16, 2025

khandelwal-prateek Feb 16, 2025

Blazer-007 Feb 16, 2025

khandelwal-prateek Feb 16, 2025

khandelwal-prateek Feb 16, 2025

[GOBBLIN-2193] Fail Azkaban job on Temporal Job Failure #4096

Are you sure you want to change the base?

[GOBBLIN-2193] Fail Azkaban job on Temporal Job Failure #4096

Conversation

iPalash commented Feb 6, 2025 • edited Loading

JIRA

Description

Tests

Commits

Copilot AI left a comment

Choose a reason for hiding this comment

codecov-commenter commented Feb 6, 2025 • edited Loading

Codecov Report

khandelwal-prateek Feb 16, 2025

Choose a reason for hiding this comment

khandelwal-prateek Feb 16, 2025

Choose a reason for hiding this comment

khandelwal-prateek Feb 16, 2025

Choose a reason for hiding this comment

Blazer-007 Feb 16, 2025

Choose a reason for hiding this comment

khandelwal-prateek Feb 16, 2025

Choose a reason for hiding this comment

khandelwal-prateek Feb 16, 2025

Choose a reason for hiding this comment

iPalash commented Feb 6, 2025 •

edited

Loading

codecov-commenter commented Feb 6, 2025 •

edited

Loading