xkrogen commented on a change in pull request #31591:
URL: https://github.com/apache/spark/pull/31591#discussion_r583760800
##########
File path:
resource-managers/yarn/src/main/scala/org/apache/spark/deploy/yarn/Client.scala
##########
@@ -793,6 +793,25 @@ private[spark] class Client(
// distributed file.
amKeytabFileName.foreach { kt => props.setProperty(KEYTAB.key, kt) }
+ // Upload user provided ivysettings.xml file to the distributed cache
+ val ivySettings = sparkConf.getOption("spark.jars.ivySettings")
+ if (isClusterMode && ivySettings.isDefined) {
+ val ivySettingsFile = new File(ivySettings.get)
+ require(ivySettingsFile.exists(), s"Ivy settings file $ivySettingsFile
not found")
Review comment:
This would be a little more Scala-idiomatic for unpacking the `Option`:
```
ivySettings match {
case Some(ivySettingsPath) if isClusterMode =>
// ... logic here
case _ => // do nothing
}
```
##########
File path:
resource-managers/yarn/src/main/scala/org/apache/spark/deploy/yarn/Client.scala
##########
@@ -793,6 +793,25 @@ private[spark] class Client(
// distributed file.
amKeytabFileName.foreach { kt => props.setProperty(KEYTAB.key, kt) }
+ // Upload user provided ivysettings.xml file to the distributed cache
+ val ivySettings = sparkConf.getOption("spark.jars.ivySettings")
+ if (isClusterMode && ivySettings.isDefined) {
+ val ivySettingsFile = new File(ivySettings.get)
+ require(ivySettingsFile.exists(), s"Ivy settings file $ivySettingsFile
not found")
+ require(ivySettingsFile.isFile(),
+ s"Ivy settings file $ivySettingsFile is not a normal file")
+ // Generate a file name that can be used for the ivySettings file,
that does not conflict
+ // with any other conf file.
+ val amIvySettingsFileName = ivySettingsFile.getName() + "-" +
UUID.randomUUID().toString
+ confStream.putNextEntry(new ZipEntry(amIvySettingsFileName))
+ Files.copy(ivySettingsFile, confStream)
Review comment:
Can we use `java.nio.file.Files` here instead of Guava's `Files`? I know
Guava is already used in this file but I think it's better to leverage the
built-in functionality moving forward. You can do a rename import like `import
java.nio.file.{Files => NioFiles}` (or update the other references as well if
it's a small change?)
##########
File path:
resource-managers/yarn/src/test/scala/org/apache/spark/deploy/yarn/YarnClusterSuite.scala
##########
@@ -583,6 +597,44 @@ private object YarnClasspathTest extends Logging {
}
+private object YarnAddJarTest extends Logging {
+ def main(args: Array[String]): Unit = {
+ if (args.length != 1) {
+ // scalastyle:off println
+ System.err.println(
+ s"""
+ |Invalid command line: ${args.mkString(" ")}
+ |
+ |Usage: YarnAddJarTest [result file]
+ """.stripMargin)
+ // scalastyle:on println
+ System.exit(1)
+ }
+
+ val resultPath = args(0)
+ val sc = new SparkContext(new SparkConf())
+
+ var result = "failure"
+ try {
+ val settingsFile = sc.getConf.get("spark.jars.ivySettings")
+ // Make sure that ivySettings conf was set to the localized file
+ assert(settingsFile.startsWith(Client.LOCALIZED_CONF_DIR))
+
+ val caught = intercept[RuntimeException] {
+ sc.addJar("ivy://org.fake-project.test:test:1.0.0")
+ }
+ if (caught.getMessage.contains("unresolved dependency:
org.fake-project.test#test")) {
+ // "unresolved dependency" is expected as the dependency does not exist
+ // but exception like "Ivy settings file <file> does not exist should
result in failure"
Review comment:
Minor nit: end quote is in the wrong place
##########
File path:
resource-managers/yarn/src/test/scala/org/apache/spark/deploy/yarn/YarnClusterSuite.scala
##########
@@ -368,6 +369,19 @@ class YarnClusterSuite extends BaseYarnClusterSuite {
)
checkResult(finalState, result, "true")
}
+
+ test("SPARK-34472: ivySettings file should be localized on driver in cluster
mode") {
+
+ val emptyIvySettings = File.createTempFile("ivy", ".xml")
+ FileUtils.write(emptyIvySettings, "<ivysettings />",
StandardCharsets.UTF_8)
+
Review comment:
Can we use NIO for these? No need for commons-io now that NIO supports
this kind of stuff built-in.
```
val emptyIvySettings = Files.createTempFile(...)
Files.write(emptyIvySettings, Seq(...))
```
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
