Skip to content

[DSIP-55][Master] Separate the waiting dispatched task into different queue by worker group #17037

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 46 commits into
base: dev
Choose a base branch
from
Open

[DSIP-55][Master] Separate the waiting dispatched task into different queue by worker group #17037

wants to merge 46 commits into from
+808 −175

Conversation

det101
Copy link
Contributor

@det101 det101 commented Mar 3, 2025 •
edited by SbloodyS
Loading

Purpose of the pull request

Simplify the GlobalTaskDispatchWaitingQueue to only handle sorting based on time.
Introduce a new WorkerGroup queue that sorts tasks based on priority and time, with priority being higher than time. Additionally, it is responsible for retrying failed tasks with a delay.

close #16260

Brief change log

This change added tests and can be verified as follows:

  • Manually verified the change by testing locally.

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 boring-cyborg
Copy link

boring-cyborg bot commented Mar 3, 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-actions github-actions bot added UI ui and front end related backend test labels Mar 3, 2025
@SbloodyS SbloodyS changed the title [Feature-16260][master]add WorkerGroup queue [DSIP-55][Master] Separate the waiting dispatched task into different queue by worker group Mar 4, 2025
@SbloodyS SbloodyS added the DSIP label Mar 4, 2025
@SbloodyS SbloodyS added this to the 3.3.0 milestone Mar 4, 2025
ruanwenjun
ruanwenjun requested changes Mar 5, 2025
View reviewed changes
@det101 det101 marked this pull request as draft March 7, 2025 04:04
@ruanwenjun ruanwenjun marked this pull request as ready for review March 7, 2025 06:55
ruanwenjun
ruanwenjun requested changes Mar 10, 2025
View reviewed changes
@det101 det101 marked this pull request as draft March 11, 2025 01:43
@det101 det101 marked this pull request as ready for review March 11, 2025 03:51
@det101 det101 marked this pull request as draft March 11, 2025 03:57
@det101 det101 marked this pull request as ready for review March 11, 2025 07:13
@github-actions github-actions bot removed the UI ui and front end related label Mar 12, 2025
luxl added 22 commits April 15, 2025 20:32
@ruanwenjun
optimized code
8a60ce4
@ruanwenjun
global queue is delayQueue
19c6128
@ruanwenjun
PriorityDelayQueue move to WorkerGroupTaskDispatcher,Simplify the Wor…
1159a1e 
…kerGroupTaskDispatcherManager logic
@ruanwenjun
Remove unnecessary resource cleanup
bd6d9ee
@ruanwenjun
less unit test time
63ffb7c
@ruanwenjun
fix jdk8 unit test
fcdec92
@ruanwenjun
optimized code
ee3fae8
@ruanwenjun
fix sonar
1e86477
@ruanwenjun
override equals
c751272
@ruanwenjun
Optimize concurrency issues
85fa740 
The logic for the non-existence of workergroup needs to be supplemented.
@ruanwenjun
\cannot find the workergroup, task will kill.
af6be6a
@ruanwenjun
fix unit test
ca78f4f
@ruanwenjun
fix unit test
48ebb78
@ruanwenjun
spotless
9340610
@ruanwenjun
retry e2e
7c9f842
@ruanwenjun
jdk 11 unit test
057cac9
@ruanwenjun
Maintain the original logic: if the worker group cannot be found, pla…
18cf5c0 
…ce it back into the queue and wait.
@ruanwenjun
optimized code
d86b44a
@ruanwenjun
Remove the close logic for WorkerGroupTaskDispatcher. Since this scen…
b4abd0b 
…ario is rare and the logic is simplified for now, it will be left as a TODO.
@ruanwenjun
fix unit test
0fb49ae
@ruanwenjun
Change the logic to dynamically create the workerGroupTaskDispatcher …
f5f0bfe 
…when adding tasks.
@ruanwenjun
fix unit test
c2487a1
SbloodyS
SbloodyS reviewed Apr 16, 2025
View reviewed changes
Copy link
Member

@SbloodyS SbloodyS left a comment

Choose a reason for hiding this comment

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

I agree with this implementation, I just have some questions. After reviewing the code, I found that this PR is to put the failed tasks in the delayed queue and wait for a certain period of time before resubmitting, instead of actually dividing them into multiple queues according to the worker group as stated in the title, is that right? @det101

@sonarqubecloud SonarQubeCloud
Copy link

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

@det101
Copy link
Contributor Author

det101 commented Apr 16, 2025

I agree with this implementation, I just have some questions. After reviewing the code, I found that this PR is to put the failed tasks in the delayed queue and wait for a certain period of time before resubmitting, instead of actually dividing them into multiple queues according to the worker group as stated in the title, is that right? @det101

If the global queue fails to be distributed to the workergoup queue, the queue will be delayed. If it has been distributed to the workergoup queue, the failure will only be placed in the workergoup's delay queue.
image

@SbloodyS
Copy link
Member

If the global queue fails to be distributed to the workergoup queue, the queue will be delayed. If it has been distributed to the workergoup queue, the failure will only be placed in the workergoup's delay queue.

Okay.

SbloodyS
SbloodyS approved these changes Apr 16, 2025
View reviewed changes
Copy link
Member

@SbloodyS SbloodyS left a comment

Choose a reason for hiding this comment

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

+1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SbloodyS SbloodyS SbloodyS approved these changes

@ruanwenjun ruanwenjun ruanwenjun left review comments

@songjianet songjianet Awaiting requested review from songjianet

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

At least 2 approving reviews are required to merge this pull request.

Assignees

@det101 det101

Labels
backend DSIP priority:high test
Projects
None yet
Milestone
3.3.1
Development

Successfully merging this pull request may close these issues.

[DSIP-55][Master] Separate the waiting dispatched task into different queue by worker group
3 participants
@det101 @SbloodyS @ruanwenjun