-
Notifications
You must be signed in to change notification settings - Fork 28.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[SPARK-1395] Allow "local:" URIs to work on Yarn. #303
Conversation
Can one of the admins verify this patch? |
@@ -427,28 +430,49 @@ object ClientBase { | |||
} | |||
|
|||
def populateClasspath(conf: Configuration, sparkConf: SparkConf, addLog4j: Boolean, env: HashMap[String, String]) { | |||
var appJarEnvProperty = "SPARK_YARN_APP_JAR" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SPARK_YARN_APP_JAR is no longer used in master branch. It was unneeded. If you are using client mode you should use distributed cache or add jar to distribute your jar.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note this was also only a yarn-client mode thing and never applied to yarn-cluster/yarn-standalone mode, so I suspect this doesn't make it work for the yarn-cluster mode.
From my understanding local: is a spark thing that says the file is on each worker. This is a bit of an odd usage on yarn. Generally Yarn recommends sending things with the application so that you can easily upgrade or use new/different versions of applications. I can see it maybe being used for the spark jar but it doesn't really make sense to me for the application jar or the logging config which I would expect to be very different per user. If you put those in the public distributed cache, they will be there on all nodes for most applications after the first app downloads them. I am ok with making it work though since seems to be a standard Spark thing. |
@tgravescs yeah, I'm following the online docs and the cluster mode one lists "local:" as an option. Also probably why I'm still using SPARK_YARN_APP_JAR; I see now that the examples are using SparkContext.jarOfClass, so I'll remove that dead code and make sure the code still does what it's supposed to do for the local: case (since the fat example jar may have masked bugs in my change). |
Updated the patch to handle all cases of files passed in the yarn client's ClientArguments. Tested by running both with SPARK_JAR using local: and not using local, with a modified SparkPi.scala with hardcoded "local:" URI for the examples jar; verified custom log4j.properties also worked. |
val linkname = Option(localURI.getFragment()).getOrElse(localPath.getName()) | ||
val destPath = copyRemoteFile(dst, localPath, replication) | ||
// Only add the resource to the Spark ApplicationMaster. | ||
val appMasterOnly = true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is overriding the value you are passing in so its getting set to true for files and archives. Just remove it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch, copy & paste error.
Can someone with permissions kick jenkins? |
Jenkins, test this please |
Merged build triggered. |
Merged build started. |
Merged build finished. All automated tests passed. |
All automated tests passed. |
Not sure if you intended to make this work, but local: doesn't work in the context of running yarn-cluster mode and specifying the --jar? ./bin/spark-class org.apache.spark.deploy.yarn.Client --jar local:/home/tgraves/test2/tgravescs-spark/examples/target/scala-2.10/spark-examples_2.10-assembly-1.0.0-SNAPSHOT.jar --class org.apache.spark.examples.SparkPi --args yarn-cluster local: also doesn't work with the spark-shell in yarn-client mode. I'm fine with filing a separate jira for that. We should document in the jira that it is only in the yarn-cluster mode. |
Code changes look good other then the above mentioned use cases. |
Testing yarn-cluster is on my todo list, haven't gotten to that yet. If I can fix before anybody merges this pr, great. :-) Haven't really tried spark-shell on yarn yet, either. |
The issue with yarn-cluster is the following: SparkPi.scala uses SparkContext.jarOfClass() to define which jar to add to the SparkContext. This ends up adding the path of the jar without the "local:" prefix, which means the jar is expected to be in the distributed cache (as per the comment in SparkContext: "In order for this to work in yarn-cluster mode the user must specify the --addjars option"). If you add "-addJars /home/tgraves/test2/tgravescs-spark/examples/target/scala-2.10/spark-examples_2.10-assembly-1.0.0-SNAPSHOT.jar" to your command line it works (well, it works for me), but it sort of defeats the purpose of using local: URIs. I had a modified SparkPi in my tree that hardcoded a local: URI for the addJar() argument, and that worked fine without needing to add the extra argument (and did not incur in extra copying of the jar around). I'm not sure there's an easy way to fix this (how can SparkPi know to add the jar with a local: URI without some kind of command line argument telling it to do so?), but it's caused by the client code (in this case, SparkPi), so I'm less concerned. |
Ok, thanks for looking into that. If this is just because of the way the spark examples are working and it works in the normal use case its fine. |
|
||
/** | ||
* Adds the given path to the classpath, handling "local:" URIs correctly. | ||
* <p/> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure this mark up is meaningful for scala docs... what is the reason for these tags?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's javadoc (which is supposed to be well-formatted HTML). I saw somewhere that comments should use javadoc-style instead of scaladoc style, but that might have been just for the first line. I'll take a look at what scaladoc expects.
Sorry for my delay. The changes look good. Could you upmerge this to the master? |
Don't distribute resources that have the "local" scheme in their URIs. Also fix an issue where the classpath for custom log4j config files was incorrectly being set to include the file's path (instead of its parent), and propagate the user log4j configuration to the AM / workers. This has the side effect of eliminating the "SPARK_YARN_LOG4J_CONF" environment variable, which used to be used by the ExecutorRunner class, in favor of the "SPARK_LOG4J_CONF" variable used by the Yarn client class.
Rebased on top of current master; re-built and re-tested. |
I merged this to master and branch-1.0 Thanks @vanzin! |
This only works for the three paths defined in the environment (SPARK_JAR, SPARK_YARN_APP_JAR and SPARK_LOG4J_CONF). Tested by running SparkPi with local: and file: URIs against Yarn cluster (no "upload" shows up in logs in the local case). Author: Marcelo Vanzin <[email protected]> Closes #303 from vanzin/yarn-local and squashes the following commits: 82219c1 [Marcelo Vanzin] [SPARK-1395] Allow "local:" URIs to work on Yarn. (cherry picked from commit 6904750) Signed-off-by: Thomas Graves <[email protected]>
This only works for the three paths defined in the environment (SPARK_JAR, SPARK_YARN_APP_JAR and SPARK_LOG4J_CONF). Tested by running SparkPi with local: and file: URIs against Yarn cluster (no "upload" shows up in logs in the local case). Author: Marcelo Vanzin <[email protected]> Closes apache#303 from vanzin/yarn-local and squashes the following commits: 82219c1 [Marcelo Vanzin] [SPARK-1395] Allow "local:" URIs to work on Yarn.
This reverts commit e017c20.
Add sleep for debuging bosh job
This only works for the three paths defined in the environment
(SPARK_JAR, SPARK_YARN_APP_JAR and SPARK_LOG4J_CONF).
Tested by running SparkPi with local: and file: URIs against Yarn cluster (no "upload" shows up in logs in the local case).