[SPARK-33912][SQL] Refactor DependencyUtils ivy property parameter #30928

AngersZhuuuu · 2020-12-25T09:00:05Z

What changes were proposed in this pull request?

According to #29966 (comment)
we'd better refactor ivy property parameters.

Why are the changes needed?

Refactor code to use Option instead of null

Does this PR introduce any user-facing change?

No

How was this patch tested?

Existed UT

AngersZhuuuu · 2020-12-25T09:00:58Z

I see many parameter use null as default value, if we can refactor all parameter to Option?

AngersZhuuuu · 2020-12-25T09:02:21Z

FYI @xkrogen @maropu @dongjoon-hyun

SparkQA · 2020-12-25T09:05:00Z

Test build #133381 has finished for PR 30928 at commit 83686af.

This patch fails to build.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2020-12-27T09:14:13Z

Test build #133403 has finished for PR 30928 at commit 978e994.

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

srowen · 2020-12-27T16:53:06Z

core/src/main/scala/org/apache/spark/deploy/SparkSubmit.scala

@@ -588,7 +588,7 @@ private[spark] class SparkSubmit extends Logging {
      OptionAssigner(args.deployMode, ALL_CLUSTER_MGRS, ALL_DEPLOY_MODES,
        confKey = SUBMIT_DEPLOY_MODE.key),
      OptionAssigner(args.name, ALL_CLUSTER_MGRS, ALL_DEPLOY_MODES, confKey = "spark.app.name"),
-      OptionAssigner(args.ivyRepoPath, ALL_CLUSTER_MGRS, CLIENT, confKey = "spark.jars.ivy"),
+      OptionAssignerWrapper(args.ivyRepoPath, ALL_CLUSTER_MGRS, CLIENT, confKey = "spark.jars.ivy"),


Seems fine either way honestly.
My main concern is consistency - why are these options handled differently from those using OptionAssigner?

Seems fine either way honestly.
My main concern is consistency - why are these options handled differently from those using OptionAssigner?

Since I want to add a constructor to support Option[String]. Emmm seems will cause None.get exception.
Change to to use orNull

I see, OptionAssigner takes an object or null, not an Option. Ok either way. It's fine as is now.

This reverts commit 978e994.

SparkQA · 2020-12-28T02:01:29Z

Test build #133416 has started for PR 30928 at commit 7da53f2.

HyukjinKwon · 2020-12-28T06:54:05Z

core/src/main/scala/org/apache/spark/deploy/SparkSubmitArguments.scala

-  var packages: String = null
-  var repositories: String = null
-  var ivyRepoPath: String = null
+  var packages: Option[String] = None


Sorry but why do we bother change this? Looks like there are a lot of nulls above too.

Sorry but why do we bother change this? Looks like there are a lot of nulls above too.

Yea, that's why I asked about #30928 (comment)

Since want to refactor parameter to use Option[String], but here are just String, so change this place .

String vs null is a valid way probably in Java context. Option might be preferred since we're in Scala but I wouldn't change it just for this reason. The gain is small compared to the size of change and potential maintenance overhead.

String vs null is a valid way probably in Java context. Option might be preferred since we're in Scala but I wouldn't change it just for this reason. The gain is small compared to the size of change and potential maintenance overhead.

So would you prefer to keep current or revert this change and just make it as Option when use method

val resolvedMavenCoordinates = DependencyUtils.resolveMavenDependencies( packagesTransitive = true, Option(args.packagesExclusions), Option(args.packages), Option(args.repositories), Option(args.ivyRepoPath), Option(args.ivySettingsPath)

Yeah, I wouldn't particularly mind when I read these codes. If you still prefer, feel free to wait for other committers' approval or review. I don't mind if any committer prefers and wants to merge it in.

Ok， wait for other committer's review.

I'd be neutral on it - but if we're just changing a few instances? not sure it's worth the inconsistency.

I'd be neutral on it - but if we're just changing a few instances? not sure it's worth the inconsistency.

I decide to change this since there is already a Option[String]

var ivySettingsPath: Option[String] = None

I'm neutral on this change, too. I find xxx: String = null in many places of the core package and I'm not sure that this partial fix can make any benefit.

I'm neutral on this change, too. I find xxx: String = null in many places of the core package and I'm not sure that this partial fix can make any benefit.

Ok, revert this change. about this.How about current

SparkQA · 2020-12-28T15:29:28Z

Test build #133432 has finished for PR 30928 at commit e09d8e0.

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

AngersZhuuuu · 2020-12-29T05:19:30Z

retest this please

SparkQA · 2020-12-29T13:44:17Z

Test build #133468 has finished for PR 30928 at commit e09d8e0.

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

AngersZhuuuu · 2020-12-29T14:23:53Z

retest this please

SparkQA · 2020-12-29T23:26:05Z

Test build #133497 has finished for PR 30928 at commit e09d8e0.

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

SparkQA · 2020-12-30T01:10:04Z

Test build #133511 has started for PR 30928 at commit c7d1aee.

srowen

I'm not against it, but, is this simplifying anything?

AngersZhuuuu · 2020-12-31T02:41:04Z

retest this please

AngersZhuuuu · 2020-12-31T02:57:19Z

I'm not against it, but, is this simplifying anything?

Just a refactor about method parameter, to avoid handle null and empty string such as getOrElse(""), make these method more readable from a Scala perspective.

SparkQA · 2020-12-31T11:15:00Z

Test build #133552 has finished for PR 30928 at commit c7d1aee.

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

xkrogen

Current diff seems like a good tradeoff to me, I think it brings more consistency between the SparkSubmitUtils.(build|load)IvySettings and resolveMavenDependencies and generally makes the intent more clear. IMO Option is better, at least in this case where it isn't leveraged from Java code, and we're already using it in nearby places so even if it is a small improvement it's still helpful.

That being said, I recognize the previous comments that null is used in many other places as well, so we still haven't made everything consistent.

xkrogen · 2021-01-04T17:49:36Z

core/src/main/scala/org/apache/spark/util/DependencyUtils.scala

+      if (packagesExclusions.nonEmpty) {
+        packagesExclusions.map(_.split(",")).get
      } else {
        Nil
      }


Better to use idiomatic Option processing, either a match or packagesExclusions.map(_.split(",")).getOrElse(Nil)

Better to use idiomatic Option processing, either a match or packagesExclusions.map(_.split(",")).getOrElse(Nil)

DOne

xkrogen · 2021-01-04T17:49:57Z

core/src/main/scala/org/apache/spark/util/DependencyUtils.scala

    }

-    SparkSubmitUtils.resolveMavenCoordinates(packages, ivySettings,
+    SparkSubmitUtils.resolveMavenCoordinates(packages.get, ivySettings,


Should be packages.orNull ?

Should be packages.orNull ?

Yea

SparkQA · 2021-01-05T01:13:13Z

Test build #133625 has finished for PR 30928 at commit de0eee1.

This patch fails to build.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2021-01-05T03:56:06Z

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

SparkQA · 2021-01-05T04:27:50Z

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

SparkQA · 2021-01-05T05:01:10Z

Test build #133636 has finished for PR 30928 at commit de1225d.

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

SparkQA · 2021-01-05T06:02:01Z

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

SparkQA · 2021-01-05T06:34:39Z

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

github-actions · 2021-04-16T00:17:14Z

We're closing this PR because it hasn't been updated in a while. This isn't a judgement on the merit of the PR in any way. It's just a way of keeping the PR queue manageable.
If you'd like to revive this PR, please reopen it and ask a committer to remove the Stale tag!

[SPARK-33912][SQL] Refactor �DependencyUtils ivy property parameter

83686af

github-actions bot added the CORE label Dec 25, 2020

Update SparkSubmit.scala

978e994

srowen reviewed Dec 27, 2020

View reviewed changes

AngersZhuuuu added 2 commits December 28, 2020 09:28

Revert "Update SparkSubmit.scala"

23d4451

This reverts commit 978e994.

Update SparkSubmit.scala

7da53f2

HyukjinKwon reviewed Dec 28, 2020

View reviewed changes

Merge branch 'master' into SPARK-33912

e09d8e0

follow comment

c7d1aee

srowen reviewed Dec 31, 2020

View reviewed changes

xkrogen reviewed Jan 4, 2021

View reviewed changes

Update DependencyUtils.scala

de0eee1

Update DependencyUtils.scala

de1225d

github-actions bot added the Stale label Apr 16, 2021

github-actions bot closed this Apr 17, 2021

[SPARK-33912][SQL] Refactor DependencyUtils ivy property parameter #30928

[SPARK-33912][SQL] Refactor DependencyUtils ivy property parameter #30928

Conversation

AngersZhuuuu commented Dec 25, 2020

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

AngersZhuuuu commented Dec 25, 2020

AngersZhuuuu commented Dec 25, 2020

SparkQA commented Dec 25, 2020

SparkQA commented Dec 27, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SparkQA commented Dec 28, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AngersZhuuuu Dec 30, 2020 • edited

Choose a reason for hiding this comment

SparkQA commented Dec 28, 2020

AngersZhuuuu commented Dec 29, 2020

SparkQA commented Dec 29, 2020

AngersZhuuuu commented Dec 29, 2020

SparkQA commented Dec 29, 2020

SparkQA commented Dec 30, 2020

srowen left a comment

Choose a reason for hiding this comment

AngersZhuuuu commented Dec 31, 2020

AngersZhuuuu commented Dec 31, 2020

SparkQA commented Dec 31, 2020

xkrogen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SparkQA commented Jan 5, 2021

SparkQA commented Jan 5, 2021

SparkQA commented Jan 5, 2021

SparkQA commented Jan 5, 2021

SparkQA commented Jan 5, 2021

SparkQA commented Jan 5, 2021

github-actions bot commented Apr 16, 2021

AngersZhuuuu Dec 30, 2020 •

edited