-
Notifications
You must be signed in to change notification settings - Fork 28.3k
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
[SPARK-33912][SQL] Refactor DependencyUtils ivy property parameter #30928
Conversation
I see many parameter use null as default value, if we can refactor all parameter to Option? |
Test build #133381 has finished for PR 30928 at commit
|
Test build #133403 has finished for PR 30928 at commit
|
@@ -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"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems fine either way honestly.
My main concern is consistency - why are these options handled differently from those using OptionAssigner?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see, OptionAssigner takes an object or null, not an Option. Ok either way. It's fine as is now.
This reverts commit 978e994.
Test build #133416 has started for PR 30928 at commit |
var packages: String = null | ||
var repositories: String = null | ||
var ivyRepoPath: String = null | ||
var packages: Option[String] = None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry but why do we bother change this? Looks like there are a lot of null
s above too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry but why do we bother change this? Looks like there are a lot of
null
s 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 .
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
String
vsnull
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)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, wait for other committer's review.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd be neutral on it - but if we're just changing a few instances? not sure it's worth the inconsistency.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm neutral on this change, too. I find
xxx: String = null
in many places of thecore
package and I'm not sure that this partial fix can make any benefit.
Ok, revert this change. about this.How about current
Test build #133432 has finished for PR 30928 at commit
|
retest this please |
Test build #133468 has finished for PR 30928 at commit
|
retest this please |
Test build #133497 has finished for PR 30928 at commit
|
Test build #133511 has started for PR 30928 at commit |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not against it, but, is this simplifying anything?
retest this please |
Just a refactor about method parameter, to avoid handle null and empty string such as |
Test build #133552 has finished for PR 30928 at commit
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
if (packagesExclusions.nonEmpty) { | ||
packagesExclusions.map(_.split(",")).get | ||
} else { | ||
Nil | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Better to use idiomatic Option processing, either a match
or packagesExclusions.map(_.split(",")).getOrElse(Nil)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Better to use idiomatic Option processing, either a
match
orpackagesExclusions.map(_.split(",")).getOrElse(Nil)
DOne
} | ||
|
||
SparkSubmitUtils.resolveMavenCoordinates(packages, ivySettings, | ||
SparkSubmitUtils.resolveMavenCoordinates(packages.get, ivySettings, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be packages.orNull
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be
packages.orNull
?
Yea
Test build #133625 has finished for PR 30928 at commit
|
Kubernetes integration test starting |
Kubernetes integration test status success |
Test build #133636 has finished for PR 30928 at commit
|
Kubernetes integration test starting |
Kubernetes integration test status success |
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. |
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