[GOBBLIN-2181] ]Handling for Non transient exceptions #4084

vsinghal85 · 2024-12-11T14:45:12Z

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-XXX] My Gobblin PR"
- https://issues.apache.org/jira/browse/GOBBLIN-XXX

Description

Here are some details about my PR, including screenshots (if applicable):
Many times we experience failures in flow initialization or processing due to which flow could not be concluded properly
Azkaban client exceptions and SQLIntegrityViolation exceptions are examples which have caused failures in recent history
Currently most of these failures are by default considered transient exceptions and are retried infinitely
As a side effect, it causes flows not to conclude and causes failures in future flow submissions which have caused incidents recently
As a first step we want to consider all exceptions as non transient and not retry and remove conclude the flow by removing flowspec and dag action
This PR contains the changes to conclude the flow for non transient exceptions and also mark them as failure to reflect the correct status of the flow

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"

abhishekmjain · 2024-12-12T07:32:38Z

...ervice/src/main/java/org/apache/gobblin/service/modules/orchestration/proc/DagProcUtils.java

-      if (jobFailedTimer != null) {
-        jobFailedTimer.stop(jobMetadata);
+      // Only mark the job as failed in case of non transient exceptions
+      if(!DagProcessingEngine.isTransientException(e)){


nit: missing spaces
if (!DagProcessingEngine.isTransientException(e)) {

abhishekmjain · 2024-12-12T07:33:44Z

...ervice/src/main/java/org/apache/gobblin/service/modules/orchestration/proc/DagProcUtils.java

+      if(!DagProcessingEngine.isTransientException(e)){
+        TimingEvent jobFailedTimer = DagProc.eventSubmitter.getTimingEvent(TimingEvent.LauncherTimings.JOB_FAILED);
+        String message = "Cannot submit job " + DagUtils.getFullyQualifiedJobName(dagNode) + " on executor " + specExecutorUri;
+        log.error(message, e);


Should we atleast log the message? Even if it is a transient exception.

Right now all exceptions would be considered non transient, still updated as per suggestion

abhishekmjain · 2024-12-12T07:37:56Z

...ce/src/test/java/org/apache/gobblin/service/modules/orchestration/proc/DagProcUtilsTest.java

+    }
+    Dag<JobExecutionPlan> dag = new Dag<>(dagNodeList);
+    Mockito.doNothing().when(dagManagementStateStore).addJobDagAction(Mockito.anyString(), Mockito.anyString(), Mockito.anyLong(), Mockito.anyString(), Mockito.any());
+    DagProcUtils.submitNextNodes(dagManagementStateStore, dag, dagId);


Can we add a Mockito.verify for all the tests on dagManagementStateStore?

Sure makes sense, added in all tests

khandelwal-prateek · 2024-12-12T16:59:48Z

...vice/src/main/java/org/apache/gobblin/service/modules/orchestration/DagProcessingEngine.java

+            log.error("Ignoring non transient exception. DagTask {} will conclude and will not be retried. Exception - {} ",
+                dagTask, e);


the exception is included as part of the message string instead of passed as a separate parameter, this would log only the exception's toString(), without the full stack trace.

Please update the log statement to pass the exception as a separate parameter to log the stack trace

log.error("Ignoring non-transient exception. DagTask {} will conclude and will not be retried.", dagTask, e);

khandelwal-prateek · 2024-12-12T17:03:32Z

...vice/src/main/java/org/apache/gobblin/service/modules/orchestration/DagProcessingEngine.java

@@ -67,6 +70,8 @@ public class DagProcessingEngine extends AbstractIdleService {
  public static final String DEFAULT_JOB_START_DEADLINE_TIME_MS = "defaultJobStartDeadlineTimeMillis";
  @Getter static long defaultJobStartDeadlineTimeMillis;
  public static final String DEFAULT_FLOW_FAILURE_OPTION = FailureOption.FINISH_ALL_POSSIBLE.name();
+  // Todo Update to fetch list from config once transient exception handling is implemented and retryable exceptions defined


nit: the convention is to use TODO: , please update from Todo to TODO: for consistency

khandelwal-prateek · 2024-12-12T17:17:58Z

...vice/src/main/java/org/apache/gobblin/service/modules/orchestration/DagProcessingEngine.java

@@ -149,6 +158,13 @@ public void run() {
          dagTask.conclude();
          log.info(dagProc.contextualizeStatus("concluded dagTask"));
        } catch (Exception e) {
+          if(!DagProcessingEngine.isTransientException(e)) {
+            log.error("Ignoring non transient exception. DagTask {} will conclude and will not be retried. Exception - {} ",


don't see toString() for dagTask, what does dagTask log? It would be useful to have dagId & dagAction in the log.

Thanks for pointing out, I had moved the existing log as it is, updated it now.

phet

good start!

...lin-service/src/main/java/org/apache/gobblin/service/modules/orchestration/proc/DagProc.java

...vice/src/main/java/org/apache/gobblin/service/modules/orchestration/DagProcessingEngine.java

...ervice/src/main/java/org/apache/gobblin/service/modules/orchestration/proc/DagProcUtils.java

.../src/test/java/org/apache/gobblin/service/modules/orchestration/DagProcessingEngineTest.java

...ce/src/test/java/org/apache/gobblin/service/modules/orchestration/proc/DagProcUtilsTest.java

phet

nearly there!

...vice/src/main/java/org/apache/gobblin/service/modules/orchestration/DagProcessingEngine.java

...ervice/src/main/java/org/apache/gobblin/service/modules/orchestration/proc/DagProcUtils.java

phet · 2024-12-13T16:53:59Z

.../src/test/java/org/apache/gobblin/service/modules/orchestration/DagProcessingEngineTest.java

+  @Test
+  public void isNonTransientExceptionTest(){
+    Assert.assertTrue(!DagProcessingEngine.isTransientException(new RuntimeException("Simulating a non retryable exception!")));
+    Assert.assertTrue(!DagProcessingEngine.isTransientException(new AzkabanClientException("Simulating a retryable exception!")));


could use clarifying comment that there's nothing intrinsically transient or not about these, and in fact it just comes down to config, which is not presently provided.

even better however, would be to actually pass some test config and then demonstrate that the impl accordingly judges some as transient and others not. BTW, in authoring such a test you may find that it's challenging to have isTransientException as a static method, and yet be configuration-driven

added clarifying statement, probably test config based tests can be added when we actually start maintaining config and we also add tests for transient exceptions handling.

...and make isTransientException non-static to accept config

sure makes sense

phet · 2024-12-13T16:57:43Z

...ce/src/test/java/org/apache/gobblin/service/modules/orchestration/proc/DagProcUtilsTest.java

+              jobExecutionPlan.getFlowExecutionId(), jobExecutionPlan.getJobName(),
+              DagActionStore.DagActionType.REEVALUATE);
+    }
+


definitely an improvement on verification! you forgot Mockito.verifyNoMoreInteractions(DMSS)

(please add to other two tests too)

phet

very nice work here!

vsinghal85 changed the title ~~Handling for Non transient exceptions~~ [GOBBLIN-2181] ]Handling for Non transient exceptions Dec 11, 2024

vsinghal85 force-pushed the flow-conclude branch from 65c6873 to d6bdfea Compare December 11, 2024 14:52

Handling for Non transient exceptions

fdf09f9

vsinghal85 force-pushed the flow-conclude branch from d6bdfea to fdf09f9 Compare December 11, 2024 14:54

vsinghal85 marked this pull request as ready for review December 11, 2024 15:08

abhishekmjain reviewed Dec 12, 2024

View reviewed changes

Add mockito.verify in tests

dffb79a

vsinghal85 force-pushed the flow-conclude branch from a1288bc to dffb79a Compare December 12, 2024 13:00

khandelwal-prateek reviewed Dec 12, 2024

View reviewed changes

Update logging

3ec0eb6

phet reviewed Dec 13, 2024

View reviewed changes

vsinghal85 force-pushed the flow-conclude branch from 7418e58 to ae6aa56 Compare December 13, 2024 11:39

address logging and formatting comments

f2cb9e9

vsinghal85 force-pushed the flow-conclude branch from ae6aa56 to f2cb9e9 Compare December 13, 2024 11:53

phet reviewed Dec 13, 2024

View reviewed changes

vsinghal85 added 2 commits December 13, 2024 23:36

Edit logging and tests

46b470c

Remove depensOnMethod in tests

b589ded

phet approved these changes Dec 13, 2024

View reviewed changes

phet merged commit faecb35 into apache:master Dec 13, 2024
6 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[GOBBLIN-2181] ]Handling for Non transient exceptions #4084

[GOBBLIN-2181] ]Handling for Non transient exceptions #4084

vsinghal85 commented Dec 11, 2024 •

edited

Loading

abhishekmjain Dec 12, 2024

vsinghal85 Dec 12, 2024

abhishekmjain Dec 12, 2024

vsinghal85 Dec 12, 2024

abhishekmjain Dec 12, 2024

vsinghal85 Dec 12, 2024

khandelwal-prateek Dec 12, 2024

vsinghal85 Dec 12, 2024

khandelwal-prateek Dec 12, 2024

vsinghal85 Dec 12, 2024

khandelwal-prateek Dec 12, 2024

vsinghal85 Dec 12, 2024

phet left a comment

phet left a comment

phet Dec 13, 2024

vsinghal85 Dec 13, 2024

phet Dec 13, 2024

vsinghal85 Dec 13, 2024

phet Dec 13, 2024

vsinghal85 Dec 13, 2024

phet left a comment

		log.error("Ignoring non transient exception. DagTask {} will conclude and will not be retried. Exception - {} ",
		dagTask, e);

[GOBBLIN-2181] ]Handling for Non transient exceptions #4084

[GOBBLIN-2181] ]Handling for Non transient exceptions #4084

Conversation

vsinghal85 commented Dec 11, 2024 • edited Loading

JIRA

Description

Tests

Commits

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

phet left a comment

Choose a reason for hiding this comment

phet left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

phet left a comment

Choose a reason for hiding this comment

vsinghal85 commented Dec 11, 2024 •

edited

Loading