Skip to content

[Feature-8485][task-plugin] add EMR task plugin to support submit task to AWS EMR#8503

Merged
zhongjiajie merged 20 commits into
apache:devfrom
ronyang1985:emr-task-plugin
Mar 1, 2022
Merged

[Feature-8485][task-plugin] add EMR task plugin to support submit task to AWS EMR#8503
zhongjiajie merged 20 commits into
apache:devfrom
ronyang1985:emr-task-plugin

Conversation

@ronyang1985
Copy link
Copy Markdown
Contributor

@ronyang1985 ronyang1985 commented Feb 23, 2022

Purpose of the pull request

close #8485

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:

@zhongjiajie
Copy link
Copy Markdown
Member

@ronyang1985 It seems we are without UI part, is there any plan to add UI part

@zhongjiajie
Copy link
Copy Markdown
Member

I restart the failing test.

@ronyang1985
Copy link
Copy Markdown
Contributor Author

@ronyang1985 It seems we are without UI part, is there any plan to add UI part

@zhongjiajie I am not very good at front-end development, the ui part may be added after ui-next is completed by other contributors?

@zhongjiajie
Copy link
Copy Markdown
Member

@ronyang1985 It seems we are without UI part, is there any plan to add UI part

@zhongjiajie I am not very good at front-end development, the ui part may be added after ui-next is completed by other contributors?

I got it, cc: @songjianet . I also add the label miss document to this PR, to remind us to need to add a document about it

@zhongjiajie
Copy link
Copy Markdown
Member

FYI, UT do not pass

@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Feb 24, 2022

Codecov Report

Merging #8503 (285dcf6) into dev (60ddede) will decrease coverage by 0.06%.
The diff coverage is 42.95%.

❗ Current head 285dcf6 differs from pull request most recent head 62f6378. Consider uploading reports for the commit 62f6378 to get more accurate results

Impacted file tree graph

@@             Coverage Diff              @@
##                dev    #8503      +/-   ##
============================================
- Coverage     45.38%   45.32%   -0.07%     
- Complexity     4031     4060      +29     
============================================
  Files           686      696      +10     
  Lines         26725    26978     +253     
  Branches       2870     2889      +19     
============================================
+ Hits          12130    12227      +97     
- Misses        13451    13600     +149     
- Partials       1144     1151       +7     
Impacted Files Coverage Δ
...org/apache/dolphinscheduler/alert/AlertServer.java 56.41% <0.00%> (-3.05%) ⬇️
.../org/apache/dolphinscheduler/api/dto/AuditDto.java 68.75% <ø> (ø)
...apache/dolphinscheduler/api/dto/ScheduleParam.java 0.00% <ø> (ø)
...rg/apache/dolphinscheduler/api/dto/gantt/Task.java 0.00% <ø> (ø)
...he/dolphinscheduler/api/dto/treeview/Instance.java 0.00% <ø> (ø)
...heduler/api/service/impl/SchedulerServiceImpl.java 8.62% <0.00%> (-0.07%) ⬇️
...er/api/service/impl/TaskDefinitionServiceImpl.java 22.69% <0.00%> (-0.50%) ⬇️
...cheduler/common/enums/ComplementDependentMode.java 0.00% <0.00%> (ø)
...he/dolphinscheduler/common/model/DateInterval.java 38.88% <ø> (ø)
...g/apache/dolphinscheduler/common/model/Server.java 0.00% <ø> (ø)
... and 80 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 60ddede...62f6378. Read the comment docs.

@ronyang1985
Copy link
Copy Markdown
Contributor Author

ronyang1985 commented Feb 24, 2022

@ronyang1985 It seems we are without UI part, is there any plan to add UI part

@zhongjiajie I am not very good at front-end development, the ui part may be added after ui-next is completed by other contributors?

I got it, cc: @songjianet . I also add the label miss document to this PR, to remind us to need to add a document about it

@zhongjiajie hi, about the document, i submit a pr ,apache/dolphinscheduler-website#702

…ler/common/enums/TaskType.java


learned it, thx

Co-authored-by: Wenjun Ruan <wenjun@apache.org>
ronyang1985 and others added 3 commits February 24, 2022 18:14
…n/java/org/apache/dolphinscheduler/plugin/task/emr/EmrTask.java

Co-authored-by: caishunfeng <caishunfeng2021@gmail.com>
Copy link
Copy Markdown
Member

@ruanwenjun ruanwenjun left a comment

Choose a reason for hiding this comment

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

When worker submit an EMR task, it will query the task status from remote server and wait the task success.

Can the task get status by callback function? I am not familiar with AWS EMR.

It seems in the current implementation will have issue when do task failover, since the master cannot get EMR appId(like spark applicaId), the id will only send to master after the task finished.

If I miss anything please let me know.

@lenboo
Copy link
Copy Markdown
Contributor

lenboo commented Feb 25, 2022

Yes, we have some problems in different tasks failover. Now, we get yarn ids from task log, and kill yarn id in task failover. But it cannot support custom unknown tasks, such as k8s and EMR tasks. So we should consider reconstructing the process of task failover.

@ruanwenjun ruanwenjun closed this Feb 25, 2022
@ruanwenjun ruanwenjun reopened this Feb 25, 2022
@ruanwenjun
Copy link
Copy Markdown
Member

Clicked the close button by mistake, this PR is ok for me. According to @lenboo said, we can reconstruct the process of task failover in the future. Currently, we can just add TODO to describe the possible failover issue in code.

@ronyang1985
Copy link
Copy Markdown
Contributor Author

Clicked the close button by mistake, this PR is ok for me. According to @lenboo said, we can reconstruct the process of task failover in the future. Currently, we can just add TODO to describe the possible failover issue in code.

hi, 'TODO' comment was added

@sonarqubecloud
Copy link
Copy Markdown

SonarCloud Quality Gate failed.    Quality Gate failed

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

68.3% 68.3% Coverage
7.9% 7.9% Duplication

@ronyang1985 ronyang1985 requested a review from ruanwenjun March 1, 2022 01:44
Copy link
Copy Markdown
Member

@ruanwenjun ruanwenjun left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Copy Markdown
Member

@zhongjiajie zhongjiajie left a comment

Choose a reason for hiding this comment

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

LGTM

@zhongjiajie zhongjiajie merged commit 20ee9a3 into apache:dev Mar 1, 2022
@zhongjiajie zhongjiajie removed the miss:docs missing documents in PR label Mar 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Feature][task-plugin] add EMR task plugin to support submit task to AWS EMR

6 participants