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

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

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



##########
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))

Review comment:
       any reason not to just use distribute() rather then putting into the conf files?

##########
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")

Review comment:
       it would be nice to move this into the configuration package.scala and use ConfigBuilder.  Even if we just reference it by the .key option in the SparkSubmitArguments file.

##########
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:
       also check to see if Utils.isLocalUri




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