[Improvement-17751][Master] Initialize WorkflowGraph/WorkflowExecutionGraph when WorkflowStartLifecycleEvent fired #17953
+433
−305
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
GaneshPatil7517
requested review from
SbloodyS,
caishunfeng and
ruanwenjun
as code owners
|
Thanks for opening this pull request! Please check out our contributing guidelines. (https://github.com/apache/dolphinscheduler/blob/dev/docs/docs/en/contribute/join/pull-request.md) |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR refactors the initialization of WorkflowGraph and WorkflowExecutionGraph to be deferred until WorkflowStartLifecycleEvent is fired, rather than during command handling. This is achieved through a new assembler pattern using the IWorkflowExecutionGraphAssembler functional interface.
Changes:
- Introduced
IWorkflowExecutionGraphAssemblerinterface for deferred graph assembly - Modified
WorkflowExecuteContextto support lazy initialization of the execution graph with thread-safe double-checked locking - Updated all command handlers to return assembler lambdas instead of immediately creating execution graphs
- Added graph initialization call in
WorkflowRunningStateAction.onStartEvent()
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| IWorkflowExecutionGraphAssembler.java | New functional interface for deferred execution graph assembly |
| WorkflowExecuteContext.java | Added volatile field, assembler support, and lazy initialization logic with thread safety |
| IWorkflowExecuteContext.java | Added interface methods for graph initialization |
| AbstractCommandHandler.java | Changed to use assembler pattern, calling createWorkflowExecutionGraphAssembler instead of direct assembly |
| RunWorkflowCommandHandler.java | Returns assembler lambda that captures necessary context and defers graph creation |
| RecoverFailureTaskCommandHandler.java | Returns assembler lambda for failure recovery scenarios |
| WorkflowFailoverCommandHandler.java | Returns assembler lambda for failover scenarios |
| RecoverSerialWaitCommandHandler.java | Returns null assembler as no graph needed for serial wait recovery |
| ReRunWorkflowCommandHandler.java | Wraps parent assembler with task instance invalidation logic |
| WorkflowRunningStateAction.java | Calls initializeWorkflowExecutionGraph() and handles null graph case |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
.../dolphinscheduler/server/master/engine/workflow/statemachine/WorkflowRunningStateAction.java
Outdated
Show resolved
Hide resolved
...r/src/main/java/org/apache/dolphinscheduler/server/master/runner/WorkflowExecuteContext.java
Outdated
Show resolved
Hide resolved
...r/src/main/java/org/apache/dolphinscheduler/server/master/runner/WorkflowExecuteContext.java
Show resolved
Hide resolved
...r/src/main/java/org/apache/dolphinscheduler/server/master/runner/WorkflowExecuteContext.java
Show resolved
Hide resolved
...r/src/main/java/org/apache/dolphinscheduler/server/master/runner/WorkflowExecuteContext.java
Show resolved
Hide resolved
GaneshPatil7517
changed the title
Member
|
It is best not to use this approach, as the command handler should not be concerned with how instances are initialized. |
… when WorkflowStartLifecycleEvent fired This commit moves the initialization of WorkflowGraph and WorkflowExecutionGraph from command handling to when the WorkflowStartLifecycleEvent is fired. Changes: - Created IWorkflowExecutionGraphAssembler interface for deferred graph assembly - Modified WorkflowExecuteContext to support lazy initialization with assembler - Updated AbstractCommandHandler to use assembler pattern instead of direct initialization - Modified all command handlers to create assemblers instead of immediate graphs - Updated WorkflowRunningStateAction.onStartEvent to initialize the graph and handle failures Benefits: - Reduces transaction time during command processing - Enables proper workflow failure handling if graph initialization fails - Workflow can be properly marked as failed with WorkflowFailedLifecycleEvent Closes apache#17751
Address reviewer feedback: command handlers should not be concerned with how instances are initialized. Changes: - Add WorkflowExecutionGraphFactory to centralize all graph creation logic - Remove IWorkflowExecutionGraphAssembler interface (no longer needed) - Remove assembleWorkflowExecutionGraphAssembler() from AbstractCommandHandler - Remove createWorkflowExecutionGraphAssembler() from all command handlers - Update WorkflowRunningStateAction to use factory for graph initialization - Update IWorkflowExecuteContext: add getProject(), change to setWorkflowExecutionGraph() - Update WorkflowExecuteContext to remove assembler, add setter The factory handles all command-type-specific graph creation strategies: - START_PROCESS: Fresh start with configured start nodes - REPEAT_RUNNING: Rerun entire workflow - START_FAILURE_TASK_PROCESS: Recover failed/paused tasks - RECOVER_TOLERANCE_FAULT_PROCESS: Failover with existing task instances - RECOVER_SERIAL_WAIT: Serial wait recovery
GaneshPatil7517
force-pushed
the
improve-init-workflow-graph-on-start-event
branch
from
04897ae to
cba95a4
Compare
3 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Purpose of the pull request
This PR moves the initialization of
WorkflowGraphandWorkflowExecutionGraphfrom command handling to when theWorkflowStartLifecycleEventis fired, as requested in #17751.Brief description of changes
New Components
IWorkflowExecutionGraphAssembler: A functional interface for deferredWorkflowExecutionGraphassemblyModified Components
WorkflowExecuteContext: AddedworkflowExecutionGraphAssemblerfield and lazy initialization support withinitializeWorkflowExecutionGraph()andisWorkflowExecutionGraphInitialized()methodsIWorkflowExecuteContext: Added interface methods for graph initializationAbstractCommandHandler: Changed to use assembler pattern - now callscreateWorkflowExecutionGraphAssembler()instead ofassembleWorkflowExecutionGraph()RunWorkflowCommandHandlerRecoverFailureTaskCommandHandlerWorkflowFailoverCommandHandlerRecoverSerialWaitCommandHandler(returns null - no graph needed)ReRunWorkflowCommandHandler(wraps parent assembler)WorkflowRunningStateAction: Added graph initialization inonStartEvent()with proper exception handling that publishesWorkflowFailedLifecycleEventon failureBenefits
WorkflowFailedLifecycleEventVerification
Closes #17751
