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

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

Merged
merged 6 commits into from
Jul 20, 2022
Merged

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

merged 6 commits into from
Jul 20, 2022

Conversation

EricGao888
Copy link
Member

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

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
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
@Override
public String toString() {
return "ZeppelinParameters{"
+ "noteId='" + noteId + '\''
+ ", paragraphId='" + paragraphId + '\''
+ ", paragraphId='" + paragraphId + '\''
Copy link
Member

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
Member Author

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
Member Author

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
Member Author

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

@sonarcloud
Copy link

sonarcloud 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
Member Author

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
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
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

@Amy0104 Amy0104 Amy0104 approved these changes

@kezhenxu94 kezhenxu94 kezhenxu94 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