[SPARK-17568][CORE][DEPLOY] Add spark-submit option to override ivy settings used to resolve packages/artifacts #15119

BryanCutler · 2016-09-16T21:55:44Z

What changes were proposed in this pull request?

Adding option in spark-submit to allow overriding the default IvySettings used to resolve artifacts as part of the Spark Packages functionality. This will allow all artifact resolution to go through a central managed repository, such as Nexus or Artifactory, where site admins can better approve and control what is used with Spark apps.

This change restructures the creation of the IvySettings object in two distinct ways. First, if the spark.ivy.settings option is not defined then buildIvySettings will create a default settings instance, as before, with defined repositories (Maven Central) included. Second, if the option is defined, the ivy settings file will be loaded from the given path and only repositories defined within will be used for artifact resolution.

How was this patch tested?

Existing tests for default behaviour, Manual tests that load a ivysettings.xml file with local and Nexus repositories defined. Added new test to load a simple Ivy settings file with a local filesystem resolver.

…ings

…-settings

SparkQA · 2016-09-17T00:09:34Z

Test build #65506 has finished for PR 15119 at commit 1f472ff.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

themodernlife · 2016-11-08T15:32:11Z

FYI I tried this out in our environment

Firewall (no access to maven central)
Custom ivysettings.xml to point to our internal Artifactory

Everything worked just as I'd expected. Hope to see this merged!

BryanCutler · 2016-11-09T17:52:28Z

Hi @themodernlife, thanks for testing this out! Glad it worked for you, I'll try to ping some others and see if I can get more interest. ping @JoshRosen @brkyvz

vanzin · 2016-12-15T22:20:58Z

core/src/main/scala/org/apache/spark/deploy/SparkSubmit.scala

@@ -291,8 +292,12 @@ object SparkSubmit {
      } else {
        Nil
      }
