You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by vanzin <gi...@git.apache.org> on 2014/04/25 23:55:22 UTC
[GitHub] spark pull request: [SPARK-1395] Fix "local:" URI support in Yarn ...
GitHub user vanzin opened a pull request:
https://github.com/apache/spark/pull/560
[SPARK-1395] Fix "local:" URI support in Yarn mode (again).
Just update some calls to call methods that properly
handle "local:" URIs. Aside from those, two changes:
- the SPARK_JAR env variable needs to be propagated to
remote processes when the spark jar is "local:",
otherwise those processes won't be able to find it
in their PWD (since the file won't be distributed)
- I removed the hacky way that log4j configuration was
being propagated to handle the "local:" case. It's
much more cleanly (and generically) handled by using
spark-submit arguments (--files to upload a config
file, or setting spark.executor.extraJavaOptions to
pass JVM arguments and use a local file).
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/vanzin/spark yarn-local-2
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/spark/pull/560.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #560
----
commit ffa1b55ddf5164a752b88635ad5095c53874284f
Author: Marcelo Vanzin <va...@cloudera.com>
Date: 2014-04-25T18:07:28Z
[SPARK-1395] Fix "local:" URI support in Yarn mode (again).
Just update some calls to call methods that properly
handle "local:" URIs. Aside from those, two changes:
- the SPARK_JAR env variable needs to be propagated to
remote processes when the spark jar is "local:",
otherwise those processes won't be able to find it
in their PWD (since the file won't be distributed)
- I removed the hacky way that log4j configuration was
being propagated to handle the "local:" case. It's
much more cleanly (and generically) handled by using
spark-submit arguments (--files to upload a config
file, or setting spark.executor.extraJavaOptions to
pass JVM arguments and use a local file).
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SPARK-1395] Fix "local:" URI support in Yarn ...
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on the pull request:
https://github.com/apache/spark/pull/560#issuecomment-45149619
Hi Tom,
I just removed all the log4j-related code. You can achieve both previously available use cases with spark-submit arguments:
* To use a custom config file, just say "--files /path/to/log4j.properties", and that gets distributed to the worker nodes and is available at the root of the classpath, so it overrides the default log4j-spark-container.properties.
* To use a local config file, just say "spark.executor.extraJavaOpts=-Dlog4j.configuration=file:/path/to/log4j.properties"
Note that since local: is broken in 1.0.0, the second one above cannot be achieved by using SPARK_LOG4J_CONF in that release.
As for backwards compatibility, SPARK_LOG4J_CONF doesn't seem to be documented anywhere, and the tricks above work fine with 1.0.0. I guess I could add the content of SPARK_LOG4J_CONF to the list of copied files if you think that's important.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SPARK-1395] Fix "local:" URI support in Yarn ...
Posted by tgravescs <gi...@git.apache.org>.
Github user tgravescs commented on a diff in the pull request:
https://github.com/apache/spark/pull/560#discussion_r13406732
--- Diff: yarn/common/src/main/scala/org/apache/spark/deploy/yarn/ClientBase.scala ---
@@ -378,15 +378,34 @@ trait ClientBase extends Logging {
}
}
-object ClientBase {
+object ClientBase extends Logging {
val SPARK_JAR: String = "__spark__.jar"
val APP_JAR: String = "__app__.jar"
- val LOG4J_PROP: String = "log4j.properties"
- val LOG4J_CONF_ENV_KEY: String = "SPARK_LOG4J_CONF"
val LOCAL_SCHEME = "local"
+ val CONF_SPARK_JAR = "spark.yarn.jar"
--- End diff --
can you document the config
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SPARK-1395] Fix "local:" URI support in Yarn ...
Posted by cmccabe <gi...@git.apache.org>.
Github user cmccabe commented on the pull request:
https://github.com/apache/spark/pull/560#issuecomment-45130395
I was just talking about the log message in yarn/stable/src/main/scala/org/apache/spark/deploy/yarn/ExecutorRunnable.scala. The rest is shared, if I understand correctly.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SPARK-1395] Fix "local:" URI support in Yarn ...
Posted by tgravescs <gi...@git.apache.org>.
Github user tgravescs commented on the pull request:
https://github.com/apache/spark/pull/560#issuecomment-45252153
Unfortunately I guess our documentation has very little on logging, but that doesn't mean that isn't how it worked. That is the way it worked in 0.9 and still worked in 1.0 so I don't want to break that method so would like it added back. Hopefully its not to painful to just add to the list of files to copy as you mention.
I would also like to see the documentation updated to make it clear to users how they do it. If you would like to do it here that would be fine or we can file a separate jira also.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SPARK-1395] Fix "local:" URI support in Yarn ...
Posted by tgravescs <gi...@git.apache.org>.
Github user tgravescs commented on a diff in the pull request:
https://github.com/apache/spark/pull/560#discussion_r13861962
--- Diff: yarn/common/src/main/scala/org/apache/spark/deploy/yarn/ClientBase.scala ---
@@ -464,71 +485,74 @@ object ClientBase extends Logging {
triedDefault.toOption
}
+ def populateClasspath(args: ClientArguments, conf: Configuration, sparkConf: SparkConf,
+ env: HashMap[String, String], extraClassPath: Option[String] = None) {
+ extraClassPath.foreach(addClasspathEntry(_, env))
+ addClasspathEntry(Environment.PWD.$(), env)
+
+ // Normally the users app.jar is last in case conflicts with spark jars
+ if (sparkConf.get("spark.yarn.user.classpath.first", "false").toBoolean) {
+ addUserClasspath(args, sparkConf, env)
+ addFileToClasspath(sparkJar(sparkConf), SPARK_JAR, env)
+ ClientBase.populateHadoopClasspath(conf, env)
+ } else {
+ addFileToClasspath(sparkJar(sparkConf), SPARK_JAR, env)
+ ClientBase.populateHadoopClasspath(conf, env)
+ addUserClasspath(args, sparkConf, env)
+ }
+
+ // Append all class files and jar files under the working directory to the classpath.
+ addFileToClasspath("*", null, env)
--- End diff --
PWD/* will never be added because its not local:// and alternative name is null. We need to make sure * gets added.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SPARK-1395] Fix "local:" URI support in Yarn ...
Posted by tgravescs <gi...@git.apache.org>.
Github user tgravescs commented on a diff in the pull request:
https://github.com/apache/spark/pull/560#discussion_r13864023
--- Diff: yarn/common/src/main/scala/org/apache/spark/deploy/yarn/ClientBase.scala ---
@@ -342,24 +352,16 @@ trait ClientBase extends Logging {
sparkConf.set("spark.driver.extraJavaOptions", opts)
}
+ // Forward the Spark configuration to the application master / executors.
// TODO: it might be nicer to pass these as an internal environment variable rather than
// as Java options, due to complications with string parsing of nested quotes.
- if (args.amClass == classOf[ExecutorLauncher].getName) {
- // If we are being launched in client mode, forward the spark-conf options
- // onto the executor launcher
- for ((k, v) <- sparkConf.getAll) {
- javaOpts += "-D" + k + "=" + "\\\"" + v + "\\\""
--- End diff --
Note that this change breaks backwards compatibility with being able to set SPARK_JAVA_OPTS to include configs. I would rather not change it in this pr as it will take more investigation and testing to make sure it doesn't break anything.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SPARK-1395] Fix "local:" URI support in Yarn ...
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on the pull request:
https://github.com/apache/spark/pull/560#issuecomment-45370271
@pwendell the last commit I pushed to the PR changes some code you added in fb98488f. Can you take a look in case I misunderstood what you meant originally?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SPARK-1395] Fix "local:" URI support in Yarn ...
Posted by tgravescs <gi...@git.apache.org>.
Github user tgravescs commented on a diff in the pull request:
https://github.com/apache/spark/pull/560#discussion_r13858909
--- Diff: yarn/common/src/main/scala/org/apache/spark/deploy/yarn/ClientBase.scala ---
@@ -342,24 +352,16 @@ trait ClientBase extends Logging {
sparkConf.set("spark.driver.extraJavaOptions", opts)
}
+ // Forward the Spark configuration to the application master / executors.
// TODO: it might be nicer to pass these as an internal environment variable rather than
// as Java options, due to complications with string parsing of nested quotes.
- if (args.amClass == classOf[ExecutorLauncher].getName) {
- // If we are being launched in client mode, forward the spark-conf options
- // onto the executor launcher
- for ((k, v) <- sparkConf.getAll) {
- javaOpts += "-D" + k + "=" + "\\\"" + v + "\\\""
--- End diff --
was this change necessary? If not I would prefer to split this off into a separate jira. If it was please explain why.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SPARK-1395] Fix "local:" URI support in Yarn ...
Posted by tgravescs <gi...@git.apache.org>.
Github user tgravescs commented on the pull request:
https://github.com/apache/spark/pull/560#issuecomment-46847166
+1 looks good, thanks @vanzin.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SPARK-1395] Fix "local:" URI support in Yarn ...
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on the pull request:
https://github.com/apache/spark/pull/560#issuecomment-46624630
BTW I also tested "-Dspark.akka.logAkkaConfig=true" aside from my previous test case, and saw the akka logs in both driver and executor.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SPARK-1395] Fix "local:" URI support in Yarn ...
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on the pull request:
https://github.com/apache/spark/pull/560#issuecomment-46239570
Hi, does anybody have any more feedback about this?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SPARK-1395] Fix "local:" URI support in Yarn ...
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on the pull request:
https://github.com/apache/spark/pull/560#issuecomment-46596612
Your example works but only if you just want to propagate the property to the driver, and not the executors. Without being familiar with how things worked in 0.9. I'm a little sceptical that it's the full correct behaviour (since I'd expect SPARK_JAVA_OPTS to show up on both).
Anyway, I can make changes so that the use case you mention is covered.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SPARK-1395] Fix "local:" URI support in Yarn ...
Posted by tgravescs <gi...@git.apache.org>.
Github user tgravescs commented on the pull request:
https://github.com/apache/spark/pull/560#issuecomment-45146363
I haven't gotten all the way through this patch but please explain what you have done with the log4j stuff. at quick glance it looks like you just removed it all? Perhaps I'm missing something though.
You expect people to pass it with the --files option?
What about in the case where I don't want to specify one and want the default one (log4j-spark-container.properties) to be used? (added in https://issues.apache.org/jira/browse/SPARK-1252)
Also note we can't just remove the way it is done now as it breaks backwards compatibility.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SPARK-1395] Fix "local:" URI support in Yarn ...
Posted by tgravescs <gi...@git.apache.org>.
Github user tgravescs commented on a diff in the pull request:
https://github.com/apache/spark/pull/560#discussion_r13860204
--- Diff: yarn/common/src/main/scala/org/apache/spark/deploy/yarn/ClientBase.scala ---
@@ -386,12 +388,31 @@ trait ClientBase extends Logging {
object ClientBase extends Logging {
val SPARK_JAR: String = "__spark__.jar"
val APP_JAR: String = "__app__.jar"
- val LOG4J_PROP: String = "log4j.properties"
- val LOG4J_CONF_ENV_KEY: String = "SPARK_LOG4J_CONF"
val LOCAL_SCHEME = "local"
+ val CONF_SPARK_JAR = "spark.yarn.jar"
+ val CONF_SPARK_USER_JAR = "spark.yarn.user.jar"
val CONF_SPARK_YARN_SECONDARY_JARS = "spark.yarn.secondary.jars"
--- End diff --
adding a comment here about what spark.yarn.secondary.jars is would be nice too if you don't mind.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SPARK-1395] Fix "local:" URI support in Yarn ...
Posted by tgravescs <gi...@git.apache.org>.
Github user tgravescs commented on the pull request:
https://github.com/apache/spark/pull/560#issuecomment-46603671
so the example I gave above is on the master branch and the configs I set show up on both driver and executors. I'm not concerned with configs that don't start with "spark." as those aren't spark configs. When you say it doesn't show up on executors how are you checking? You don't see the logs in the executors that I listed? I ran on both 0.23 and 2.4 clusters. So I'm wondering why its not working for you.
Note that most configs get converted into SparkConf and sent to the executor via akka when it registers so they won't show up in the processes line with -D's or via getProperty. The security settings are special as they are needed before the registration happens.
Again I'm only concerned with actual spark configs (spark.*) and I'm only concerned about the spark framework properly reading them. I'm not concerned with application code reading them.
Another example you can use is :
export SPARK_JAVA_OPTS="-Dspark.authenticate=true -Dspark.ui.acls.enable=true -Dspark.akka.threads=10 -Dspark.akka.logAkkaConfig=true"
make sure that the akka settings get logged at the beginning of the executor processes.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SPARK-1395] Fix "local:" URI support in Yarn ...
Posted by tgravescs <gi...@git.apache.org>.
Github user tgravescs commented on the pull request:
https://github.com/apache/spark/pull/560#issuecomment-46583474
@vanzin I'm not sure how your change fixes still being able to use SPARK_JAVA_OPTS to set configs.
SPARK_JAVA_OPTS=-Dspark.authentication=true. When I just tried I still get the error (with both spark-submit and spark-class)
Caused by: java.lang.Exception: spark.executor.extraJavaOptions is not allowed to set Spark options (was '-Dspark.authentication=true)'. Set them directly on a SparkConf or in a properties file when using ./bin/spark-submit.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SPARK-1395] Fix "local:" URI support in Yarn ...
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on the pull request:
https://github.com/apache/spark/pull/560#issuecomment-45131171
Ah, that guy. Sorry, my bad. :-) It made my life easier debugging things, but not sure how many people actively develop Spark on top of yarn-alpha. Wouldn't hurt to add it, though.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SPARK-1395] Fix "local:" URI support in Yarn ...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/spark/pull/560
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SPARK-1395] Fix "local:" URI support in Yarn ...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/560#issuecomment-41443677
Can one of the admins verify this patch?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SPARK-1395] Fix "local:" URI support in Yarn ...
Posted by tgravescs <gi...@git.apache.org>.
Github user tgravescs commented on a diff in the pull request:
https://github.com/apache/spark/pull/560#discussion_r13406748
--- Diff: yarn/common/src/main/scala/org/apache/spark/deploy/yarn/ClientBase.scala ---
@@ -378,15 +378,34 @@ trait ClientBase extends Logging {
}
}
-object ClientBase {
+object ClientBase extends Logging {
val SPARK_JAR: String = "__spark__.jar"
val APP_JAR: String = "__app__.jar"
- val LOG4J_PROP: String = "log4j.properties"
- val LOG4J_CONF_ENV_KEY: String = "SPARK_LOG4J_CONF"
val LOCAL_SCHEME = "local"
+ val CONF_SPARK_JAR = "spark.yarn.jar"
+ val CONF_SPARK_USER_JAR = "spark.yarn.user.jar"
--- End diff --
when exactly would this be used? You have to specify the jar with spark-submit.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SPARK-1395] Fix "local:" URI support in Yarn ...
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on a diff in the pull request:
https://github.com/apache/spark/pull/560#discussion_r13870892
--- Diff: yarn/common/src/main/scala/org/apache/spark/deploy/yarn/ClientBase.scala ---
@@ -342,24 +352,16 @@ trait ClientBase extends Logging {
sparkConf.set("spark.driver.extraJavaOptions", opts)
}
+ // Forward the Spark configuration to the application master / executors.
// TODO: it might be nicer to pass these as an internal environment variable rather than
// as Java options, due to complications with string parsing of nested quotes.
- if (args.amClass == classOf[ExecutorLauncher].getName) {
- // If we are being launched in client mode, forward the spark-conf options
- // onto the executor launcher
- for ((k, v) <- sparkConf.getAll) {
- javaOpts += "-D" + k + "=" + "\\\"" + v + "\\\""
--- End diff --
So here's what's happening.
`prepareLocalResources()` propagates some settings (previously `CONF_SPARK_YARN_SECONDARY_JARS`, and now the two new settings I'm adding) using SparkConf.set. In cluster mode, `createContainerLaunchContext()` propagates only things that are set as system properties, so those config options are not provided to the executors. So if your executor depends on something that is in those options, it will fail.
I'll double-check whether `SPARK_JAVA_OPTS` works as before and make adjustments if necessary, but the change is needed for things to work correctly.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SPARK-1395] Fix "local:" URI support in Yarn ...
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on the pull request:
https://github.com/apache/spark/pull/560#issuecomment-46606660
@tgravescs I posted my test code a few comments above. I'd expect "SPARK_JAVA_OPTS" to be just that - JVM options, not SparkConf options. So the properties I'm setting via JVM options are not showing up in the executors.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SPARK-1395] Fix "local:" URI support in Yarn ...
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on the pull request:
https://github.com/apache/spark/pull/560#issuecomment-44569275
I rebased and cleaned up the code some more. I think it's in pretty good shape now, and the tests are much better.
Tested on yarn client / cluster, with and without extra jar dependencies.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SPARK-1395] Fix "local:" URI support in Yarn ...
Posted by tgravescs <gi...@git.apache.org>.
Github user tgravescs commented on a diff in the pull request:
https://github.com/apache/spark/pull/560#discussion_r13862215
--- Diff: yarn/common/src/test/scala/org/apache/spark/deploy/yarn/ClientBaseSuite.scala ---
@@ -68,6 +77,65 @@ class ClientBaseSuite extends FunSuite {
}
}
+ private val SPARK = "local:/sparkJar"
+ private val USER = "local:/userJar"
+ private val ADDED = "local:/addJar1,local:/addJar2,/addJar3"
+
+ test("Local jar URIs") {
+ val conf = new Configuration()
+ val sparkConf = new SparkConf().set(ClientBase.CONF_SPARK_JAR, SPARK)
+ val env = new MutableHashMap[String, String]()
+ val args = new ClientArguments(Array("--jar", USER, "--addJars", ADDED), sparkConf)
+
+ ClientBase.populateClasspath(args, conf, sparkConf, env, None)
+
+ val jars = env("CLASSPATH").split(File.pathSeparator)
+ s"$SPARK,$USER,$ADDED".split(",").foreach({ jar =>
+ val uri = new URI(jar)
+ if (ClientBase.LOCAL_SCHEME.equals(uri.getScheme())) {
+ jars should contain (uri.getPath())
+ } else {
+ jars should not contain (uri.getPath())
+ }
+ })
+ jars should not contain (ClientBase.SPARK_JAR)
+ jars should not contain (ClientBase.APP_JAR)
+ }
+
+ test("Jar path propagation through SparkConf") {
+ val conf = new Configuration()
+ val sparkConf = new SparkConf().set(ClientBase.CONF_SPARK_JAR, SPARK)
+ val yarnConf = new YarnConfiguration()
+ val args = new ClientArguments(Array("--jar", USER, "--addJars", ADDED), sparkConf)
+
+ val client = spy(new DummyClient(args, conf, sparkConf, yarnConf))
+ doReturn(new Path("/")).when(client).copyRemoteFile(any(classOf[Path]),
+ any(classOf[Path]), anyShort(), anyBoolean())
+
+ var tempDir = Files.createTempDir();
+ try {
+ client.prepareLocalResources(tempDir.getAbsolutePath())
+ sparkConf.getOption(ClientBase.CONF_SPARK_USER_JAR) should be (Some(USER))
+
+ // The non-local path should be propagated by name only, since it will end up in the app's
+ // staging dir.
+ val expected = ADDED.split(",")
+ .map(p => {
+ val uri = new URI(p)
+ if (ClientBase.LOCAL_SCHEME == uri.getScheme()) {
+ p
+ } else {
+ Option(uri.getFragment()).getOrElse(new File(p).getName())
+ }
+ })
+ .mkString(",")
+
+ sparkConf.getOption(ClientBase.CONF_SPARK_YARN_SECONDARY_JARS) should be (Some(expected))
+ } finally {
+ Utils.deleteRecursively(tempDir)
+ }
+ }
+
--- End diff --
can you please add a test for PWD/*
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SPARK-1395] Fix "local:" URI support in Yarn ...
Posted by witgo <gi...@git.apache.org>.
Github user witgo commented on the pull request:
https://github.com/apache/spark/pull/560#issuecomment-46036670
@vanzin Can you merge the latest code?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SPARK-1395] Fix "local:" URI support in Yarn ...
Posted by tgravescs <gi...@git.apache.org>.
Github user tgravescs commented on the pull request:
https://github.com/apache/spark/pull/560#issuecomment-46598903
what config is not showing up for you on the driver?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SPARK-1395] Fix "local:" URI support in Yarn ...
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on a diff in the pull request:
https://github.com/apache/spark/pull/560#discussion_r13867773
--- Diff: yarn/common/src/main/scala/org/apache/spark/deploy/yarn/ClientBase.scala ---
@@ -342,24 +352,16 @@ trait ClientBase extends Logging {
sparkConf.set("spark.driver.extraJavaOptions", opts)
}
+ // Forward the Spark configuration to the application master / executors.
// TODO: it might be nicer to pass these as an internal environment variable rather than
// as Java options, due to complications with string parsing of nested quotes.
- if (args.amClass == classOf[ExecutorLauncher].getName) {
- // If we are being launched in client mode, forward the spark-conf options
- // onto the executor launcher
- for ((k, v) <- sparkConf.getAll) {
- javaOpts += "-D" + k + "=" + "\\\"" + v + "\\\""
--- End diff --
Can you elaborate? SparkConf will include all the system properties that start with "spark", which is exactly what the previous code was doing when reading system properties directly.
I'll re-test without this fix (my tests were failing without it but after reading the rest of the code I'm not so sure it was because of this), but in general I think this is a cleaner way of doing it (since it treats both cases as the same).
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SPARK-1395] Fix "local:" URI support in Yarn ...
Posted by tgravescs <gi...@git.apache.org>.
Github user tgravescs commented on a diff in the pull request:
https://github.com/apache/spark/pull/560#discussion_r13858930
--- Diff: yarn/common/src/main/scala/org/apache/spark/deploy/yarn/ClientBase.scala ---
@@ -378,15 +378,34 @@ trait ClientBase extends Logging {
}
}
-object ClientBase {
+object ClientBase extends Logging {
val SPARK_JAR: String = "__spark__.jar"
val APP_JAR: String = "__app__.jar"
- val LOG4J_PROP: String = "log4j.properties"
- val LOG4J_CONF_ENV_KEY: String = "SPARK_LOG4J_CONF"
val LOCAL_SCHEME = "local"
+ val CONF_SPARK_JAR = "spark.yarn.jar"
+ val CONF_SPARK_USER_JAR = "spark.yarn.user.jar"
--- End diff --
please add a comment explaining meant to be used internally only
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SPARK-1395] Fix "local:" URI support in Yarn ...
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on the pull request:
https://github.com/apache/spark/pull/560#issuecomment-46046934
@witgo: on it, will retest and update soon. I'll also take a closer look at your related change to make sure I got everything right.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SPARK-1395] Fix "local:" URI support in Yarn ...
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on the pull request:
https://github.com/apache/spark/pull/560#issuecomment-42463814
Just rebased on top of master. No changes.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SPARK-1395] Fix "local:" URI support in Yarn ...
Posted by tgravescs <gi...@git.apache.org>.
Github user tgravescs commented on a diff in the pull request:
https://github.com/apache/spark/pull/560#discussion_r13407600
--- Diff: yarn/common/src/main/scala/org/apache/spark/deploy/yarn/ClientBase.scala ---
@@ -378,15 +378,34 @@ trait ClientBase extends Logging {
}
}
-object ClientBase {
+object ClientBase extends Logging {
val SPARK_JAR: String = "__spark__.jar"
val APP_JAR: String = "__app__.jar"
- val LOG4J_PROP: String = "log4j.properties"
- val LOG4J_CONF_ENV_KEY: String = "SPARK_LOG4J_CONF"
val LOCAL_SCHEME = "local"
+ val CONF_SPARK_JAR = "spark.yarn.jar"
+ val CONF_SPARK_USER_JAR = "spark.yarn.user.jar"
--- End diff --
Nevermind I assume its only meant to be used internally.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SPARK-1395] Fix "local:" URI support in Yarn ...
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on the pull request:
https://github.com/apache/spark/pull/560#issuecomment-46599537
On the master branch, when set using SPARK_JAVA_OPTS: anything that does not start with "spark." does not show up in the driver. Nothing at all shows up in executors.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SPARK-1395] Fix "local:" URI support in Yarn ...
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on a diff in the pull request:
https://github.com/apache/spark/pull/560#discussion_r13409609
--- Diff: yarn/common/src/main/scala/org/apache/spark/deploy/yarn/ClientBase.scala ---
@@ -378,15 +378,34 @@ trait ClientBase extends Logging {
}
}
-object ClientBase {
+object ClientBase extends Logging {
val SPARK_JAR: String = "__spark__.jar"
val APP_JAR: String = "__app__.jar"
- val LOG4J_PROP: String = "log4j.properties"
- val LOG4J_CONF_ENV_KEY: String = "SPARK_LOG4J_CONF"
val LOCAL_SCHEME = "local"
+ val CONF_SPARK_JAR = "spark.yarn.jar"
+ val CONF_SPARK_USER_JAR = "spark.yarn.user.jar"
--- End diff --
Yes, the user jar is an internal thing. It could probably be made not necessary by having SparkSubmit add it to the head of the "addJars" list, though. I also had it on my list to check how SparkSubmit deals with a local: jar (I believe it doesn't), but both of those can be handled separately.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SPARK-1395] Fix "local:" URI support in Yarn ...
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on the pull request:
https://github.com/apache/spark/pull/560#issuecomment-46620145
Rebased and fixed the SPARK_JAVA_OPTS issues. @tgravescs, let me know if that addresses your concerns. Thanks!
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SPARK-1395] Fix "local:" URI support in Yarn ...
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on the pull request:
https://github.com/apache/spark/pull/560#issuecomment-46592132
HI @tgravescs,
When I tested I used a non `spark.` property and it worked. But I just want to make sure we're on the same page as what do you think SPARK_JAVA_OPTS should do. Here's some code I have which is called both by the driver and executors:
val silly1 = System.getProperty("silly.property")
val silly2 = System.getProperty("spark.silly.property")
logInfo(s"silly.property = $silly1, spark.silly.property = $silly2")
if (silly1 == null && silly2 == null) {
throw new IllegalArgumentException("silly.property nor spark.silly.property is set!")
}
I then run this (against the master branch) in yarn-cluster mode:
SPARK_JAVA_OPTS='-Dspark.silly.property=bar -Dsilly.property=foo' ./bin/spark-submit ...
The driver launches but executors die with:
14/06/19 10:31:19 INFO SillyDepSleeper: silly.property = null, spark.silly.property = null
14/06/19 10:31:19 ERROR Executor: Exception in task ID 10
java.lang.IllegalArgumentException: silly.property nor spark.silly.property is set!
at com.cloudera.ss.SillyDepSleeper$.checkProps(SillyDepSleeper.scala:32)
So I'm not sure SPARK_JAVA_OPTS is really working on master at all. Another thing I tried is to use just ` SPARK_JAVA_OPTS='-Dsilly.property=foo' `, and in that case the driver doesn't start either (meaning non-"spark." properties are not propagated).
I can look at fixing the problem you mention, but it looks like SPARK_JAVA_OPTS has some questionable behaviour even without my changes.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SPARK-1395] Fix "local:" URI support in Yarn ...
Posted by tgravescs <gi...@git.apache.org>.
Github user tgravescs commented on the pull request:
https://github.com/apache/spark/pull/560#issuecomment-46594901
so the idea is that you can set spark configs via SPARK_JAVA_OPTS in order to be backwards compatible with 0.9. Before the SparkConf (conf/spark-defaults.conf) and spark-submit the only way to specify the spark configs was either through SPARK_JAVA_OPTS on command line or spark-env.sh. So that is what I've been looking at.
My examples which is pretty easy to see if works is set:
SPARK_JAVA_OPTS="-Dspark.authenticate=true -Dspark.ui.acls.enable=true"
Run one of the Spark examples (SparkPi):
./bin/spark-submit examples/target/scala-2.10/spark-examples-1.1.0-SNAPSHOT-hadoop0.23.9.jar --master yarn --deploy-mode cluster --class org.apache.spark.examples.SparkPi
Hopefully people using spark-submit will have converted to use configs so testing with spark-class is more important for those that haven't converted:
./bin/spark-class org.apache.spark.deploy.yarn.Client --jar examples/target/scala-2.10/spark-examples-1.1.0-SNAPSHOT-hadoop0.23.9.jar --class org.apache.spark.examples.SparkPi
With this change it fails with the error I listed, without this change it works and you can view the logs and see that the configs take affect:
14/06/19 17:54:34 INFO SecurityManager: SecurityManager: authentication enabled; ui acls enabled; users with view permissions: Set(yarn, tgraves)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: [SPARK-1395] Fix "local:" URI support in Yarn ...
Posted by tgravescs <gi...@git.apache.org>.
Github user tgravescs commented on a diff in the pull request:
https://github.com/apache/spark/pull/560#discussion_r13868874
--- Diff: yarn/common/src/main/scala/org/apache/spark/deploy/yarn/ClientBase.scala ---
@@ -342,24 +352,16 @@ trait ClientBase extends Logging {
sparkConf.set("spark.driver.extraJavaOptions", opts)
}
+ // Forward the Spark configuration to the application master / executors.
// TODO: it might be nicer to pass these as an internal environment variable rather than
// as Java options, due to complications with string parsing of nested quotes.
- if (args.amClass == classOf[ExecutorLauncher].getName) {
- // If we are being launched in client mode, forward the spark-conf options
- // onto the executor launcher
- for ((k, v) <- sparkConf.getAll) {
- javaOpts += "-D" + k + "=" + "\\\"" + v + "\\\""
--- End diff --
export SPARK_JAVA_OPTS="-Dspark...=foo" . This needs to work for backwards compatibility but it doesn't with this code change because SparkConf throws an error saying you aren't supposed to do this.
I agree it is cleaner the way you have it but its going to require more investigation and a bunch of testing to make sure it works properly and I would rather have it split off into another jira/pr. There are already alot of things in this pr.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---