Skip to content

[Fix-8296] Fix update process definition error#8309

Closed
SbloodyS wants to merge 4 commits into
apache:devfrom
SbloodyS:bug_8296
Closed

[Fix-8296] Fix update process definition error#8309
SbloodyS wants to merge 4 commits into
apache:devfrom
SbloodyS:bug_8296

Conversation

@SbloodyS
Copy link
Copy Markdown
Member

@SbloodyS SbloodyS commented Feb 8, 2022

Purpose of the pull request

This pr will close #8296

@SbloodyS SbloodyS changed the title [Fix-8296] Update process definition error [Fix-8296] Fix update process definition error Feb 8, 2022
@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Feb 8, 2022

Codecov Report

Merging #8309 (e54da8d) into dev (fc9a31f) will decrease coverage by 0.01%.
The diff coverage is 66.66%.

❗ Current head e54da8d differs from pull request most recent head b58c44e. Consider uploading reports for the commit b58c44e to get more accurate results

Impacted file tree graph

@@             Coverage Diff              @@
##                dev    #8309      +/-   ##
============================================
- Coverage     45.34%   45.32%   -0.02%     
  Complexity     3999     3999              
============================================
  Files           680      680              
  Lines         26463    26472       +9     
  Branches       2849     2848       -1     
============================================
- Hits          12000    11999       -1     
- Misses        13330    13342      +12     
+ Partials       1133     1131       -2     
Impacted Files Coverage Δ
...inscheduler/service/queue/MasterPriorityQueue.java 0.00% <0.00%> (ø)
...api/service/impl/ProcessDefinitionServiceImpl.java 32.00% <50.00%> (+0.09%) ⬆️
...lphinscheduler/service/process/ProcessService.java 32.01% <50.00%> (+0.05%) ⬆️
...e/dolphinscheduler/remote/NettyRemotingServer.java 73.77% <71.42%> (+3.18%) ⬆️
...r/api/service/impl/ProcessInstanceServiceImpl.java 60.55% <100.00%> (+0.66%) ⬆️
...he/dolphinscheduler/common/enums/SqoopJobType.java 0.00% <0.00%> (-88.89%) ⬇️
...inscheduler/common/task/sqoop/SqoopParameters.java 74.00% <0.00%> (-2.00%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fc9a31f...b58c44e. Read the comment docs.

@caishunfeng caishunfeng requested a review from brave-lee February 8, 2022 11:05
Collectors.toMap(
TaskDefinition::getCode,
taskDefinitionLog -> taskDefinitionLog,
(entityOld , entityNew) -> entityOld
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I am actually not sure the logic here, but I guess you want to retain the latest taskDefinitionLog in the map?
If so, you bettered compared by id or operateTime to determine which should retain.

Copy link
Copy Markdown
Member Author

@SbloodyS SbloodyS Feb 8, 2022

Choose a reason for hiding this comment

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

In the production environment where I upgraded from 2.0.2-release to 2.0.3-release, when entering the workflow definition modification content from the process instance page, the front end would send two identical data, which caused this error. However, I am currently unable to reproduce this problem in standalone mode. It makes me confused.

Copy link
Copy Markdown
Contributor

@jim-parsons jim-parsons Feb 9, 2022

Choose a reason for hiding this comment

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

@JinyLeeChina plz check this. i am not sure its as same as the issue #8043.

the logic maybe like this according to @JinyLeeChina .

any part of the workflow changed would lead to the workflow's version change.
so when you changed one taskDefinition, the taskDefinition has two version now, but the workflow doesn`t. When ui query this workflow it returns two taskDefinition with same code but different version and this cause the duplicate error

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

After several days of careful investigation, I probably found the cause of this situation.

Steps to reproduce:
1.Create a workflow with dependent nodes in 1.3.9, and the dependent nodes contain more than 1 dependency.
2.Create a new process definition by using copy workflow and then modify copyed workflow content randomly.
3.Upgrade version from 1.3.9 to 2.0.3.
4.Update process instance/definition.

The reason is that after using the copy workflow, the dependency will be copied, resulting in multiple identical pre tasks. At this time, there will be multiple workflow definitions and workflow relationships in the request sent by saving the workflow definition.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@JinyLeeChina plz check this. i am not sure its as same as the issue #8043.

the logic maybe like this according to @JinyLeeChina .

any part of the workflow changed would lead to the workflow's version change. so when you changed one taskDefinition, the taskDefinition has two version now, but the workflow doesn`t. When ui query this workflow it returns two taskDefinition with same code but different version and this cause the duplicate error

Yes, I also think this is the same problem as #8043. It has been fundamentally solved in version 203. There are design changes from 202 to 203. From 2.0.0 to 2.0.2, the version is designed with task as the core, and the version is designed with workflow as the core after 2.0.3. Therefore, the PR submitted in #8043 is for this design change. At the same time, the problem of repeated key is also fundamentally solved, not just the bug

@sonarqubecloud
Copy link
Copy Markdown

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 2 Code Smells

68.2% 68.2% Coverage
0.0% 0.0% Duplication

@brave-lee
Copy link
Copy Markdown
Contributor

Thank you for your contribution. I don't think this PR can be merged.
The core of version design is because the core of DS is workflow, which consists of three parts: workflow basic information, task basic information and workflow task relationship information.The changes of each of these three parts belong to the changes of workflow, so the version of workflow should be added.

@SbloodyS SbloodyS closed this Feb 16, 2022
@SbloodyS SbloodyS deleted the bug_8296 branch February 16, 2022 14:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug] [API] Update process definition error

5 participants