+
+    val ivySettings = Option(args.ivySettingsFile).map(SparkSubmitUtils.loadIvySettings).getOrElse(


So basically it's either / or. Is there any way to reconcile the two? (e.g. load the file and add repositories explicitly defined in the command line). This would allow one to override the settings file in the default Spark conf and still be able to add extra repositories.

@vanzin I thought this would be useful too, but it turns out I haven't missed it at all using this patch for the last few weeks. It's no big deal to add any "one off" repositories to an xml file if you need that kind of customization.

If it's not much work I think it would be worth it to support the use case. If there's something that blocks that from working well, then it's ok to not support it.

(It also avoids confusion: "why can't I use both of these options together?")

I just figured any repositories would be added to the settings file, but I can see how having both options could be of some benefit so let me change that.

vanzin · 2016-12-15T22:21:57Z

core/src/main/scala/org/apache/spark/deploy/SparkSubmit.scala

@@ -445,6 +450,8 @@ object SparkSubmit {
        sysProp = "spark.submit.deployMode"),
      OptionAssigner(args.name, ALL_CLUSTER_MGRS, ALL_DEPLOY_MODES, sysProp = "spark.app.name"),
      OptionAssigner(args.ivyRepoPath, ALL_CLUSTER_MGRS, CLIENT, sysProp = "spark.jars.ivy"),
+      OptionAssigner(args.ivySettingsFile, ALL_CLUSTER_MGRS, ALL_DEPLOY_MODES,


Is this setting needed by the application? It seems it only really affects SparkSubmit itself.

I remember I was unsure of this line, but was trying to follow the ivyRepoPath setting as an example. So you're saying this options are only for the application and I can delete this?

Yes; OptionAssigner is for propagating the option to SparkConf, and I don't think that's needed for this option.

vanzin · 2016-12-15T22:24:55Z

core/src/test/scala/org/apache/spark/deploy/SparkSubmitUtilsSuite.scala

-      val jarPath = SparkSubmitUtils.resolveMavenCoordinates(main.toString, None, None,
+      val jarPath = SparkSubmitUtils.resolveMavenCoordinates(
+        main.toString,
+        SparkSubmitUtils.buildIvySettings(None, None),


It would be nice to add a test that creates a settings file and uses it, instead of calling this method on every test.

Yeah, there should be a test for loading the file, I'll add that.

vanzin · 2016-12-15T22:25:19Z

core/src/main/scala/org/apache/spark/deploy/SparkSubmitArguments.scala

@@ -175,6 +176,7 @@ private[deploy] class SparkSubmitArguments(args: Seq[String], env: Map[String, S
    jars = Option(jars).orElse(sparkProperties.get("spark.jars")).orNull
    files = Option(files).orElse(sparkProperties.get("spark.files")).orNull
    ivyRepoPath = sparkProperties.get("spark.jars.ivy").orNull
+    ivySettingsFile = sparkProperties.get("spark.ivy.settings").orNull


This probably deserves documentation in docs/configuration.md.

BryanCutler · 2016-12-16T23:32:42Z

Thanks for taking a look @vanzin, it'll be a few days before I can work on this but I'll ping you and @themodernlife when ready.

…tings-SPARK-17568

BryanCutler · 2017-01-03T22:21:42Z

Sorry for the delay @vanzin and @themodernlife , I was only able to start looking at this today. I have a couple questions for you guys

I think the docs might not be quite right about spark.jars.ivy. It's supposed to be a local path to Ivy and used to make the Ivy cache path and a path to download resolved jars, not for a remote repository, but that might work. For additional repositories, the user should specify with the command line option --repositories, which doesn't seem to have a conf property. Do you guys agree? If so I could fix up the docs here.
To make this PR work with additional repositories, it's a little more complicated due to some needed checks. I think the best strategy would be to create a new ChainResolver with a unique name (to not conflict with defined resolvers). Add the default resolver from the settings file to this (if exists), followed by the additional repositories, then make this the new default resolver. The last artifact resolved is the one used, so the additional repositories would have precedence over those in the settings file. In the same vein, if spark.jars.ivy is specified, it would override the Ivy user dir and cache path, if defined in the settings file. Sound ok?

vanzin · 2017-01-03T22:25:28Z

Not sure about (1), maybe @brkyvz knows since I think he wrote that code. (2) sounds ok to me.

themodernlife · 2017-01-05T14:14:48Z

@BryanCutler

Removed reference to incubation in README.md. #1 I think you're right... the naming of that key is unfortuante, spark.jars.ivyUserDir or something would have been better... it affects defaultIvyUserDir property of the Ivy settings
Removed reference to incubation in Spark user docs. #2 sounds right to me

…file too

modified documentation for option changes closes #12

BryanCutler · 2017-01-07T01:00:35Z

@vanzin and @themodernlife , I have this fixed up to use the additional repositories when loading ivy settings from a file. Added a new test for loading a settings and fixed up the docs for spark.jars.ivy - I agree that the name is confusing, but hopefully clear in the docs. Thanks @themodernlife for helping out with the docs too. If you're able to try out this latest revision too, that would be great!

SparkQA · 2017-01-07T03:14:53Z

Test build #71001 has finished for PR 15119 at commit 7a7b6ba.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

brkyvz · 2017-01-09T17:29:11Z

docs/configuration.md

@@ -450,8 +452,20 @@ Apart from these, the following properties are also available, and may be useful
  <td><code>spark.jars.ivy</code></td>
  <td></td>
  <td>
-    Comma-separated list of additional remote repositories to search for the coordinates given


Yes, this is wrong documentation. At the time of the first PR we didn't document this, because we were unsure how popular the feature would be and we didn't want to confuse anyone with too many configurations. I think the time has come to add all this new functionality :)

brkyvz · 2017-01-09T17:30:31Z

docs/configuration.md

+  </td>
+</tr>
+<tr>
+  <td><code>spark.ivy.settings</code></td>


all other confs are spark.jars.ivy. Should we be consistent here as well? spark.jars.ivySettings perhaps?

That sounds fine with me, it would at least make it more clear that they are related

vanzin

Look ok. Just a couple minor things (aside from Burak's comment).

vanzin · 2017-01-09T22:28:44Z

core/src/main/scala/org/apache/spark/deploy/SparkSubmit.scala

@@ -283,8 +284,17 @@ object SparkSubmit extends CommandLineUtils {
      } else {
        Nil
      }
+
+    // Create the IvySettings, either load from file or build defaults
+    val ivySettings = if (Option(args.ivySettingsFile).isDefined) {


Looks like a really long way of saying args.ivySettingsFile != null.

true, that is kind of dumb I guess.. I just realized that since this is just a conf anyway and not a command-line arg, I don't need args.ivySettingsFile and I could just do

val ivySettings = args.sparkProperties.get("spark.jars.ivySettings").map { ivySettingsFile => ...

Look ok to you?

Sounds good.

vanzin · 2017-01-09T22:31:55Z

core/src/main/scala/org/apache/spark/deploy/SparkSubmit.scala

+        brr.setName(s"repo-${i + 1}")
+        cr.add(brr)
+        // scalastyle:off println
+        printStream.println(s"$repo added as a remote repository with the name: ${brr.getName}")


Is this really needed?

That's used in testing IIRC and is helpful in debugging

It was in there before and I think it's nice to get confirmation that your additional was added with that alias, in case the artifact you need isn't resolved. It will print out all the info like this

bin/run-example --repositories www.somerepo.com --packages some:package:1.0 SparkPi Listening for transport dt_socket at address: 5005 www.somerepo.com added as a remote repository with the name: repo-1 Ivy Default Cache set to: /home/bryan/.ivy2/cache The jars for the packages stored in: /home/bryan/.ivy2/jars :: loading settings :: url = jar:file:/home/bryan/git/spark/assembly/target/scala-2.11/jars/ivy-2.4.0.jar!/org/apache/ivy/core/settings/ivysettings.xml some#package added as a dependency :: resolving dependencies :: org.apache.spark#spark-submit-parent;1.0 confs: [default] :: resolution report :: resolve 1540ms :: artifacts dl 0ms :: modules in use: --------------------------------------------------------------------- | | modules || artifacts | | conf | number| search|dwnlded|evicted|| number|dwnlded| --------------------------------------------------------------------- | default | 1 | 0 | 0 | 0 || 0 | 0 | --------------------------------------------------------------------- :: problems summary :: :::: WARNINGS :::::::::::::::::::::::::::::::::::::::::::::: :: UNRESOLVED DEPENDENCIES :: :::::::::::::::::::::::::::::::::::::::::::::: :: some#package;1.0: repo-1: unable to get resource for some#package;1.0: res=www.somerepo.com/some/package/1.0/package-1.0.pom: java.net.MalformedURLException: no protocol: www.somerepo.com/some/package/1.0/package-1.0.pom ::::::::::::::::::::::::::::::::::::::::::::::

Yeah, I saw later it was already there. I just generally dislike non-error log messages that cannot be turned off.

SparkQA · 2017-01-10T02:24:06Z

Test build #71100 has finished for PR 15119 at commit 9b6d67f.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

BryanCutler · 2017-01-11T00:24:36Z

Hi @vanzin, any more comments or think this could be merged?

vanzin

Two minor nits. If no one adds more comments, I'll merge once you address them.

vanzin · 2017-01-11T00:35:41Z

docs/configuration.md

+    classpaths. If <code>spark.jars.ivySettings</code> is given artifacts will be resolved according
+    to the configuration in the file, otherwise artifacts will be searched for in the local maven repo,
+    then maven central and finally any additional remote repositories given by the command-line option
+    <code>--repositories</code> see <a href="submitting-applications.html#advanced-dependency-management">Advanced Dependency Management</a>.


You seem to be missing punctuation before "see".

vanzin · 2017-01-11T00:38:19Z

core/src/test/scala/org/apache/spark/deploy/SparkSubmitUtilsSuite.scala

+         |""".stripMargin
+
+    val settingsFile = new File(tempIvyPath, "ivysettings.xml")
+    val settingsWriter = new FileWriter(settingsFile)


Since I added another comment... minor, but guava's Files.write would be cleaner here (and it also forces you to specify the encoding, which you should be doing here anyway).

…oding

BryanCutler · 2017-01-11T01:18:33Z

Thanks @vanzin , good points. I took another read through the doc and thought it was better to move things around a little. I also noticed a misplaced tag in another setting. Let me know if you see any other issues.

SparkQA · 2017-01-11T04:02:26Z

Test build #71173 has finished for PR 15119 at commit 532e744.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

vanzin · 2017-01-11T19:57:03Z

LGTM. Merging to master.

…ettings used to resolve packages/artifacts ## What changes were proposed in this pull request? Adding option in spark-submit to allow overriding the default IvySettings used to resolve artifacts as part of the Spark Packages functionality. This will allow all artifact resolution to go through a central managed repository, such as Nexus or Artifactory, where site admins can better approve and control what is used with Spark apps. This change restructures the creation of the IvySettings object in two distinct ways. First, if the `spark.ivy.settings` option is not defined then `buildIvySettings` will create a default settings instance, as before, with defined repositories (Maven Central) included. Second, if the option is defined, the ivy settings file will be loaded from the given path and only repositories defined within will be used for artifact resolution. ## How was this patch tested? Existing tests for default behaviour, Manual tests that load a ivysettings.xml file with local and Nexus repositories defined. Added new test to load a simple Ivy settings file with a local filesystem resolver. Author: Bryan Cutler <cutlerb@gmail.com> Author: Ian Hummel <ian@themodernlife.net> Closes apache#15119 from BryanCutler/spark-custom-IvySettings.

BryanCutler added 3 commits May 11, 2016 16:51

added option to load IvySettings from file instead of hard-coded sett…

cc1ad11

…ings

added parenthesis to be consistent

97ea3a4

Merge remote-tracking branch 'upstream/master' into wip-spark-package…

1f472ff

…-settings

vanzin reviewed Dec 15, 2016

View reviewed changes

This was referenced Dec 22, 2016

Add docs aboout ivy BryanCutler/spark#11

Closed

Adjusts documentation for Ivy-related settings. BryanCutler/spark#12

Closed

BryanCutler added 3 commits January 3, 2017 09:33

Merge remote-tracking branch 'upstream/master' into spark-conf-IvySet…

46ac2ef

…tings-SPARK-17568

removed OptionAssigner, not needed

716454b

cleaned up alternateIvyCache setting

a25170a

BryanCutler and others added 4 commits January 6, 2017 11:31

added option for additional repos and ivy path when loading settings …

28f504a

…file too

added test for loading ivy settings from file

498a34f

Adjusts documentation for Ivy-related settings.

a9da2b4

modified documentation for option changes closes #12

fixed up package related docs

7a7b6ba

brkyvz reviewed Jan 9, 2017

View reviewed changes

vanzin reviewed Jan 9, 2017

View reviewed changes

BryanCutler added 2 commits January 9, 2017 14:50

changed conf to spark.jars.ivySettings

c855a17

removed ivySettings from SparkSubmitArguments

9b6d67f

vanzin approved these changes Jan 11, 2017

View reviewed changes

BryanCutler added 2 commits January 10, 2017 17:06

fixed doc punctuation and non-related tag error

ff34f99

used Files.write for writing test suite file, cleaner and specify enc…

532e744

…oding

asfgit closed this in 3bc2eff Jan 11, 2017

themodernlife mentioned this pull request Jan 12, 2017

[SPARK-17568][CORE][DEPLOY] Add spark-submit option to override ivy settings used to resolve packages/artifacts #16563

Closed

BryanCutler deleted the spark-custom-IvySettings branch March 7, 2017 20:01

[SPARK-17568][CORE][DEPLOY] Add spark-submit option to override ivy settings used to resolve packages/artifacts #15119

[SPARK-17568][CORE][DEPLOY] Add spark-submit option to override ivy settings used to resolve packages/artifacts #15119

Conversation

BryanCutler commented Sep 16, 2016 • edited

What changes were proposed in this pull request?

How was this patch tested?

SparkQA commented Sep 17, 2016

themodernlife commented Nov 8, 2016

BryanCutler commented Nov 9, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vanzin Dec 16, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BryanCutler commented Dec 16, 2016

BryanCutler commented Jan 3, 2017

vanzin commented Jan 3, 2017

themodernlife commented Jan 5, 2017

BryanCutler commented Jan 7, 2017

SparkQA commented Jan 7, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vanzin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SparkQA commented Jan 10, 2017

BryanCutler commented Jan 11, 2017

vanzin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BryanCutler commented Jan 11, 2017

SparkQA commented Jan 11, 2017

vanzin commented Jan 11, 2017

BryanCutler commented Sep 16, 2016 •

edited

vanzin Dec 16, 2016 •

edited