-
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
[DSIP-73] Add dolphinscheduler-task-executor module to unify the task execution logic #16790
[DSIP-73] Add dolphinscheduler-task-executor module to unify the task execution logic #16790
Conversation
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.
CodeQL found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.
d3942d4
to
9abfb14
Compare
ac13593
to
e3f5a95
Compare
2c71278
to
2830a68
Compare
a8c7311
to
d1fa92b
Compare
95ea2da
to
2f72848
Compare
...g/apache/dolphinscheduler/server/master/engine/executor/plugin/dynamic/DynamicLogicTask.java
Show resolved
Hide resolved
...org/apache/dolphinscheduler/server/master/engine/system/event/GlobalMasterFailoverEvent.java
Outdated
Show resolved
Hide resolved
.../main/java/org/apache/dolphinscheduler/server/master/engine/WorkflowEventBusFireWorkers.java
Outdated
Show resolved
Hide resolved
c053789
to
0fcf9ab
Compare
0fcf9ab
to
1e2eaaa
Compare
…dTaskExecutorContainer
Quality Gate failedFailed conditions See analysis details on SonarQube Cloud Catch issues before they fail your Quality Gate with our IDE extension SonarQube for IDE |
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.
LGTM
...nscheduler/server/master/engine/executor/plugin/dynamic/DynamicAsyncTaskExecuteFunction.java
Show resolved
Hide resolved
...e/dolphinscheduler/server/master/engine/task/client/PhysicalTaskExecutorClientDelegator.java
Show resolved
Hide resolved
/** | ||
* The task instance's runtime context changed. | ||
*/ | ||
RUNTIME_CONTEXT_CHANGED, |
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 need to add this event type? The runtime context should be check in running event or finish event.
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.
Import a new event type here can avoid we have multiple event schema for an event type. This event type represent the task runtime context changed, will not affect the state, if we use running event and finish event, we need to add runtime context info into these events, once runtime context changed, we will need to change these events.
And it's hard to explain why some running events have runtime context but some not.
So split the runtime context to a single event type can avoid these, this event type doesn't affect the lifecycle of a task.
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.
LGTM overall
Purpose of the pull request
close #16619
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 contain incompatible change, you should also add it to
docs/docs/en/guide/upgrede/incompatible.md