dongjoon-hyun commented on code in PR #59587:
URL: https://github.com/apache/spark/pull/59587#discussion_r4317078369
##########
sql/hive/src/test/scala/org/apache/spark/sql/hive/execution/HiveDDLSuite.scala:
##########
@@ -3251,8 +3251,9 @@ class HiveDDLSuite
val udtfJar = TestUDTFJar.jar
val jarName = udtfJar.getName
val jar = udtfJar.toURI.toString
+ // listJars() flattens every artifact bucket, so clear the jar from all
of them.
spark.sparkContext.allAddedJars.keys.find(_.contains(jarName))
Review Comment:
`find` picks only the first matching key. If the same jar was registered
under two different keys (for example, `local:/.../TestUDTFxxx.jar` gives a
`file:/...` key, while `ADD JAR <path>` gives a
`spark://host:port/jars/TestUDTFxxx.jar` key), only one of them is removed. The
next `assert(!listJars().exists(_.contains(jarName)))` would then still fail in
an order-dependent way. Removing every matching key per bucket covers this:
```scala
spark.sparkContext.addedJars.values.foreach { jars =>
jars.keys.filter(_.contains(jarName)).foreach(jars.remove)
}
```
##########
sql/hive/src/test/scala/org/apache/spark/sql/hive/execution/HiveDDLSuite.scala:
##########
@@ -3251,8 +3251,9 @@ class HiveDDLSuite
val udtfJar = TestUDTFJar.jar
val jarName = udtfJar.getName
val jar = udtfJar.toURI.toString
+ // listJars() flattens every artifact bucket, so clear the jar from all
of them.
spark.sparkContext.allAddedJars.keys.find(_.contains(jarName))
- .foreach(k =>
spark.sparkContext.addedJars.get("default").foreach(_.remove(k)))
+ .foreach(k => spark.sparkContext.addedJars.values.foreach(_.remove(k)))
Review Comment:
This now removes the key from every bucket, including the buckets of other
sessions that may still be alive (for example, a `newSession()` or a cloned
session created by an earlier test while isolation is enabled). The final
`CREATE OR REPLACE` only re-adds the jar to the current session's bucket, so
those other sessions stop shipping the jar to executors (`JobArtifactSet` reads
`addedJars(uuid)`). The file server entry is also left in place, so `addedJars`
and the file server drift apart. The previous code had the same issue, but only
for `"default"`. Could we limit the removal to the buckets this test can
actually affect (`"default"` and `spark`'s own session UUID)?
##########
sql/hive/src/test/scala/org/apache/spark/sql/hive/execution/HiveDDLSuite.scala:
##########
@@ -3251,8 +3251,9 @@ class HiveDDLSuite
val udtfJar = TestUDTFJar.jar
Review Comment:
As an alternative to editing `private[spark]` SparkContext state: copy
`TestUDTFJar.jar` to a fresh temp file with a name unique to this test. Then
`listJars()` cannot contain it beforehand, and the first assertion holds
without any cleanup. This test has already needed a fix twice when jar tracking
changed (SPARK-46237 and this PR), and a unique jar would make it independent
of how `addedJars` is keyed.
##########
sql/hive/src/test/scala/org/apache/spark/sql/hive/execution/HiveDDLSuite.scala:
##########
@@ -3251,8 +3251,9 @@ class HiveDDLSuite
val udtfJar = TestUDTFJar.jar
val jarName = udtfJar.getName
val jar = udtfJar.toURI.toString
+ // listJars() flattens every artifact bucket, so clear the jar from all
of them.
Review Comment:
Nothing in this test puts the jar into a non-default bucket, so the new path
only runs when an earlier test happens to leave the jar behind. As the PR
description says, the standalone run removes nothing. If this cleanup regressed
(for example, back to `get("default")`), the test would still pass on its own.
Could we add `sql(s"ADD JAR $jar")` before the cleanup? That always puts the
jar into the session bucket, so the test checks the fix deterministically.
##########
sql/hive/src/test/scala/org/apache/spark/sql/hive/execution/HiveDDLSuite.scala:
##########
@@ -3251,8 +3251,9 @@ class HiveDDLSuite
val udtfJar = TestUDTFJar.jar
val jarName = udtfJar.getName
val jar = udtfJar.toURI.toString
+ // listJars() flattens every artifact bucket, so clear the jar from all
of them.
spark.sparkContext.allAddedJars.keys.find(_.contains(jarName))
Review Comment:
nit: `allAddedJars` builds a flattened `Map` just to find a key, and then
the code walks the same buckets again to remove it. A single pass over
`addedJars.values` (as in the snippet in the other comment) does both and is
easier to read.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
