Skip to content

Add base loop task execotor and http template parser#11137

Merged
ruanwenjun merged 5 commits intoapache:devfrom
ruanwenjun:dev_wenjun_addAbstractLoopTask
Jul 26, 2022
Merged

Add base loop task execotor and http template parser#11137
ruanwenjun merged 5 commits intoapache:devfrom
ruanwenjun:dev_wenjun_addAbstractLoopTask

Conversation

@ruanwenjun
Copy link
Copy Markdown
Member

@ruanwenjun ruanwenjun commented Jul 25, 2022

Purpose of the pull request

close #11103

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:

@SbloodyS SbloodyS added the improvement make more easy to user or prompt friendly label Jul 25, 2022
@SbloodyS SbloodyS added this to the 3.1.0-alpha milestone Jul 25, 2022
@ruanwenjun ruanwenjun force-pushed the dev_wenjun_addAbstractLoopTask branch from 9955559 to 1a97ee6 Compare July 25, 2022 13:02
caishunfeng
caishunfeng reviewed Jul 25, 2022
View reviewed changes
...eduler/plugin/task/api/loop/template/http/method/HttpLoopTaskCancelTaskMethodDefinition.java

@Override
public void cancelTaskInstance(LoopTaskInstanceInfo loopTaskInstanceInfo) {
if (requestParams != null) {
Copy link
Copy Markdown
Contributor

@caishunfeng caishunfeng Jul 25, 2022

Choose a reason for hiding this comment

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

It seems some same code.

Copy link
Copy Markdown
Member Author

@ruanwenjun ruanwenjun Jul 25, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Yes, right now there are some same code in the queryStatus/cancelTask, but these two methods will be different :) they may inject parameter from other place in the future.

@ruanwenjun ruanwenjun force-pushed the dev_wenjun_addAbstractLoopTask branch 6 times, most recently from 42c9ec8 to f530f85 Compare July 26, 2022 02:11
@zhuxt2015
Copy link
Copy Markdown
Contributor

zhuxt2015 commented Jul 26, 2022 •
edited
Loading

I have three small points:

  1. The use of the mode is slightly complicated, requiring a third-party platform to provide an interface without user authentication
  2. If the number of tasks is large, it may slow down the performance of the worker.
  3. I think the better approach is: When starting a workflow, a unique ID is returned as the ID of the workflow instance, third-party platform use the instance ID to query status of workflow instance and tasks.

@ruanwenjun WDYT?

@ruanwenjun
Copy link
Copy Markdown
Member Author

ruanwenjun commented Jul 26, 2022 •
edited
Loading

I have three small points:

  1. The use of the mode is slightly complicated, requiring a third-party platform to provide an interface without user authentication

In fact, there exist these kinds of tasks, ds is as a scheduler trigger to submit tasks to these platforms.

  1. If the number of tasks is large, it may slow down the performance of the worker.

Yes, we can use single thread pool to loop the status in the future, but I think this will not heavier than process.

  1. When starting a workflow, a unique ID is returned as the ID of the workflow instance, third-party platform use the instance ID to query status of workflow instance and tasks.

Third-party platform can call API-Server to query the status, this PR is used to provide a base task for query status task like EMR.

@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Jul 26, 2022 •
edited
Loading

Codecov Report

Merging #11137 (c7deb67) into dev (f13e7a9) will decrease coverage by 0.11%.
The diff coverage is 10.72%.

@@             Coverage Diff              @@
##                dev   #11137      +/-   ##
============================================
- Coverage     40.39%   40.28%   -0.12%     
- Complexity     4878     4897      +19     
============================================
  Files           950      970      +20     
  Lines         37186    37490     +304     
  Branches       4079     4119      +40     
============================================
+ Hits          15022    15102      +80     
- Misses        20642    20855     +213     
- Partials       1522     1533      +11
Impacted Files Coverage Δ
...dolphinscheduler/plugin/task/api/AbstractTask.java 0.00% <ø> (ø)
...olphinscheduler/plugin/task/api/TaskConstants.java 0.00% <0.00%> (ø)
...ler/plugin/task/api/loop/BaseLoopTaskExecutor.java 0.00% <0.00%> (ø)
...duler/plugin/task/api/loop/LoopTaskMethodType.java 0.00% <0.00%> (ø)
.../api/loop/LoopTaskQueryStatusMethodDefinition.java 0.00% <0.00%> (ø)
...emplate/http/BaseHttpTemplateLoopTaskExecutor.java 0.00% <0.00%> (ø)
...api/loop/template/http/HttpLoopTaskDefinition.java 0.00% <0.00%> (ø)
...i/loop/template/http/HttpLoopTaskInstanceInfo.java 0.00% <0.00%> (ø)
...loop/template/http/HttpLoopTaskInstanceStatus.java 0.00% <0.00%> (ø)
...op/template/http/HttpLoopTaskMethodDefinition.java 0.00% <0.00%> (ø)
... and 20 more

Help us with your feedback. Take ten seconds to tell us how you rate us.

@ruanwenjun ruanwenjun force-pushed the dev_wenjun_addAbstractLoopTask branch from f530f85 to c7deb67 Compare July 26, 2022 03:36
@sonarqubecloud
Copy link
Copy Markdown

sonarqubecloud bot commented Jul 26, 2022

SonarCloud Quality Gate failed.    Quality Gate failed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 33 Code Smells

9.0% 9.0% Coverage
0.0% 0.0% Duplication

@ruanwenjun ruanwenjun requested a review from caishunfeng July 26, 2022 05:34
caishunfeng
caishunfeng approved these changes Jul 26, 2022
View reviewed changes
Copy link
Copy Markdown
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

@ruanwenjun ruanwenjun merged commit c7789bf into apache:dev Jul 26, 2022
@ruanwenjun ruanwenjun deleted the dev_wenjun_addAbstractLoopTask branch July 26, 2022 06:51
@ruanwenjun ruanwenjun mentioned this pull request Jul 27, 2022
Merged
ruanwenjun added a commit to ruanwenjun/dolphinscheduler that referenced this pull request Aug 1, 2022
@ruanwenjun
Cherry-Pick: Add bom module, add loop task (apache#26)
590f3cc 
* Add dolphinscheduler-bom to manage the dependency version (apache#11025)

(cherry picked from commit 5e9c7da)

* Add bom module

* Add base loop task execotor and http template parser (apache#11137)

* Add base loop task execotor and http template parser

* Add JsonPathUtils

(cherry picked from commit c7789bf)
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

@kezhenxu94 kezhenxu94 Awaiting requested review from kezhenxu94

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

@zhuangchong zhuangchong Awaiting requested review from zhuangchong

Assignees

@ruanwenjun ruanwenjun

Labels

backend improvement make more easy to user or prompt friendly

Projects

None yet

Milestone

3.1.0

Development

Successfully merging this pull request may close these issues.

[Feature][Task] Add a new abstract task-AbstractLoopStateTask

5 participants

@ruanwenjun @zhuxt2015 @codecov-commenter @caishunfeng @SbloodyS