Repository navigation
Conversation
iPalash
changed the title
There was a problem hiding this comment.
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
testHandleJobFailureEventmethod should also test the scenario where the job state is successful.
}
| 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); |
There was a problem hiding this comment.
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
| TextStringBuilder sb = new TextStringBuilder(); | ||
| try { | ||
| List<Issue> issues = this.getIssueRepository().getAll(); | ||
| if (issues.size() == 0) { |
There was a problem hiding this comment.
please use issues.isEmpty()
| 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())) { |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
| synchronized (this.applicationDone) { | ||
| while (!this.applicationCompleted) { | ||
| try { | ||
| this.applicationDone.wait(); |
There was a problem hiding this comment.
it might be simpler and cleaner to use CountDownLatch instead of explicit synchronization with synchronized, wait(), and notify()
There was a problem hiding this comment.
throw exception?
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
Description
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
Tested manually
Commits