-
Notifications
You must be signed in to change notification settings - Fork 4.9k
[Improvement-17169][service] unify preTasks and depList to predecessors in TaskNode, to avoid type conversion #17170
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
base: dev
Are you sure you want to change the base?
Conversation
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) |
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 not unify this in front-end and back-end, we can only keep one field in TaskNode.
good idea, let me try it |
d09565a
to
72a4e0e
Compare
I think |
could you approval this workflow? @ruanwenjun @SbloodyS thx~ |
Done. |
could you have a time to review this pr? thx~ @SbloodyS @ruanwenjun |
/** | ||
* inner dependency information | ||
*/ | ||
@JsonDeserialize(using = JSONUtils.JsonDataDeserializer.class) | ||
@JsonSerialize(using = JSONUtils.JsonDataSerializer.class) | ||
private String preTasks; | ||
|
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.
Seems this field used in front-end?
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.
I think TaskNode just used in back-end when workflow running, in front-end it use taskDefinitionJson to represent task node in dag. And I searched ui module and not found TaskNode to use.
It maybe same name preTasks
in front-end, because TaskNode
is a inner data structure in Master module when workflow running.
If I loss some info, please tell me.
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.
This field is used by frontend in workflow definition and workflow instance and task definition and task instance. You should change the frontend too.
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.
This field is used by frontend in workflow definition and workflow instance and task definition and task instance. You should change the frontend too.
Maybe I miss some info and not found usage by frontend, could you give me an example? @SbloodyS thank you~
e.g. I found the field preTasks
used in task-node.ts and taskRelationJson
in definition/create/create/index.tsx, but maybe it is not same with preTasks
in class TaskNode
. It represent task relation, but it is not TaskNode
.
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.
Are you mean just change field name preTasks
to predecessors
in front-end? @SbloodyS @ruanwenjun
And I would like to use |
6c177e4
to
432d021
Compare
Done. |
|
Do you have any suggestions? thank you @ruanwenjun @SbloodyS |
You can check comments #17170 (comment) |
Purpose of the pull request
fix #17169 , unify
depList
andpreTasks
topredecessors
inTaskNode
, to avoid type conversionBrief 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