Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[Improvement-17057][Master]Check if the WorkflowGrapth is a dag at constructor #17058

Open
wants to merge 6 commits into
base: dev
Choose a base branch
from
Open

[Improvement-17057][Master]Check if the WorkflowGrapth is a dag at constructor #17058

wants to merge 6 commits into from

Conversation

yingh0ng
Copy link
Contributor

@yingh0ng yingh0ng commented Mar 14, 2025 •
edited by SbloodyS
Loading

Purpose of the pull request

Check if the WorkflowGrapth is a dag close #17057

Brief change log

Conclusion: A graph is not a DAG if its task length is less than the total number of tasks after topological sorting

Verify this pull request

This change added tests and can be verified as follows:

  • WorkflowGraphCheckIfDAGTest

Pull Request Notice

Pull Request Notice

If your pull request contains incompatible change, you should also add it to docs/docs/en/guide/upgrade/incompatible.md

@boring-cyborg boring-cyborg
Copy link

boring-cyborg bot commented Mar 14, 2025

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)

@SbloodyS SbloodyS added first time contributor First-time contributor improvement make more easy to user or prompt friendly labels Mar 17, 2025
@yingh0ng
Copy link
Contributor Author

Update: When a ring exists, it is not a DAG

@yingh0ng
Copy link
Contributor Author

@SbloodyS Hello, please take a look.

@yingh0ng
Copy link
Contributor Author

@SbloodyS The Mergeable check failed because this PR was not added to the milestone.

@sonarqubecloud SonarQubeCloud
Copy link

Quality Gate Passed Quality Gate passed

Issues
8 New issues
0 Accepted issues

Measures
0 Security Hotspots
100.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

ruanwenjun
ruanwenjun reviewed Mar 24, 2025
View reviewed changes
...ster/src/main/java/org/apache/dolphinscheduler/server/master/engine/graph/WorkflowGraph.java Outdated
@@ -57,6 +59,22 @@ public WorkflowGraph(List<WorkflowTaskRelation> workflowTaskRelations, List<Task
addTaskEdge(workflowTaskRelations);
}

private void checkIfDAG(List<WorkflowTaskRelation> workflowTaskRelations, List<TaskDefinition> taskDefinitions) {
DAG<Long, TaskDefinition, WorkflowTaskRelation> graph = new DAG<>();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please don't use DAG, this class will be removed in the future.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

つ﹏⊂
So, are there other implemented classes?
Otherwise I will implement it using my previous commit.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ruanwenjun PTAL

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

WorkflowGraph is used to replace DAG

@yingh0ng yingh0ng force-pushed the improvement-17057-checkIfDAG branch from ce52582 to c537e51 Compare April 3, 2025 08:11
@yingh0ng yingh0ng requested a review from ruanwenjun April 7, 2025 01:34
@yingh0ng
Copy link
Contributor Author

yingh0ng commented Apr 8, 2025

@ruanwenjun PTAL

@yingh0ng yingh0ng force-pushed the improvement-17057-checkIfDAG branch from ae02625 to a85f8a9 Compare April 11, 2025 03:29
@yingh0ng yingh0ng force-pushed the improvement-17057-checkIfDAG branch from a85f8a9 to 88863d5 Compare April 11, 2025 10:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@caishunfeng caishunfeng Awaiting requested review from caishunfeng caishunfeng is a code owner

@SbloodyS SbloodyS Awaiting requested review from SbloodyS SbloodyS is a code owner

@ruanwenjun ruanwenjun Awaiting requested review from ruanwenjun ruanwenjun is a code owner

At least 2 approving reviews are required to merge this pull request.

Assignees

@yingh0ng yingh0ng

Labels
backend first time contributor First-time contributor improvement make more easy to user or prompt friendly test
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Improvement][Master] Check if the WorkflowGrapth is a dag at constructor
3 participants
@yingh0ng @ruanwenjun @SbloodyS