[GitHub] [spark] xkrogen commented on a change in pull request #31591: [SPARK-34472][YARN] Ship ivySettings file to driver in cluster mode

[GitBox <gi...@apache.org>]

xkrogen commented on a change in pull request #31591:
URL: https://github.com/apache/spark/pull/31591#discussion_r613342371



##########
File path: resource-managers/yarn/src/test/scala/org/apache/spark/deploy/yarn/YarnClusterSuite.scala
##########
@@ -583,6 +602,44 @@ private object YarnClasspathTest extends Logging {
 
 }
 
+private object YarnAddJarTest extends Logging {
+  def main(args: Array[String]): Unit = {
+    if (args.length != 2) {
+      // scalastyle:off println
+      System.err.println(
+        s"""
+           |Invalid command line: ${args.mkString(" ")}
+           |
+           |Usage: YarnAddJarTest [result file] [expected ivy settings prefix]
+        """.stripMargin)
+      // scalastyle:on println
+      System.exit(1)
+    }
+
+    val resultPath = args(0)
+    val expectedIvySettingsPrefix = args(1)
+    val sc = new SparkContext(new SparkConf())
+
+    var result = "failure"
+    try {
+      val settingsFile = sc.getConf.get("spark.jars.ivySettings")
+      assert(settingsFile.startsWith(expectedIvySettingsPrefix))

Review comment:
       Should we also `assert(settingsFile !== expectedIvySettingsPrefix)` ?

##########
File path: resource-managers/yarn/src/test/scala/org/apache/spark/deploy/yarn/YarnClusterSuite.scala
##########
@@ -368,6 +368,25 @@ class YarnClusterSuite extends BaseYarnClusterSuite {
     )
     checkResult(finalState, result, "true")
   }
+
+  def createEmptyIvySettingsFile: File = {
+    val emptyIvySettings = File.createTempFile("ivy", ".xml")
+    Files.write("<ivysettings />", emptyIvySettings, StandardCharsets.UTF_8)
+    emptyIvySettings
+  }
+
+  test("SPARK-34472: non-local ivySettings file should be localized on driver in cluster mode") {

Review comment:
       Can we also test `clientMode = true` and assert that the file name hasn't been modified?
   
   Could also be good to add tests for explicitly using `file:///` URI and for an expected failure when using `local:///` URI?

##########
File path: resource-managers/yarn/src/main/scala/org/apache/spark/deploy/yarn/Client.scala
##########
@@ -518,6 +518,32 @@ private[spark] class Client(
       require(localizedPath != null, "Keytab file already distributed.")
     }
 
+    // If we passed in a ivySettings file, make sure we copy the file to the distributed cache
+    // in cluster mode so that the driver can access it
+    val ivySettings = sparkConf.getOption("spark.jars.ivySettings")
+    val ivySettingsLocalizedPath: Option[String] = ivySettings match {
+      case Some(ivySettingsPath) if isClusterMode =>
+        val uri = new URI(ivySettingsPath)
+        Option(uri.getScheme).getOrElse("file") match {
+          case "file" =>
+            val ivySettingsFile = new File(ivySettingsPath)
+            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

Review comment:
        This block of code (and the IllegalArgumentException below) -- convert to URI, check scheme, throw if bad scheme, assert on file existence -- is duplicated between `SparkSubmit` and `Client`. Can we put a method like `getIvySettingsFile` into `SparkSubmit` which is leveraged in both places?
    
    Not too big of a duplication so if it can't be done cleanly no worries from my end.




-- 
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:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org