Skip to content

[Feature-16127] Support emr serverless spark #16126

New issue

Have a question about this project? No Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

No Sign up for GitHub

By clicking “No 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? No Sign in to your account

Jump to bottom
Merged
merged 18 commits into from
Aug 2, 2024
Merged

[Feature-16127] Support emr serverless spark #16126

merged 18 commits into from
Aug 2, 2024

Conversation

EricGao888
Copy link
Member

@EricGao888 EricGao888 commented Jun 7, 2024 •
edited
Loading

Purpose of the pull request

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:

(or)

If your pull request contain incompatible change, you should also add it to docs/docs/en/guide/upgrede/incompatible.md

@EricGao888 EricGao888 self-assigned this Jun 7, 2024
@github-actions github-actions bot added UI ui and front end related backend document labels Jun 7, 2024
@EricGao888 EricGao888 changed the title [Feature] [Task Plugin] Support emr serverless spark community [Feature] [Task Plugin] Support emr serverless spark Jun 7, 2024
@SbloodyS SbloodyS added the 3.3.0 label Jun 7, 2024
@SbloodyS SbloodyS added this to the 3.3.0 milestone Jun 7, 2024
@codecov-commenter
Copy link

codecov-commenter commented Jun 7, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 63.42593% with 79 lines in your changes missing coverage. Please review.

Project coverage is 40.81%. Comparing base (834c320) to head (ee8d664).

❗ Current head ee8d664 differs from pull request most recent head 0552a93

Please upload reports for the commit 0552a93 to get more accurate results.

Files Patch % Lines
...iyunserverlessspark/AliyunServerlessSparkTask.java 74.75% 21 Missing and 5 partials ⚠️
...aram/AliyunServerlessSparkDataSourceProcessor.java 75.51% 10 Missing and 2 partials ⚠️
...yunserverlessspark/AliyunServerlessSparkUtils.java 0.00% 8 Missing ⚠️
...rlessspark/AliyunServerlessSparkTaskException.java 0.00% 6 Missing ⚠️
...verlessspark/AliyunServerlessSparkTaskChannel.java 0.00% 5 Missing ⚠️
...spark/AliyunServerlessSparkTaskChannelFactory.java 0.00% 4 Missing ⚠️
...rlessspark/AliyunServerlessSparkClientWrapper.java 80.00% 3 Missing ⚠️
...sspark/AliyunServerlessSparkDataSourceChannel.java 0.00% 3 Missing ⚠️
...AliyunServerlessSparkDataSourceChannelFactory.java 0.00% 3 Missing ⚠️
...rverlessspark/AliyunServerlessSparkParameters.java 40.00% 3 Missing ⚠️
... and 3 more
Additional details and impacted files 
@@             Coverage Diff              @@
##                dev   #16126      +/-   ##
============================================
+ Coverage     40.71%   40.81%   +0.10%     
- Complexity     5246     5273      +27     
============================================
  Files          1385     1397      +12     
  Lines         46109    46324     +215     
  Branches       4923     4931       +8     
============================================
+ Hits          18771    18908     +137     
- Misses        25412    25482      +70     
- Partials       1926     1934       +8

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@EricGao888 EricGao888 force-pushed the support-emr-serverless-spark-community branch from 33cc954 to c107f11 Compare June 11, 2024 03:32
@EricGao888 EricGao888 marked this pull request as ready for review June 11, 2024 06:25
SbloodyS
SbloodyS reviewed Jun 11, 2024
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.

Gernerally LGTM. Just some questions.

...hinscheduler/plugin/datasource/aliyunserverlessspark/AliyunServerlessSparkClientWrapper.java Outdated
Comment on lines 52 to 53
} catch (Exception e) {
log.info("spark client failed to connect to the server");
Copy link
Member

Choose a reason for hiding this comment

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

Is it better to print stack trace to the logs for better debug?

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks, I missed this one, will fix it.

Copy link
Member Author

Choose a reason for hiding this comment

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

@SbloodyS Fixed : )

