Skip to content

[Feature][Task Plugin] Increase zeppelin task stability in production#11010

Merged
Amy0104 merged 6 commits intoapache:devfrom
EricGao888:Fix-10584
Jul 20, 2022
Merged

[Feature][Task Plugin] Increase zeppelin task stability in production#11010
Amy0104 merged 6 commits intoapache:devfrom
EricGao888:Fix-10584

Conversation

@EricGao888
Copy link
Copy Markdown
Member

@EricGao888 EricGao888 commented Jul 16, 2022

Purpose of the pull request

Brief change log

  • If a user set a production directory in zeppelin task, the zeppelin task plugin will clone a note to the production directory and run it instead of the original one to ensure stability. In that case, if the user somehow modifies a running note, the production task will not get influenced.

Verify this pull request

  • Still WIP, will add the front-end and update the docs.

@EricGao888 EricGao888 changed the title [Feature][Task Plugin] Increase zeppelin task stability in production (#10584) [Feature][Task Plugin] Increase zeppelin task stability in production Jul 16, 2022
@github-actions github-actions bot added document UI ui and front end related labels Jul 16, 2022
@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Jul 16, 2022 •
edited
Loading

Codecov Report

Merging #11010 (32b18a6) into dev (083ab2b) will decrease coverage by 0.00%.
The diff coverage is 90.90%.

❗ Current head 32b18a6 differs from pull request most recent head 313e3ff. Consider uploading reports for the commit 313e3ff to get more accurate results

@@             Coverage Diff              @@
##                dev   #11010      +/-   ##
============================================
- Coverage     40.36%   40.35%   -0.01%     
+ Complexity     4865     4864       -1     
============================================
  Files           943      943              
  Lines         37061    37062       +1     
  Branches       4068     4070       +2     
============================================
  Hits          14958    14958              
  Misses        20600    20600              
- Partials       1503     1504       +1
Impacted Files Coverage Δ
...er/master/builder/TaskExecutionContextBuilder.java 64.70% <ø> (ø)
...er/server/master/event/TaskResultEventHandler.java 0.00% <ø> (ø)
...r/server/master/runner/task/BaseTaskProcessor.java 13.43% <ø> (ø)
...ver/master/runner/task/DependentTaskProcessor.java 5.33% <ø> (ø)
...server/master/runner/task/SwitchTaskProcessor.java 2.58% <ø> (ø)
...r/server/master/service/MasterFailoverService.java 56.77% <ø> (ø)
...r/server/master/service/WorkerFailoverService.java 50.00% <ø> (ø)
...server/master/utils/DataQualityResultOperator.java 0.00% <ø> (ø)
...cheduler/server/master/utils/DependentExecute.java 0.00% <ø> (ø)
...scheduler/server/master/utils/SwitchTaskUtils.java 0.00% <ø> (ø)
... and 4 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 083ab2b...313e3ff. Read the comment docs.

@EricGao888 EricGao888 marked this pull request as ready for review July 17, 2022 00:14
SbloodyS
SbloodyS previously approved these changes Jul 17, 2022
View reviewed changes
Copy link
Copy Markdown
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.

Backend part LGTM.

kezhenxu94
kezhenxu94 reviewed Jul 17, 2022
View reviewed changes
...pelin/src/main/java/org/apache/dolphinscheduler/plugin/task/zeppelin/ZeppelinParameters.java Outdated
return "ZeppelinParameters{"
+ "noteId='" + noteId + '\''
+ ", paragraphId='" + paragraphId + '\''
+ ", paragraphId='" + paragraphId + '\''
Copy link
Copy Markdown
Member

@kezhenxu94 kezhenxu94 Jul 17, 2022

Choose a reason for hiding this comment

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

Can you help to also refactor this class to use lombok annotations?

Copy link
Copy Markdown
Member Author

@EricGao888 EricGao888 Jul 17, 2022

Choose a reason for hiding this comment

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

@kezhenxu94 Certainly yes! I'm a lombok fan, lol.

@EricGao888
Copy link
Copy Markdown
Member Author

EricGao888 commented Jul 17, 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 0 Code Smells

23.3% 23.3% Coverage 0.0% 0.0% Duplication

Coverage seems decreasing, is it because of setters and getters being removed?

@EricGao888 EricGao888 self-assigned this Jul 17, 2022
@EricGao888
Copy link
Copy Markdown
Member Author

EricGao888 commented Jul 19, 2022

Hi @kezhenxu94, could you plz take another look when available? Thx~

@sonarqubecloud
Copy link
Copy Markdown

sonarqubecloud bot commented Jul 19, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

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

93.3% 93.3% Coverage
0.0% 0.0% Duplication

@EricGao888
Copy link
Copy Markdown
Member Author

EricGao888 commented Jul 19, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

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

93.3% 93.3% Coverage 0.0% 0.0% Duplication

Nailed it. [AT]Data annotation generates too much unrelated stuff. Replacing it with [AT] Setters, Getters, ToString fixes the test coverage.

kezhenxu94
kezhenxu94 approved these changes Jul 20, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@kezhenxu94 kezhenxu94 left a comment

Choose a reason for hiding this comment

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

backend part LGTM

Amy0104
Amy0104 approved these changes Jul 20, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@Amy0104 Amy0104 left a comment

Choose a reason for hiding this comment

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

The front end part LGTM.

@Amy0104 Amy0104 merged commit f689220 into apache:dev Jul 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@kezhenxu94 kezhenxu94 kezhenxu94 approved these changes

@Amy0104 Amy0104 Amy0104 approved these changes

@zhongjiajie zhongjiajie Awaiting requested review from zhongjiajie

@Tianqi-Dotes Tianqi-Dotes Awaiting requested review from Tianqi-Dotes

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

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

@zhuangchong zhuangchong Awaiting requested review from zhuangchong

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

Assignees

@EricGao888 EricGao888

Labels

backend document UI ui and front end related

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[Feature][Task Plugin] Zeppelin Task will run a duplicate note instead of the original one

5 participants

@EricGao888 @codecov-commenter @kezhenxu94 @SbloodyS @Amy0104