Skip to content

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

Closed
yingh0ng wants to merge 6 commits intoapache:devfrom
yingh0ng:improvement-17057-checkIfDAG
Closed

[Improvement-17057][Master] Check if the WorkflowGrapth is a dag at constructor#17058
yingh0ng wants to merge 6 commits intoapache:devfrom
yingh0ng:improvement-17057-checkIfDAG

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
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)

github-advanced-security[bot]
github-advanced-security bot found potential problems Mar 14, 2025
View reviewed changes
...ster/src/main/java/org/apache/dolphinscheduler/server/master/engine/graph/WorkflowGraph.java
Queue<Long> queue = new ArrayDeque<>();
for (Map.Entry<Long, Integer> entry : inDegreeCount.entrySet()) {
if (entry.getValue() == 0 && visitTable.add(entry.getKey())) {
queue.offer(entry.getKey());

Check notice

Code scanning / CodeQL

Ignored error status of call

Method checkIfDAG ignores exceptional return value of Queue<Long>.offer.
...ster/src/main/java/org/apache/dolphinscheduler/server/master/engine/graph/WorkflowGraph.java
inDegreeCount.put(postCode, inDegreeCount.get(postCode) - 1);

if (inDegreeCount.get(postCode) == 0) {
queue.offer(postCode);

Check notice

Code scanning / CodeQL

Ignored error status of call

Method checkIfDAG ignores exceptional return value of Queue<Long>.offer.
@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

yingh0ng commented Mar 17, 2025

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

@yingh0ng
Copy link
Contributor Author

yingh0ng commented Mar 20, 2025

@SbloodyS Hello, please take a look.

@yingh0ng
Copy link
Contributor Author

yingh0ng commented Mar 21, 2025

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

ruanwenjun
ruanwenjun reviewed Mar 24, 2025
View reviewed changes
...ster/src/main/java/org/apache/dolphinscheduler/server/master/engine/graph/WorkflowGraph.java Outdated
}

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

@ruanwenjun ruanwenjun Mar 24, 2025

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

@yingh0ng yingh0ng Mar 25, 2025

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

@yingh0ng yingh0ng Mar 31, 2025

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

@ruanwenjun ruanwenjun Apr 2, 2025

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 2 times, most recently from a85f8a9 to 88863d5 Compare April 11, 2025 10:28
@SbloodyS SbloodyS changed the title [Improvement-17057][Master]Check if the WorkflowGrapth is a dag at constructor [Improvement-17057][Master] Check if the WorkflowGrapth is a dag at constructor Apr 14, 2025
@SbloodyS SbloodyS added this to the 3.3.1 milestone Apr 14, 2025
@yingh0ng yingh0ng force-pushed the improvement-17057-checkIfDAG branch from 88863d5 to c664d33 Compare April 15, 2025 01:39
@sonarqubecloud
Copy link

sonarqubecloud bot commented Apr 16, 2025

Quality Gate Passed Quality Gate passed

Issues
21 New issues
0 Accepted issues

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

See analysis details on SonarQube Cloud

@yingh0ng yingh0ng force-pushed the improvement-17057-checkIfDAG branch from c664d33 to 66db1e9 Compare April 21, 2025 01:36
@yingh0ng yingh0ng force-pushed the improvement-17057-checkIfDAG branch from 66db1e9 to b772a6d Compare May 9, 2025 06:14
@yingh0ng
Copy link
Contributor Author

yingh0ng commented May 9, 2025

@ruanwenjun @SbloodyS PTAL

@yingh0ng
Copy link
Contributor Author

yingh0ng commented Jun 5, 2025

Can somebody review this again.

ruanwenjun
ruanwenjun reviewed Jun 5, 2025
View reviewed changes
...ster/src/main/java/org/apache/dolphinscheduler/server/master/engine/graph/WorkflowGraph.java
addTaskEdge(workflowTaskRelations);
}

private void checkIfDAG(List<WorkflowTaskRelation> workflowTaskRelations, List<TaskDefinition> taskDefinitions) {
Copy link
Member

@ruanwenjun ruanwenjun Jun 5, 2025

Choose a reason for hiding this comment

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

It seems we can directly use DFS to check if this is DAG after addTaskNodes and addTaskEdge.

Copy link
Contributor Author

@yingh0ng yingh0ng Jun 6, 2025

Choose a reason for hiding this comment

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

Yes, DFS can check whether a graph is a DAG. My implementation is Kahn's algorithm.
Both methods can perform DAG detection.

Copy link
Contributor Author

@yingh0ng yingh0ng Jul 4, 2025

Choose a reason for hiding this comment

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

Hey, if you have question or suggestion for this, please tell me.

@sonarqubecloud
Copy link

sonarqubecloud bot commented Jun 5, 2025

Please retry analysis of this Pull-Request directly on SonarQube Cloud

@yingh0ng yingh0ng force-pushed the improvement-17057-checkIfDAG branch from b772a6d to b7a2d0f Compare June 10, 2025 01:34
@SbloodyS
Copy link
Member

SbloodyS commented Aug 6, 2025

Closing for inactivity.

@SbloodyS SbloodyS closed this Aug 6, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ruanwenjun ruanwenjun ruanwenjun left review comments

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

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

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

3.3.1

Development

Successfully merging this pull request may close these issues.

[Improvement][Master] Check if the WorkflowGrapth is a dag at constructor

3 participants

@yingh0ng @SbloodyS @ruanwenjun