phet commented on code in PR #3996:
URL: https://github.com/apache/gobblin/pull/3996#discussion_r1670930350
##########
gobblin-service/src/main/java/org/apache/gobblin/service/modules/orchestration/Orchestrator.java:
##########
@@ -289,7 +293,7 @@ public void submitFlowToDagManager(FlowSpec flowSpec,
Dag<JobExecutionPlan> jobE
Note that the responsibility of the multi-active scheduler mode ends
after this method is completed AND the
consumption of a launch type event is committed to the consumer.
*/
- this.dagManager.addDag(jobExecutionPlanDag, true, true);
+ this.dagManager.addDagAndRemoveAdhocFlowSpec(flowSpec,
jobExecutionPlanDag, true, true);
Review Comment:
now the attempt to remove an ad hoc spec would occur twice, since it's also
on line 267.
I believe your aim is to have removal also happen in the path from
[DagActionStoreChangeMonitor](https://github.com/apache/gobblin/blob/4721d086f78553cab28e46bf406787df6cc2dea8/gobblin-service/src/main/java/org/apache/gobblin/service/monitoring/DagActionStoreChangeMonitor.java#L318),
correct?
couldn't that run into the same issue, where if the flow compilation
validation helper fails (see L281 directly above), so the call to this new
combo addDag+removeAdHoc would be skipped?
this new PR definitely shows me I missed the `DagActionStoreChangeMonitor`
case in my recent PR (sorry to see). the crux there was to disentangle adding
the dag from removing the ad hoc spec. the latter MUST happen, suggesting it
belongs inside a `finally`, whereas the former is only best-effort. I would
imagine you'll want the same approach here, in the change monitor code path I
failed to notice
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
