[GitHub] [gobblin] umustafi commented on a diff in pull request #3661: [GOBBLIN-1800] Fix bug where retries would not consider jobs that are SLA killed

["umustafi (via GitHub)" <gi...@apache.org>]

umustafi commented on code in PR #3661:
URL: https://github.com/apache/gobblin/pull/3661#discussion_r1175638739


##########
gobblin-service/src/main/java/org/apache/gobblin/service/modules/orchestration/DagManager.java:
##########
@@ -681,18 +681,20 @@ private void cancelDag(DagId dagId) throws ExecutionException, InterruptedExcept
       clearUpDagAction(dagId);
     }
 
-    private void cancelDagNode(DagNode<JobExecutionPlan> dagNodeToCancel) throws ExecutionException, InterruptedException {
+    private void cancelDagNode(DagNode<JobExecutionPlan> dagNodeToCancel, boolean shouldSendCancellationEvent) throws ExecutionException, InterruptedException {

Review Comment:
   can you add a comment to explain why you added this boolean and what case we don't send cancellation event even though this function is called?



##########
gobblin-service/src/main/java/org/apache/gobblin/service/modules/orchestration/DagManager.java:
##########
@@ -853,27 +852,40 @@ private boolean killJobIfOrphaned(DagNode<JobExecutionPlan> node, JobStatus jobS
             DagManagerUtils.getFullyQualifiedDagName(node),
             timeOutForJobStart);
         dagManagerMetrics.incrementCountsStartSlaExceeded(node);
-        cancelDagNode(node);
 
         String dagId = DagManagerUtils.generateDagId(node).toString();
-        this.dags.get(dagId).setFlowEvent(TimingEvent.FlowTimings.FLOW_START_DEADLINE_EXCEEDED);
+        this.dags.get(dagId).setFlowEvent(TimingEvent.FlowTimings.FLOW_CANCELLED);

Review Comment:
   we're not calling `cancelDagNode` but still updating flow status also



-- 
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.

To unsubscribe, e-mail: dev-unsubscribe@gobblin.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org