[Improvement-17057][Master] Check if the WorkflowGraph is a DAG at constructor#18385
[Improvement-17057][Master] Check if the WorkflowGraph is a DAG at constructor#18385shangeyao wants to merge 4 commits into
Conversation
…nstructor Validate workflow task relations with Kahn topological sort when building WorkflowGraph on the master, so cyclic graphs fail fast before execution. Co-authored-by: Cursor <cursoragent@cursor.com>
shangeyao
requested review from
SbloodyS,
caishunfeng and
ruanwenjun
as code owners
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
…r return value Use Deque.addLast/pollFirst instead of Queue.offer/poll in DAG validation. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Addressed CodeQL review comments in 7cbfb43: replaced Note: |
| } | ||
|
|
||
| if (sortedTaskCodes.size() < taskDefinitions.size()) { | ||
| throw new IllegalArgumentException("The workflow task relation is not a DAG"); |
There was a problem hiding this comment.
Include details about the relevant tasks in the suggestion error to facilitate quick identification.
…tion error Co-authored-by: Cursor <cursoragent@cursor.com>
ruanwenjun
left a comment
ruanwenjun
left a comment
There was a problem hiding this comment.
I'm a bit concerned that this implementation adds quite a bit of complexity for relatively limited benefit. It also needs to account for several edge cases, such as duplicate and invalid edges, which may increase the maintenance burden.
|
Thanks for the review, @ruanwenjun. On complexity: Happy to extract it into a small utility class if that would be easier to maintain. |
5 participants
Was this PR generated or assisted by AI?
YES. Use AI to implement the DAG validation logic and unit tests, then verify locally with Maven.
Purpose of the pull request
close #17057
Brief change log
WorkflowGraphconstructor on the master serverIllegalArgumentExceptionwhen workflow task relations contain a cycleWorkflowGraphCheckIfDAGTestcovering single task, valid DAG, and cyclic graph casesVerify this pull request
This change added tests and can be verified as follows:
WorkflowGraphCheckIfDAGTestto verify DAG validation inWorkflowGraphconstructor./mvnw -pl dolphinscheduler-master -am test -Dtest=WorkflowGraphCheckIfDAGTest -Dsurefire.failIfNoSpecifiedTests=false -Djacoco.skip=truePull Request Notice
Pull Request Notice
If your pull request contains incompatible change, you should also add it to
docs/docs/en/guide/upgrade/incompatible.mdMade with Cursor