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.

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

[feature][task-flink] Support Flink application mode #9577

Merged
merged 1 commit into from
Jul 1, 2022

Conversation

paul8263
Copy link
Contributor

@paul8263 paul8263 commented Apr 19, 2022

Purpose of the pull request

Added feature that enables Flink to submit jobs in application mode.

This pr will close #9416

Brief change log

  • dolphinscheduler-task-plugin/dolphinscheduler-task-flink/src/main/java/org/apache/dolphinscheduler/plugin/task/flink/FlinkArgsUtils.java
  • dolphinscheduler-task-plugin/dolphinscheduler-task-flink/src/main/java/org/apache/dolphinscheduler/plugin/task/flink/FlinkConstants.java
  • dolphinscheduler-task-plugin/dolphinscheduler-task-flink/src/main/java/org/apache/dolphinscheduler/plugin/task/flink/FlinkDeployMode.java
  • dolphinscheduler-task-plugin/dolphinscheduler-task-flink/src/main/java/org/apache/dolphinscheduler/plugin/task/flink/FlinkParameters.java
  • dolphinscheduler-task-plugin/dolphinscheduler-task-flink/src/main/java/org/apache/dolphinscheduler/plugin/task/flink/FlinkTask.java
  • dolphinscheduler-task-plugin/dolphinscheduler-task-flink/src/test/java/org/apache/dolphinscheduler/plugin/task/flink/FlinkArgsUtilsTest.java
  • dolphinscheduler-ui-next/src/views/projects/task/components/node/fields/use-flink.ts
  • dolphinscheduler-ui/src/js/conf/home/pages/dag/_source/formModel/tasks/flink.vue

Verify this pull request

This change added tests and can be verified as follows:

Added test for dolphinscheduler-task-plugin/dolphinscheduler-task-flink/src/main/java/org/apache/dolphinscheduler/plugin/task/flink/FlinkArgsUtils.java

  • dolphinscheduler-task-plugin/dolphinscheduler-task-flink/src/test/java/org/apache/dolphinscheduler/plugin/task/flink/FlinkArgsUtilsTest.java

@codecov-commenter
Copy link

codecov-commenter commented Apr 19, 2022

Codecov Report

Merging #9577 (f5e4ff7) into dev (a5bbf78) will increase coverage by 0.12%.
The diff coverage is 35.37%.

❗ Current head f5e4ff7 differs from pull request most recent head 0dd6bd7. Consider uploading reports for the commit 0dd6bd7 to get more accurate results

@@             Coverage Diff              @@
##                dev    #9577      +/-   ##
============================================
+ Coverage     39.98%   40.11%   +0.12%     
- Complexity     4432     4489      +57     
============================================
  Files           831      836       +5     
  Lines         33314    33553     +239     
  Branches       3677     3708      +31     
============================================
+ Hits          13320    13459     +139     
- Misses        18766    18839      +73     
- Partials       1228     1255      +27     
Impacted Files Coverage Δ
...er/api/controller/ProcessDefinitionController.java 45.34% <0.00%> (-2.22%) ⬇️
...lphinscheduler/api/controller/UsersController.java 59.25% <0.00%> (-2.28%) ⬇️
...api/service/impl/ProcessDefinitionServiceImpl.java 31.07% <0.00%> (-0.67%) ⬇️
...i/service/impl/ProcessTaskRelationServiceImpl.java 21.03% <0.00%> (-0.64%) ⬇️
...scheduler/api/service/impl/ProjectServiceImpl.java 77.12% <0.00%> (-0.54%) ⬇️
.../org/apache/dolphinscheduler/common/Constants.java 80.95% <ø> (ø)
...che/dolphinscheduler/common/utils/HadoopUtils.java 30.39% <ø> (ø)
.../apache/dolphinscheduler/common/utils/S3Utils.java 0.00% <ø> (ø)
...heduler/dao/entity/DependentProcessDefinition.java 0.00% <ø> (ø)
...eduler/dao/entity/DependentSimplifyDefinition.java 0.00% <0.00%> (ø)
... and 39 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 a5bbf78...0dd6bd7. Read the comment docs.

@SbloodyS
Copy link
Member

I think your describe see issue 9416 is not work for this case.
to target specific issue from PR, we recommend use keyword fix: #issue_id or close: #issue_id or closes: #issue_id in your PR describe(not title, jus desc). I would not only connect issue to PR but also close issue automatically when PR is be closed.

@SbloodyS SbloodyS requested a review from songjianet April 20, 2022 06:13
@SbloodyS SbloodyS added the miss:docs missing documents in PR label Apr 20, 2022
@paul8263
Copy link
Contributor Author

The docs were updated in fff3d69

@SbloodyS SbloodyS added improvement make more easy to user or prompt friendly and removed miss:docs missing documents in PR labels Apr 20, 2022
@@ -24,7 +24,7 @@ Flink task type for executing Flink programs. For Flink nodes, the worker submit
- **Program type**: Supports Java, Scala and Python.
- **The class of main function**: The **full path** of Main Class, the entry point of the Flink program.
- **Main jar package**: The jar package of the Flink program (upload by Resource Center).
- **Deployment mode**: Support 2 deployment modes: cluster and local.
- **Deployment mode**: Support 3 deployment modes: cluster, local and application.
Copy link
Member

Choose a reason for hiding this comment

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

