Skip to content
Closed
Show file tree
Hide file tree
Changes from 4 commits
Commits
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
28 changes: 14 additions & 14 deletions core/src/main/scala/org/apache/spark/deploy/SparkSubmit.scala
Original file line number Diff line number Diff line change
Expand Up @@ -727,7 +727,7 @@ private[spark] class SparkSubmit extends Logging {
if (opt.value != null &&
(deployMode & opt.deployMode) != 0 &&
(clusterManager & opt.clusterManager) != 0) {
if (opt.clOption != null) { childArgs += (opt.clOption, opt.value) }
if (opt.clOption != null) { childArgs += opt.clOption += opt.value }
if (opt.confKey != null) {
if (opt.mergeFn.isDefined && sparkConf.contains(opt.confKey)) {
sparkConf.set(opt.confKey, opt.mergeFn.get.apply(sparkConf.get(opt.confKey), opt.value))
Expand Down Expand Up @@ -759,15 +759,15 @@ private[spark] class SparkSubmit extends Logging {
if (args.isStandaloneCluster) {
if (args.useRest) {
childMainClass = REST_CLUSTER_SUBMIT_CLASS
childArgs += (args.primaryResource, args.mainClass)
childArgs += args.primaryResource += args.mainClass
} else {
// In legacy standalone cluster mode, use Client as a wrapper around the user class
childMainClass = STANDALONE_CLUSTER_SUBMIT_CLASS
if (args.supervise) { childArgs += "--supervise" }
Option(args.driverMemory).foreach { m => childArgs += ("--memory", m) }
Option(args.driverCores).foreach { c => childArgs += ("--cores", c) }
Option(args.driverMemory).foreach { m => childArgs += "--memory" += m }
Option(args.driverCores).foreach { c => childArgs += "--cores" += c }
childArgs += "launch"
childArgs += (args.master, args.primaryResource, args.mainClass)
childArgs += args.master += args.primaryResource += args.mainClass
}
if (args.childArgs != null) {
childArgs ++= args.childArgs
Expand All @@ -789,20 +789,20 @@ private[spark] class SparkSubmit extends Logging {
if (isYarnCluster) {
childMainClass = YARN_CLUSTER_SUBMIT_CLASS
if (args.isPython) {
childArgs += ("--primary-py-file", args.primaryResource)
childArgs += ("--class", "org.apache.spark.deploy.PythonRunner")
childArgs += "--primary-py-file" += args.primaryResource
childArgs += "--class" += "org.apache.spark.deploy.PythonRunner"
} else if (args.isR) {
val mainFile = new Path(args.primaryResource).getName
childArgs += ("--primary-r-file", mainFile)
childArgs += ("--class", "org.apache.spark.deploy.RRunner")
childArgs += "--primary-r-file" += mainFile
childArgs += "--class" += "org.apache.spark.deploy.RRunner"
} else {
if (args.primaryResource != SparkLauncher.NO_RESOURCE) {
childArgs += ("--jar", args.primaryResource)
childArgs += "--jar" += args.primaryResource
}
childArgs += ("--class", args.mainClass)
childArgs += "--class" += args.mainClass
}
if (args.childArgs != null) {
args.childArgs.foreach { arg => childArgs += ("--arg", arg) }
args.childArgs.foreach { arg => childArgs += "--arg" += arg }
}
}

Expand All @@ -825,12 +825,12 @@ private[spark] class SparkSubmit extends Logging {
}
if (args.childArgs != null) {
args.childArgs.foreach { arg =>
childArgs += ("--arg", arg)
childArgs += "--arg" += arg
}
}
// Pass the proxyUser to the k8s app so it is possible to add it to the driver args
if (args.proxyUser != null) {
childArgs += ("--proxy-user", args.proxyUser)
childArgs += "--proxy-user" += args.proxyUser
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -81,14 +81,15 @@ object CommandUtils extends Logging {

var newEnvironment = if (libraryPathEntries.nonEmpty && libraryPathName.nonEmpty) {
val libraryPaths = libraryPathEntries ++ cmdLibraryPath ++ env.get(libraryPathName)
command.environment + ((libraryPathName, libraryPaths.mkString(File.pathSeparator)))
command.environment ++ Map(libraryPathName -> libraryPaths.mkString(File.pathSeparator))
} else {
command.environment
}

// set auth secret to env variable if needed
if (securityMgr.isAuthenticationEnabled()) {
newEnvironment += (SecurityManager.ENV_AUTH_SECRET -> securityMgr.getSecretKey())
newEnvironment = newEnvironment ++
Map(SecurityManager.ENV_AUTH_SECRET -> securityMgr.getSecretKey())
}
// set SSL env variables if needed
newEnvironment ++= securityMgr.getEnvironmentForSslRpcPasswords
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -626,7 +626,7 @@ class JsonProtocolSuite extends SparkFunSuite {
val expectedEvent: SparkListenerEnvironmentUpdate = {
val e = JsonProtocol.environmentUpdateFromJson(environmentUpdateJsonString)
e.copy(environmentDetails =
e.environmentDetails + ("Metrics Properties" -> Seq.empty[(String, String)]))
e.environmentDetails ++ Map("Metrics Properties" -> Seq.empty[(String, String)]))
}
val oldEnvironmentUpdateJson = environmentUpdateJsonString
.removeField("Metrics Properties")
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -118,18 +118,18 @@ class TimeStampedHashMapSuite extends SparkFunSuite {
assert(testMap2.iterator.toSeq.head === (("k1", "v1")))

// +
val testMap3 = testMap2 + (("k0", "v0"))
val testMap3 = testMap2 ++ Map("k0" -> "v0")
assert(testMap3.size === 2)
assert(testMap3.get("k1").isDefined)
assert(testMap3("k1") === "v1")
assert(testMap3.get("k0").isDefined)
assert(testMap3("k0") === "v0")

// -
val testMap4 = testMap3 - "k0"
assert(testMap4.size === 1)
assert(testMap4.get("k1").isDefined)
assert(testMap4("k1") === "v1")
testMap3.remove("k0")
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure if this modification aligns with the original test objective.

Copy link
Contributor

Choose a reason for hiding this comment

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

I found that TimeStampedHashMap is now a utility class that is no longer used by Spark, and only test cases are still using it.

I personally have the following optional suggestions:

  1. Since it is private[spark] visibility, perhaps we can directly delete TimeStampedHashMap.
  2. Or we can mark it as deprecated in Spark 4.0, suppress the compilation warnings with @nowarn, and then remove it in Spark 4.1.
  3. Refactor TimeStampedHashMap into an immutable implementation to maintain the current usage.

cc @dongjoon-hyun FYI

Copy link
Contributor

@LuciferYang LuciferYang Nov 1, 2023

Choose a reason for hiding this comment

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

b18d708#diff-77b12178a7036c71135074c6ddf7d659e5a69906264d5e3061087e4352e304ed introduced this data structure

After #22339, this data structure is only being used in unit tests. So, after Spark 3.0, this data structure has not been used by any production code of Spark.

Copy link
Member

Choose a reason for hiding this comment

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

+1 for (1) direct deletion.

assert(testMap3.size === 1)
assert(testMap3.get("k1").isDefined)
assert(testMap3("k1") === "v1")
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ private[spark] class YarnClientSchedulerBackend(
sc.ui.foreach { ui => conf.set(DRIVER_APP_UI_ADDRESS, ui.webUrl) }

val argsArrayBuf = new ArrayBuffer[String]()
argsArrayBuf += ("--arg", hostport)
argsArrayBuf += "--arg" += hostport

logDebug("ClientArguments called with: " + argsArrayBuf.mkString(" "))
val args = new ClientArguments(argsArrayBuf.toArray)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -206,13 +206,12 @@ case class JoinEstimation(join: Join) extends Logging {
case _ =>
computeByNdv(leftKey, rightKey, newMin, newMax)
}
keyStatsAfterJoin += (
// Histograms are propagated as unchanged. During future estimation, they should be
// truncated by the updated max/min. In this way, only pointers of the histograms are
// propagated and thus reduce memory consumption.
leftKey -> joinStat.copy(histogram = leftKeyStat.histogram),
rightKey -> joinStat.copy(histogram = rightKeyStat.histogram)
)
// Histograms are propagated as unchanged. During future estimation, they should be
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it possible not to modify keyStatsAfterJoin += and the relative position of the comments?

// truncated by the updated max/min. In this way, only pointers of the histograms are
// propagated and thus reduce memory consumption.
keyStatsAfterJoin +=
(leftKey -> joinStat.copy(histogram = leftKeyStat.histogram)) +=
(rightKey -> joinStat.copy(histogram = rightKeyStat.histogram))
// Return cardinality estimated from the most selective join keys.
if (card < joinCard) joinCard = card
} else {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,8 @@ case class DescribeNamespaceExec(
}

if (isExtended) {
val properties = metadata.asScala -- CatalogV2Util.NAMESPACE_RESERVED_PROPERTIES
val properties = metadata.asScala.filterNot(
Copy link
Contributor

Choose a reason for hiding this comment

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

how about metadata.asScala.toMap --, then the subsequent properties don't need to call toMap.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good suggestion!

m => CatalogV2Util.NAMESPACE_RESERVED_PROPERTIES.toSet.contains(m._1))
val propertiesStr =
if (properties.isEmpty) {
""
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -825,9 +825,8 @@ class V2SessionCatalogNamespaceSuite extends V2SessionCatalogBaseSuite {
expected: scala.collection.Map[String, String],
actual: scala.collection.Map[String, String]): Unit = {
// remove location and comment that are automatically added by HMS unless they are expected
val toRemove =
CatalogV2Util.NAMESPACE_RESERVED_PROPERTIES.filter(expected.contains)
assert(expected -- toRemove === actual)
val toRemove = CatalogV2Util.NAMESPACE_RESERVED_PROPERTIES.toSet
Copy link
Contributor

Choose a reason for hiding this comment

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

how about change to use immutable.Map

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay.

assert(expected.filterNot(e => toRemove.contains(e._1)) === actual)
}

test("listNamespaces: basic behavior") {
Expand Down