Skip to content

[SPARK-33908][CORE] Refactor SparkSubmitUtils.resolveMavenCoordinates() 's return parameter #30922

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
Closed
wants to merge 3 commits into from
Closed

[SPARK-33908][CORE] Refactor SparkSubmitUtils.resolveMavenCoordinates() 's return parameter #30922

wants to merge 3 commits into from

Conversation

@AngersZhuuuu
Copy link
Contributor

@AngersZhuuuu AngersZhuuuu commented Dec 25, 2020

What changes were proposed in this pull request?

Per discuss in #29966 (comment)
We'd better change SparkSubmitUtils.resolveMavenCoordinates() 's return value as Seq[String]

Why are the changes needed?

refactor code

Does this PR introduce any user-facing change?

No

How was this patch tested?

Existed UT

@AngersZhuuuu
Copy link
Contributor Author

AngersZhuuuu commented Dec 25, 2020

FYI @xkrogen @maropu @dongjoon-hyun

@SparkQA
Copy link

SparkQA commented Dec 25, 2020

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/37955/

@SparkQA
Copy link

SparkQA commented Dec 25, 2020

Kubernetes integration test status failure
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/37955/

maropu
maropu reviewed Dec 25, 2020
View reviewed changes
@maropu
Copy link
Member

maropu commented Dec 25, 2020

Looks fine otherwise if the tests pass.

@SparkQA
Copy link

SparkQA commented Dec 25, 2020

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/37969/

@SparkQA
Copy link

SparkQA commented Dec 25, 2020

Kubernetes integration test status success
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/37969/

@SparkQA
Copy link

SparkQA commented Dec 25, 2020

Test build #133378 has finished for PR 30922 at commit 4d88496.

  • This patch fails Spark unit tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@SparkQA
Copy link

SparkQA commented Dec 25, 2020

Test build #133364 has finished for PR 30922 at commit c93f585.

  • This patch fails from timeout after a configured wait of 500m.
  • This patch merges cleanly.
  • This patch adds no public classes.

@AngersZhuuuu
Copy link
Contributor Author

AngersZhuuuu commented Dec 25, 2020

retest this please

@SparkQA
Copy link

SparkQA commented Dec 25, 2020

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/37976/

@SparkQA
Copy link

SparkQA commented Dec 25, 2020

Kubernetes integration test status success
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/37976/

@SparkQA
Copy link

SparkQA commented Dec 25, 2020

Test build #133384 has finished for PR 30922 at commit 4d88496.

  • This patch fails SparkR unit tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@AngersZhuuuu
Copy link
Contributor Author

AngersZhuuuu commented Dec 26, 2020

retest this please

@SparkQA
Copy link

SparkQA commented Dec 26, 2020

Test build #133390 has finished for PR 30922 at commit 4d88496.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@AngersZhuuuu
Copy link
Contributor Author

AngersZhuuuu commented Dec 28, 2020

Gentle ping @maropu

@HyukjinKwon
Copy link
Member

HyukjinKwon commented Dec 28, 2020

Merged to master.

xkrogen
xkrogen reviewed Jan 4, 2021
View reviewed changes
Copy link
Contributor

@xkrogen xkrogen left a comment

Choose a reason for hiding this comment

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

Many thanks for this change @AngersZhuuuu ! Looks much cleaner this way.

core/src/main/scala/org/apache/spark/deploy/SparkSubmit.scala
* @param artifacts Sequence of dependencies that were resolved and retrieved
* @param cacheDirectory directory where jars are cached
* @return a comma-delimited list of paths for the dependencies
*/
Copy link
Contributor

@xkrogen xkrogen Jan 4, 2021

Choose a reason for hiding this comment

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

@AngersZhuuuu FYI you missed updating the @return Scaladoc tag here, as well as the description which explicitly mentions a comma-delimited list. Would you mind submitting a follow-on to update?

@AngersZhuuuu AngersZhuuuu mentioned this pull request Jan 4, 2021
Closed
dongjoon-hyun pushed a commit that referenced this pull request Jan 4, 2021
@AngersZhuuuu @dongjoon-hyun
[SPARK-33908][CORE][FOLLOWUP] Correct Scaladoc of resolveDependencyPa…
559f411 
…ths/resolveMavenDependencies

### What changes were proposed in this pull request?
Fix un-correct doc of last change #30922 (comment)

### Why are the changes needed?
FIx doc

### Does this PR introduce _any_ user-facing change?
No

### How was this patch tested?
Builds finished correctly.

Closes #31016 from AngersZhuuuu/SPARK-33908-FOLLOW-UP.

Authored-by: angerszhu <[email protected]>
Signed-off-by: Dongjoon Hyun <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@maropu maropu maropu left review comments

@srowen srowen srowen approved these changes

@HyukjinKwon HyukjinKwon HyukjinKwon approved these changes

+1 more reviewer

@xkrogen xkrogen xkrogen left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

CORE SQL

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@AngersZhuuuu @SparkQA @maropu @HyukjinKwon @srowen @xkrogen