Skip to content
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

[Improvement] Move delay calculation to Master #15278

Merged
merged 1 commit into from
Dec 6, 2023
Merged

[Improvement] Move delay calculation to Master #15278

merged 1 commit into from
Dec 6, 2023

Conversation

ruanwenjun
Copy link
Member

@ruanwenjun ruanwenjun commented Dec 5, 2023 •
edited
Loading

Purpose of the pull request

  • Fix if the worker clock is different with master clock, the task might be delay.
  • Fix the delay task will cost the worker's task queue.
  • Rename xxTaskExecuteRunnable to xxTaskExecutor

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)

If your pull request contain incompatible change, you should also add it to docs/docs/en/guide/upgrede/incompatible.md

@codecov-commenter
Copy link

codecov-commenter commented Dec 5, 2023 •
edited
Loading

Codecov Report

Attention: 119 lines in your changes are missing coverage. Please review.

Comparison is base (db3d84b) 38.09% compared to head (57bc97d) 38.10%.
Report is 1 commits behind head on dev.

❗ Current head 57bc97d differs from pull request most recent head 7bd7d49. Consider uploading reports for the commit 7bd7d49 to get more accurate results

Files Patch % Lines
...rator/BaseTaskExecuteRunnableDispatchOperator.java 0.00% 11 Missing ⚠️
.../runner/execute/AsyncTaskCallbackFunctionImpl.java 0.00% 10 Missing ⚠️
...r/rpc/LogicITaskInstanceKillOperationFunction.java 0.00% 7 Missing ⚠️
...c/LogicITaskInstanceDispatchOperationFunction.java 0.00% 6 Missing ⚠️
...r/runner/GlobalMasterTaskExecuteRunnableQueue.java 0.00% 6 Missing ⚠️
...orker/runner/DefaultWorkerTaskExecutorFactory.java 0.00% 6 Missing ⚠️
...r/runner/GlobalTaskInstanceWaitingQueueLooper.java 0.00% 6 Missing ⚠️
...orker/runner/WorkerTaskExecutorFactoryBuilder.java 0.00% 6 Missing ⚠️
.../rpc/LogicITaskInstancePauseOperationFunction.java 0.00% 5 Missing ⚠️
...er/GlobalMasterTaskExecuteRunnableQueueLooper.java 0.00% 5 Missing ⚠️
... and 23 more
Additional details and impacted files 
@@             Coverage Diff              @@
##                dev   #15278      +/-   ##
============================================
+ Coverage     38.09%   38.10%   +0.01%     
+ Complexity     4672     4662      -10     
============================================
  Files          1280     1278       -2     
  Lines         44583    44543      -40     
  Branches       4803     4792      -11     
============================================
- Hits          16983    16973      -10     
+ Misses        25731    25703      -28     
+ Partials       1869     1867       -2

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@ruanwenjun ruanwenjun added the improvement make more easy to user or prompt friendly label Dec 5, 2023
@ruanwenjun ruanwenjun force-pushed the dev_wenjun_moveDelayToMaster branch from 5f10044 to 0ecec0f Compare December 5, 2023 10:15
github-advanced-security[bot]
github-advanced-security bot found potential problems Dec 5, 2023
View reviewed changes
...worker/src/main/java/org/apache/dolphinscheduler/server/worker/runner/WorkerExecService.java
@@ -90,7 +84,7 @@
return ((ThreadPoolExecutor) this.execService).getActiveCount();
}

public Map<Integer, WorkerTaskExecuteRunnable> getTaskExecuteThreadMap() {
public Map<Integer, WorkerTaskExecutor> getTaskExecuteThreadMap() {

Check notice

Code scanning / CodeQL

Exposing internal representation Note

getTaskExecuteThreadMap exposes the internal representation stored in field taskExecuteThreadMap. The value may be modified
after this call to getTaskExecuteThreadMap
Loading
.
.../java/org/apache/dolphinscheduler/server/master/runner/PriorityDelayTaskExecuteRunnable.java
public ProcessInstance getWorkflowInstance() {
return workflowInstance;
}
public abstract class PriorityDelayTaskExecuteRunnable extends BaseTaskExecuteRunnable implements Delayed {

Check warning

Code scanning / CodeQL

Inconsistent compareTo Warning

This class declares
compareTo
Loading
but inherits equals; the two could be inconsistent.
@ruanwenjun ruanwenjun force-pushed the dev_wenjun_moveDelayToMaster branch from 0ecec0f to 7f56f96 Compare December 5, 2023 11:20
caishunfeng
caishunfeng reviewed Dec 5, 2023
View reviewed changes
...a/org/apache/dolphinscheduler/server/master/runner/GlobalMasterTaskExecuteRunnableQueue.java Outdated Show resolved Hide resolved
...in/java/org/apache/dolphinscheduler/server/master/runner/GlobalTaskDispatchWaitingQueue.java
@@ -29,7 +27,7 @@
@Component
public class GlobalTaskDispatchWaitingQueue {

private final PriorityBlockingQueue<DefaultTaskExecuteRunnable> queue = new PriorityBlockingQueue<>();
private final DelayQueue<DefaultTaskExecuteRunnable> queue = new DelayQueue<>();
Copy link
Contributor

Choose a reason for hiding this comment

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

Should be DelayPriorityQueue?

Copy link
Member Author

@ruanwenjun ruanwenjun Dec 5, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

JDK doesn't have DelayPriorityQueue, the DelayQueue contains priority logic, in fact, the DelayQueue use PriorityQueue to store element.

.../dolphinscheduler/server/master/runner/operator/BaseTaskExecuteRunnableDispatchOperator.java Show resolved Hide resolved
.../java/org/apache/dolphinscheduler/server/worker/runner/DefaultWorkerTaskExecutorFactory.java Show resolved Hide resolved
@ruanwenjun ruanwenjun force-pushed the dev_wenjun_moveDelayToMaster branch 3 times, most recently from 1d78067 to 004ba76 Compare December 5, 2023 13:15
@ruanwenjun ruanwenjun force-pushed the dev_wenjun_moveDelayToMaster branch from 004ba76 to 9bda0bc Compare December 5, 2023 13:15
@sonarqubecloud SonarQubeCloud
Copy link

sonarqubecloud bot commented Dec 5, 2023

Please retry analysis of this Pull-Request directly on SonarCloud.

caishunfeng
caishunfeng approved these changes Dec 6, 2023
View reviewed changes
Copy link
Contributor

@caishunfeng caishunfeng left a comment

Choose a reason for hiding this comment

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

LGTM overall

@caishunfeng caishunfeng added this to the 3.3.0 milestone Dec 6, 2023
@caishunfeng caishunfeng changed the title Move delay calculation to Master [Improvement] Move delay calculation to Master Dec 6, 2023
@caishunfeng caishunfeng merged commit 2119e41 into apache:dev Dec 6, 2023
52 of 53 checks passed
@ruanwenjun ruanwenjun deleted the dev_wenjun_moveDelayToMaster branch December 6, 2023 02:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@caishunfeng caishunfeng caishunfeng approved these changes

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

@zhuangchong zhuangchong Awaiting requested review from zhuangchong

Assignees

@ruanwenjun ruanwenjun

Labels
3.3.0 backend improvement make more easy to user or prompt friendly ready-to-merge
Projects
None yet
Milestone
3.3.0
Development

Successfully merging this pull request may close these issues.
3 participants
@ruanwenjun @codecov-commenter @caishunfeng