-
Notifications
You must be signed in to change notification settings - Fork 28.7k
[SPARK-1870] Made deployment with --jars work in yarn-standalone mode. #1013
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
Conversation
Merged build triggered. |
Merged build started. |
Merged build finished. All automated tests passed. |
All automated tests passed. |
// Normally the users app.jar is last in case conflicts with spark jars | ||
val userClasspathFirst = sparkConf.get("spark.yarn.user.classpath.first", "false").toBoolean | ||
if (userClasspathFirst) { | ||
Apps.addToEnvironment(env, Environment.CLASSPATH.name, Environment.PWD.$() + | ||
Path.SEPARATOR + APP_JAR) | ||
cachedSecondaryJarLinks.foreach(jarLink => | ||
Apps.addToEnvironment(env, Environment.CLASSPATH.name, Environment.PWD.$() + | ||
Path.SEPARATOR + jarLink)) |
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.
minor: add two more spaces.
@dbtsai Thanks for fixing it in branch-0.9! Did you verify the patch on a YARN cluster? |
Work in my local VM. Should work in real yarn cluster. Will test it tomorrow in the office. |
@dbtsai could you update the title of this PR to make it more clear what issue this relates to? |
Merged build triggered. |
Merged build started. |
Tested in PivotalHD 1.1 Yarn 4 node cluster. With --addjars file:///somePath/to/jar, launching spark application works. |
Merged build finished. All automated tests passed. |
All automated tests passed. |
@@ -281,18 +280,19 @@ class Client(args: ClientArguments, conf: Configuration, sparkConf: SparkConf) | |||
} | |||
|
|||
// Handle jars local to the ApplicationMaster. | |||
var cachedSecondaryJarLinks = ListBuffer.empty[String] |
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 think this can be a val
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.
thks.
@@ -507,12 +508,19 @@ object Client { | |||
Apps.addToEnvironment(env, Environment.CLASSPATH.name, Environment.PWD.$() + | |||
Path.SEPARATOR + LOG4J_PROP) | |||
} | |||
|
|||
val cachedSecondaryJarLinks = | |||
sparkConf.getOption(CONF_SPARK_YARN_SECONDARY_JARS).getOrElse("").split(",") |
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 actually returns a one element Array("")
if there are no secondary jars. We should .filter(_.nonEmpty)
after splitting.
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.
Thanks. You are right. It will add empty string to array, and then add the folder without file into classpath. Will fix in master as well.
Merged build triggered. |
|
||
val cachedSecondaryJarLinks = | ||
sparkConf.getOption(CONF_SPARK_YARN_SECONDARY_JARS).getOrElse("").split(",") | ||
|
||
// Normally the users app.jar is last in case conflicts with spark jars | ||
val userClasspathFirst = sparkConf.get("spark.yarn.user.classpath.first", "false") | ||
.toBoolean |
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.
minor: Not your code, but could you bump .toBoolean
up one line? I'm pretty sure it's under 100 characters.
Merged build triggered. |
Merged build started. |
1 similar comment
Merged build started. |
Merged build finished. All automated tests passed. |
All automated tests passed. |
Merged build finished. All automated tests passed. |
All automated tests passed. |
@@ -507,12 +508,18 @@ object Client { | |||
Apps.addToEnvironment(env, Environment.CLASSPATH.name, Environment.PWD.$() + | |||
Path.SEPARATOR + LOG4J_PROP) | |||
} | |||
|
|||
val cachedSecondaryJarLinks = | |||
sparkConf.getOption(CONF_SPARK_YARN_SECONDARY_JARS).getOrElse("").split(",").filter(_.nonEmpty) |
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.
line too wide?
Merged build triggered. |
Merged build started. |
Merged build finished. |
Refer to this link for build results: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/15600/ |
Jenkins, retest this please. |
Merged build triggered. |
Merged build started. |
Merged build finished. All automated tests passed. |
All automated tests passed. |
LGTM. Thanks! |
Ported from 1.0 branch to 0.9 branch. Sent secondary jars to distributed cache of all containers and add the cached jars to classpath before executors start. Author: DB Tsai <[email protected]> Closes #1013 from dbtsai/branch-0.9 and squashes the following commits: c5696f4 [DB Tsai] fix line too long b085f10 [DB Tsai] Make sure that empty string is filtered out when we get secondary jars 3cc1085 [DB Tsai] changed from var to val ab94aa1 [DB Tsai] Code formatting. 0956af9 [DB Tsai] Ported SPARK-1870 from 1.0 branch to 0.9 branch
Ported from 1.0 branch to 0.9 branch. Sent secondary jars to distributed cache of all containers and add the cached jars to classpath before executors start.