qiuyanjun888 commented on issue #17751: URL: https://github.com/apache/dolphinscheduler/issues/17751#issuecomment-4766155673
Hi @ruanwenjun, I checked the history of this issue and the closed PR #17953. I agree that simply moving `WorkflowGraph` / `WorkflowExecutionGraph` creation into `WorkflowStartLifecycleEvent` is unsafe, especially if the master crashes between command handling and the start event. Here is a different design direction that avoids keeping graph construction inside the command transaction, but also avoids lazy initialization without a durable lifecycle boundary. ## Proposed direction Introduce an explicit workflow execution bootstrap phase between command persistence and workflow start. ### 1. Keep command handling focused on durable workflow metadata The command handler should create/update the durable workflow instance and command-related metadata inside the transaction, but should not construct the in-memory `WorkflowExecutionGraph` while the command transaction is held. The command transaction should leave the workflow in a durable state that means roughly: "workflow instance created, execution bootstrap still pending". This could be represented by either: - a new workflow instance state / flag such as `INITIALIZING` or `BOOTSTRAPPING`; or - an existing state plus a durable bootstrap marker, if adding a new state is too invasive. ### 2. Add a bootstrapper before `WorkflowStartLifecycleEvent` After the command transaction commits, a dedicated `WorkflowExecutionBootstrapper` can: - assemble `WorkflowGraph` and `WorkflowExecutionGraph` outside the command transaction; - initialize task execution state in one well-defined place; - attach the initialized graph to `WorkflowExecuteContext`; - only then publish `WorkflowStartLifecycleEvent`. So `WorkflowStartLifecycleEvent` would still assume that the graph is ready. It would not become the component that constructs the graph. ### 3. Make bootstrap idempotent for failover The key invariant should be: if the master crashes after the command transaction but before the start event, the new master can detect a bootstrap-pending workflow instance and safely run the bootstrapper again. Possible rules: - if no task instances / execution graph have been materialized yet, reconstruct from workflow definition and bootstrap normally; - if task instances were already materialized, rebuild the in-memory graph from persisted task-instance state instead of creating duplicates; - bootstrap should be guarded so repeated attempts do not create duplicate task instances or inconsistent edges. ### 4. Add a separate failure path for bootstrap failures If graph construction fails before any task execution graph exists, the current normal failed-event path may not be sufficient because `WorkflowRunningStateAction.onFailedEvent` expects a failed task chain in the graph. A safer design may need a separate bootstrap failure transition, for example: - mark the workflow instance as failed; - move or clean the related command if needed; - record the bootstrap error; - avoid requiring `WorkflowExecutionGraph.isExistFailureTaskExecutionChain()` when the graph was never initialized. ### 5. Suggested staged implementation 1. First PR: add regression/integration tests that describe the crash/failover boundary and graph-initialization-failure behavior, without changing the production flow. 2. Second PR: introduce the bootstrap state/marker and `WorkflowExecutionBootstrapper`, but keep existing command types functionally equivalent. 3. Third PR: move graph construction for the normal run-workflow path out of the command transaction. 4. Later PRs: migrate rerun/recover/failover command paths one by one. ## Questions - Is a new workflow instance state/marker for bootstrap-pending acceptable, or should this be modeled with existing states only? - Should `WorkflowStartLifecycleEvent` continue to require a fully initialized `WorkflowExecutionGraph`, with initialization handled just before publishing the event? - What persisted invariants should hold across these three boundaries: command transaction committed, bootstrap completed, start event published? - Would a first PR that only adds failover/bootstrap regression tests be useful before implementation? I do not want to repeat the unsafe approach from #17953, so I would like to align on these lifecycle/failover invariants before preparing code. -- 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]
