-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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.
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
[Fix-16934][api] When creating workflows containing switch nodes in different orders, the copied workflows may lose associations. #16939
base: dev
Are you sure you want to change the base?
Conversation
…ifferent orders, the copied workflows may lose associations
@SbloodyS @caishunfeng PTAL |
Quality Gate passedIssues Measures |
if (TaskTypeUtils.isSwitchTask(taskDefinitionLog.getTaskType())) { | ||
final String taskParams = taskDefinitionLog.getTaskParams(); | ||
final SwitchParameters switchParameters = | ||
JSONUtils.parseObject(taskParams, SwitchParameters.class); | ||
if (switchParameters == null) { | ||
throw new IllegalArgumentException( | ||
"Switch task params: " + taskParams + " is invalid."); | ||
} | ||
SwitchParameters.SwitchResult switchResult = switchParameters.getSwitchResult(); | ||
switchResult.getDependTaskList().forEach(switchResultVo -> { | ||
switchResultVo.setNextNode(taskCodeMap.get(switchResultVo.getNextNode())); | ||
}); | ||
if (switchResult.getNextNode() != null) { | ||
switchResult.setNextNode( | ||
taskCodeMap.get(switchResult.getNextNode())); | ||
} | ||
taskDefinitionLog.setTaskParams(JSONUtils.toJsonString(switchParameters)); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why move this code to here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why move this code to here?
I think this is git's display problem,
I don't move this code to here,
I added taskDefinitionLogs.forEach
before for (TaskDefinitionLog taskDefinitionLog : taskDefinitionLogs)
, because we need put all taskDefinitionLog's new code into taskCodeMap before taskDefinitionLog.setTaskParams
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The commit shows it's not a display issue. https://github.com/apache/dolphinscheduler/blob/48304f2a6c20de7471739b8889c2d02c41eef17d/dolphinscheduler-api/src/main/java/org/apache/dolphinscheduler/api/service/impl/WorkflowDefinitionServiceImpl.java
I think I misunderstood your question.
Do you want to ask me why I move this code after taskDefinitionLog.setName
?
Because for better readability:
taskDefinitionLog.setCode();
taskDefinitionLog.setProjectCode();
taskDefinitionLog.setVersion();
taskDefinitionLog.setName();
if (TaskTypeUtils.isSwitchTask(taskDefinitionLog.getTaskType())) {
...
taskDefinitionLog.setTaskParams();
...
}
Is better than
taskDefinitionLog.setCode();
if (TaskTypeUtils.isSwitchTask(taskDefinitionLog.getTaskType())) {
...
taskDefinitionLog.setTaskParams();
...
}
taskDefinitionLog.setProjectCode();
taskDefinitionLog.setVersion();
taskDefinitionLog.setName();
Purpose of the pull request
Fix #16934
Brief change log
Verify this pull request
This pull request is code cleanup without any test coverage.
(or)
This pull request is already covered by existing tests, such as (please describe tests).
(or)
This change added tests and can be verified as follows:
(or)
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