It is better to add an example about how to using flink in application mode

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Er.. The only difference is the run command. Should I add some explaination for those 3 modes or simply add a reference link to Flink official website?

Copy link
Member

Choose a reason for hiding this comment

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

I think adding some links to FL website is better, but please use the fix version docs and avoid using latest or stale url

@paul8263 paul8263 force-pushed the ISSUE-9416 branch 3 times, most recently from 0dd6bd7 to 784a0ca Compare April 24, 2022 01:25
SbloodyS
SbloodyS previously approved these changes May 5, 2022
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.

The backend part LGTM. Thanks for your contribution.

@zhongjiajie
Copy link
Member

@paul8263 have conflict in use-flink.ts, we remove module dolphinscheduler-ui and renamed dolphinscheduler-ui-next to dolphinscheduler-ui in #9909, So you have to migrate your change to dolphinscheduler-ui

@zhongjiajie
Copy link
Member

BTW, document change LGTM, we should ask @songjianet to see the frontend change after you solve the conflict

@paul8263
Copy link
Contributor Author

paul8263 commented May 9, 2022

Hi @zhongjiajie ,
Did I miss something? I rebased upstream/dev branch, solved the conflict and then force pushed my working branch. But GitHub says the conflict still exists.

@paul8263
Copy link
Contributor Author

Hi @SbloodyS and @zhongjiajie ,
Got lots of conflicts to resolve. The newly contributed codes seem to be not well structured. It may take some time for me to refactor and solve the conflicts.

@SbloodyS
Copy link
Member

Hi @SbloodyS and @zhongjiajie , Got lots of conflicts to resolve. The newly contributed codes seem to be not well structured. It may take some time for me to refactor and solve the conflicts.

Feel free to ping us when you are ready to review. ^_^

@paul8263
Copy link
Contributor Author

Hi @SbloodyS ,
I am confused about getting this error:
java.lang.RuntimeException: No download button in file manage list
at org.apache.dolphinscheduler.e2e.cases.FileManageE2ETest.testDownloadFile(FileManageE2ETest.java:303)

@paul8263
Copy link
Contributor Author

Hi @SbloodyS ,
Please review this issue as Flink is under development actively. Otherwise we have to frequently rebase and solve the conflicts. Thanks.

@SbloodyS
Copy link
Member

@labbomb @Amy0104 @devosend PTAL

@SbloodyS SbloodyS added this to the 3.1.0-alpha milestone May 18, 2022
() => {
model.deployMode = 'cluster'
}
)
Copy link
Member

Choose a reason for hiding this comment

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

There will be a bug here after setting deployMode to application and flinkVersion to 1.11.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @Amy0104 ,
I updated use-flink.ts. Now deploy mode will reset to 'cluster' only when flink version is '<1.10' and deploy mode is 'application'.

@paul8263
Copy link
Contributor Author

@SbloodyS ,
How could I rerun the test suite?

@paul8263
Copy link
Contributor Author

paul8263 commented Jun 2, 2022

Hi @SbloodyS ,
Could you help review this commit please?

SbloodyS
SbloodyS previously approved these changes Jun 5, 2022
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

@caishunfeng
Copy link
Contributor

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs Vulnerability A 0 Vulnerabilities Security Hotspot E 1 Security Hotspot Code Smell A 13 Code Smells

60.2% 60.2% Coverage 0.0% 0.0% Duplication

Hi @paul8263 please take a look of the security hotspots.

@paul8263
Copy link
Contributor Author

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed
Bug A 0 Bugs Vulnerability A 0 Vulnerabilities Security Hotspot E 1 Security Hotspot Code Smell A 13 Code Smells
60.2% 60.2% Coverage 0.0% 0.0% Duplication

Hi @paul8263 please take a look of the security hotspots.

Hi @caishunfeng ,
Thank you for reviewing. I think the script file permission should be RWXR_XR_X as in other tasks such DataxTask, generated scripts are all granted with the same permission.

@paul8263
Copy link
Contributor Author

paul8263 commented Jul 1, 2022

Hi @SbloodyS ,
Thanks your for previous suggestions and code reviewing.

Please remind all the reviewers to go over this PR. I am afraid if it was left untouched for a long time, it might diverge greatly from the dev branch. Solving the conflict might be time-consuming.

If there are some places that need to change, please let me know. Thank you very much.

@SbloodyS
Copy link
Member

SbloodyS commented Jul 1, 2022

@caishunfeng @zhongjiajie @Amy0104 @songjianet @zhuangchong Cound you guys spare some time to take a look at this PR? This PR has been over for 2 months.

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

@sonarqubecloud
Copy link

sonarqubecloud bot commented Jul 1, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot E 1 Security Hotspot
Code Smell A 19 Code Smells

62.3% 62.3% Coverage
0.0% 0.0% Duplication

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.

Copy link
Contributor

@zhuangchong zhuangchong left a comment

Choose a reason for hiding this comment

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

+1

@zhuangchong zhuangchong merged commit 4a3c3e7 into apache:dev Jul 1, 2022
welsh-wen pushed a commit to welsh-wen/dolphinscheduler that referenced this pull request Jul 2, 2022
zhongjiajie pushed a commit to zhongjiajie/dolphinscheduler that referenced this pull request Jul 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
improvement make more easy to user or prompt friendly
Projects
None yet
7 participants