Skip to content

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

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

Closed
AngersZhuuuu wants to merge 8 commits into from
Closed

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

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
83686af
[SPARK-33912][SQL] Refactor DependencyUtils ivy property parameter
AngersZhuuuu Dec 25, 2020
978e994
Update SparkSubmit.scala
AngersZhuuuu Dec 27, 2020
23d4451
Revert "Update SparkSubmit.scala"
AngersZhuuuu Dec 28, 2020
7da53f2
Update SparkSubmit.scala
AngersZhuuuu Dec 28, 2020
e09d8e0
Merge branch 'master' into SPARK-33912
AngersZhuuuu Dec 28, 2020
c7d1aee
follow comment
AngersZhuuuu Dec 30, 2020
de0eee1
Update DependencyUtils.scala
AngersZhuuuu Jan 4, 2021
de1225d
Update DependencyUtils.scala
AngersZhuuuu Jan 5, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 6 additions & 6 deletions core/src/main/scala/org/apache/spark/deploy/SparkSubmit.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"),
OptionAssigner(args.ivyRepoPath.orNull, ALL_CLUSTER_MGRS, CLIENT, confKey = "spark.jars.ivy"),
OptionAssigner(args.driverMemory, ALL_CLUSTER_MGRS, CLIENT,
confKey = DRIVER_MEMORY.key),
OptionAssigner(args.driverExtraClassPath, ALL_CLUSTER_MGRS, ALL_DEPLOY_MODES,
Expand All @@ -604,13 +604,13 @@ private[spark] class SparkSubmit extends Logging {
OptionAssigner(args.pyFiles, ALL_CLUSTER_MGRS, CLUSTER, confKey = SUBMIT_PYTHON_FILES.key),

// Propagate attributes for dependency resolution at the driver side
OptionAssigner(args.packages, STANDALONE | MESOS | KUBERNETES,
OptionAssigner(args.packages.orNull, STANDALONE | MESOS | KUBERNETES,
CLUSTER, confKey = "spark.jars.packages"),
OptionAssigner(args.repositories, STANDALONE | MESOS | KUBERNETES,
OptionAssigner(args.repositories.orNull, STANDALONE | MESOS | KUBERNETES,
CLUSTER, confKey = "spark.jars.repositories"),
OptionAssigner(args.ivyRepoPath, STANDALONE | MESOS | KUBERNETES,
OptionAssigner(args.ivyRepoPath.orNull, STANDALONE | MESOS | KUBERNETES,
CLUSTER, confKey = "spark.jars.ivy"),
OptionAssigner(args.packagesExclusions, STANDALONE | MESOS | KUBERNETES,
OptionAssigner(args.packagesExclusions.orNull, STANDALONE | MESOS | KUBERNETES,
CLUSTER, confKey = "spark.jars.excludes"),

// Yarn only
Expand Down Expand Up @@ -646,7 +646,7 @@ private[spark] class SparkSubmit extends Logging {
confKey = DRIVER_CORES.key),
OptionAssigner(args.supervise.toString, STANDALONE | MESOS, CLUSTER,
confKey = DRIVER_SUPERVISE.key),
OptionAssigner(args.ivyRepoPath, STANDALONE, CLUSTER, confKey = "spark.jars.ivy"),
OptionAssigner(args.ivyRepoPath.orNull, STANDALONE, CLUSTER, confKey = "spark.jars.ivy"),

// An internal option used only for spark-shell to add user jars to repl's classloader,
// previously it uses "spark.jars" or "spark.yarn.dist.jars" which now may be pointed to
Expand Down
24 changes: 11 additions & 13 deletions core/src/main/scala/org/apache/spark/deploy/SparkSubmitArguments.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,11 +60,11 @@ private[deploy] class SparkSubmitArguments(args: Seq[String], env: Map[String, S
var name: String = null
var childArgs: ArrayBuffer[String] = new ArrayBuffer[String]()
var jars: String = null
var packages: String = null
var repositories: String = null
var ivyRepoPath: String = null
var packages: Option[String] = None
Copy link
Member

@HyukjinKwon HyukjinKwon Dec 28, 2020

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 nulls above too.

Copy link
Contributor Author

@AngersZhuuuu AngersZhuuuu Dec 28, 2020

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 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 .

Copy link
Member

@HyukjinKwon HyukjinKwon Dec 28, 2020

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.

Copy link
Contributor Author

@AngersZhuuuu AngersZhuuuu Dec 28, 2020

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.

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)

Copy link
Member

@HyukjinKwon HyukjinKwon Dec 28, 2020

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.

Copy link
Contributor Author

@AngersZhuuuu AngersZhuuuu Dec 28, 2020

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.

Copy link
Member

@srowen srowen Dec 28, 2020

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.

Copy link
Contributor Author

@AngersZhuuuu AngersZhuuuu Dec 28, 2020

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

Copy link
Member

@maropu maropu Dec 30, 2020

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.

Copy link
Contributor Author

@AngersZhuuuu AngersZhuuuu Dec 30, 2020 •
edited
Loading

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.

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

var repositories: Option[String] = None
var ivyRepoPath: Option[String] = None
var ivySettingsPath: Option[String] = None
var packagesExclusions: String = null
var packagesExclusions: Option[String] = None
var verbose: Boolean = false
var isPython: Boolean = false
var pyFiles: String = null
Expand Down Expand Up @@ -185,13 +185,11 @@ private[deploy] class SparkSubmitArguments(args: Seq[String], env: Map[String, S
files = Option(files).orElse(sparkProperties.get(config.FILES.key)).orNull
archives = Option(archives).orElse(sparkProperties.get(config.ARCHIVES.key)).orNull
pyFiles = Option(pyFiles).orElse(sparkProperties.get(config.SUBMIT_PYTHON_FILES.key)).orNull
ivyRepoPath = sparkProperties.get("spark.jars.ivy").orNull
ivyRepoPath = sparkProperties.get("spark.jars.ivy")
ivySettingsPath = sparkProperties.get("spark.jars.ivySettings")
packages = Option(packages).orElse(sparkProperties.get("spark.jars.packages")).orNull
packagesExclusions = Option(packagesExclusions)
.orElse(sparkProperties.get("spark.jars.excludes")).orNull
repositories = Option(repositories)
.orElse(sparkProperties.get("spark.jars.repositories")).orNull
packages = packages.orElse(sparkProperties.get("spark.jars.packages"))
packagesExclusions = packagesExclusions.orElse(sparkProperties.get("spark.jars.excludes"))
repositories = repositories.orElse(sparkProperties.get("spark.jars.repositories"))
deployMode = Option(deployMode)
.orElse(sparkProperties.get(config.SUBMIT_DEPLOY_MODE.key))
.orElse(env.get("DEPLOY_MODE"))
Expand Down Expand Up @@ -409,13 +407,13 @@ private[deploy] class SparkSubmitArguments(args: Seq[String], env: Map[String, S
jars = Utils.resolveURIs(value)

case PACKAGES =>
packages = value
packages = Some(value)

case PACKAGES_EXCLUDE =>
packagesExclusions = value
packagesExclusions = Some(value)

case REPOSITORIES =>
repositories = value
repositories = Some(value)

case CONF =>
val (confName, confValue) = SparkSubmitUtils.parseSparkConfProperty(value)
Expand Down
2 changes: 1 addition & 1 deletion core/src/main/scala/org/apache/spark/deploy/worker/DriverWrapper.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,7 @@ object DriverWrapper extends Logging {

val resolvedMavenCoordinates = DependencyUtils.resolveMavenDependencies(true,
ivyProperties.packagesExclusions, ivyProperties.packages, ivyProperties.repositories,
ivyProperties.ivyRepoPath, Option(ivyProperties.ivySettingsPath))
ivyProperties.ivyRepoPath, ivyProperties.ivySettingsPath)
val jars = {
val jarsProp = sys.props.get(config.JARS.key).orNull
if (!StringUtils.isBlank(resolvedMavenCoordinates)) {
Expand Down
40 changes: 20 additions & 20 deletions core/src/main/scala/org/apache/spark/util/DependencyUtils.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,11 +29,11 @@ import org.apache.spark.deploy.SparkSubmitUtils
import org.apache.spark.internal.Logging

case class IvyProperties(
packagesExclusions: String,
packages: String,
repositories: String,
ivyRepoPath: String,
ivySettingsPath: String)
packagesExclusions: Option[String],
packages: Option[String],
repositories: Option[String],
ivyRepoPath: Option[String],
ivySettingsPath: Option[String])

private[spark] object DependencyUtils extends Logging {

Expand All @@ -44,7 +44,7 @@ private[spark] object DependencyUtils extends Logging {
"spark.jars.repositories",
"spark.jars.ivy",
"spark.jars.ivySettings"
).map(sys.props.get(_).orNull)
).map(sys.props.get(_))
IvyProperties(packagesExclusions, packages, repositories, ivyRepoPath, ivySettingsPath)
}

Expand All @@ -69,10 +69,10 @@ private[spark] object DependencyUtils extends Logging {
* Example: Input: excludeorg.mortbay.jetty:jetty,org.eclipse.jetty:jetty-http
* Output: [org.mortbay.jetty:jetty,org.eclipse.jetty:jetty-http]
*/
private def parseQueryParams(uri: URI): (Boolean, String) = {
private def parseQueryParams(uri: URI): (Boolean, Option[String]) = {
val uriQuery = uri.getQuery
if (uriQuery == null) {
(false, "")
(false, None)
} else {
val mapTokens = uriQuery.split("&").map(_.split("="))
if (mapTokens.exists(isInvalidQueryString)) {
Expand Down Expand Up @@ -103,7 +103,7 @@ private[spark] object DependencyUtils extends Logging {
}
excludes
}.mkString(",")
}.getOrElse("")
}

val validParams = Set("transitive", "exclude")
val invalidParams = groupedParams.keys.filterNot(validParams.contains).toSeq
Expand Down Expand Up @@ -150,36 +150,36 @@ private[spark] object DependencyUtils extends Logging {
resolveMavenDependencies(
transitive,
exclusionList,
authority,
Some(authority),
ivyProperties.repositories,
ivyProperties.ivyRepoPath,
Option(ivyProperties.ivySettingsPath)
ivyProperties.ivySettingsPath
).split(",")
}

def resolveMavenDependencies(
packagesTransitive: Boolean,
packagesExclusions: String,
packages: String,
repositories: String,
ivyRepoPath: String,
packagesExclusions: Option[String],
packages: Option[String],
repositories: Option[String],
ivyRepoPath: Option[String],
ivySettingsPath: Option[String]): String = {
val exclusions: Seq[String] =
if (!StringUtils.isBlank(packagesExclusions)) {
packagesExclusions.split(",")
if (packagesExclusions.nonEmpty) {
packagesExclusions.map(_.split(",")).get
} else {
Nil
}
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.

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

Copy link
Contributor Author

@AngersZhuuuu AngersZhuuuu Jan 4, 2021

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)

DOne

// Create the IvySettings, either load from file or build defaults
val ivySettings = ivySettingsPath match {
case Some(path) =>
SparkSubmitUtils.loadIvySettings(path, Option(repositories), Option(ivyRepoPath))
SparkSubmitUtils.loadIvySettings(path, repositories, ivyRepoPath)

case None =>
SparkSubmitUtils.buildIvySettings(Option(repositories), Option(ivyRepoPath))
SparkSubmitUtils.buildIvySettings(repositories, ivyRepoPath)
}

SparkSubmitUtils.resolveMavenCoordinates(packages, ivySettings,
SparkSubmitUtils.resolveMavenCoordinates(packages.get, ivySettings,
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.

Should be packages.orNull ?

Copy link
Contributor Author

@AngersZhuuuu AngersZhuuuu Jan 4, 2021

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

transitive = packagesTransitive, exclusions = exclusions)
}

Expand Down