@EricGao888 EricGao888 requested a review from ruanwenjun June 11, 2024 11:19
lenboo
lenboo reviewed Jun 12, 2024
View reviewed changes
...org/apache/dolphinscheduler/plugin/task/aliyunserverlessspark/AliyunServerlessSparkTask.java Outdated

protected Client buildAliyunServerlessSparkClient(String accessKeyId, String accessKeySecret,
String regionId) throws Exception {
String endpoint = String.format("emr-serverless-spark.%s.aliyuncs.com", regionId);
Copy link
Contributor

Choose a reason for hiding this comment

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

I have seen this string in some other places, maybe a constant is more suitable.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure, will fix it.

Copy link
Member Author

Choose a reason for hiding this comment

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

@lenboo Fixed : )

rickchengx
rickchengx previously approved these changes Jun 12, 2024
View reviewed changes
...hinscheduler/plugin/datasource/aliyunserverlessspark/AliyunServerlessSparkClientWrapper.java Outdated
throws Exception {

checkNotNull(accessKeyId, accessKeySecret, regionId);
String endpoint = String.format("emr-serverless-spark.%s.aliyuncs.com", regionId);
Copy link
Contributor

@rickchengx rickchengx Jun 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Overall LGTM, maybe we should make endpoint as a config since users may use endpoint in other formats (etc., in VPC).

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point. Will make it configurable.

Copy link
Member Author

Choose a reason for hiding this comment

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

@rickchengx Endpoint added : )

@sonarqubecloud SonarQubeCloud
Copy link

Please retry analysis of this Pull-Request directly on SonarCloud

@EricGao888 EricGao888 requested a review from Gallardot as a code owner July 24, 2024 10:13
@github-actions github-actions bot added the test label Jul 24, 2024
@EricGao888 EricGao888 force-pushed the support-emr-serverless-spark-community branch from 26c4c8c to 64fa932 Compare August 1, 2024 11:19
@EricGao888 EricGao888 force-pushed the support-emr-serverless-spark-community branch from 64fa932 to 208ecc4 Compare August 1, 2024 11:27
@EricGao888 EricGao888 changed the title [Feature] [Task Plugin] Support emr serverless spark [Feature] Support emr serverless spark Aug 1, 2024
@EricGao888 EricGao888 changed the title [Feature] Support emr serverless spark [Feature-16127] Support emr serverless spark Aug 1, 2024
SbloodyS
SbloodyS reviewed Aug 2, 2024
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.

Generally LGTM. Just some NIT.

SbloodyS
SbloodyS approved these changes Aug 2, 2024
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.

LGTM

@sonarqubecloud SonarQubeCloud
Copy link

sonarqubecloud bot commented Aug 2, 2024

Quality Gate Failed Quality Gate failed

Failed conditions
C Reliability Rating on New Code (required ≥ A)

See analysis details on SonarCloud

Catch issues before they fail your Quality Gate with our IDE extension SonarLint

rickchengx
rickchengx approved these changes Aug 2, 2024
View reviewed changes
Copy link
Contributor

@rickchengx rickchengx left a comment

Choose a reason for hiding this comment

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

LGTM

@EricGao888 EricGao888 merged commit f7da8f3 into apache:dev Aug 2, 2024
63 of 65 checks passed
@EricGao888 EricGao888 mentioned this pull request Aug 21, 2024
Closed
3 tasks
@SbloodyS SbloodyS added the feature new feature label Aug 30, 2024
No Sign up for free to join this conversation on GitHub. Already have an account? No Sign in to comment
Reviewers

@lenboo lenboo lenboo left review comments

@SbloodyS SbloodyS SbloodyS approved these changes

@rickchengx rickchengx rickchengx approved these changes

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

@Amy0104 Amy0104 Awaiting requested review from Amy0104

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

@zhuangchong zhuangchong Awaiting requested review from zhuangchong

@ruanwenjun ruanwenjun Awaiting requested review from ruanwenjun

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

Assignees

@EricGao888 EricGao888

Labels
3.3.0 backend document feature new feature test UI ui and front end related
Projects
None yet
Milestone
3.3.0-alpha
Development

Successfully merging this pull request may close these issues.
5 participants
@EricGao888 @codecov-commenter @SbloodyS @lenboo @rickchengx