Skip to content

[SPARK-1825] Fixes cross-platform submit problem #899

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.

Sign up for GitHub

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

Closed
zeodtr wants to merge 1 commit into from
Closed

[SPARK-1825] Fixes cross-platform submit problem #899

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 9 additions & 8 deletions yarn/common/src/main/scala/org/apache/spark/deploy/yarn/ClientBase.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ import org.apache.hadoop.mapreduce.MRJobConfig
import org.apache.hadoop.security.UserGroupInformation
import org.apache.hadoop.util.StringUtils
import org.apache.hadoop.yarn.api._
import org.apache.hadoop.yarn.api.ApplicationConstants
import org.apache.hadoop.yarn.api.ApplicationConstants.Environment
import org.apache.hadoop.yarn.api.protocolrecords._
import org.apache.hadoop.yarn.api.records._
Expand Down Expand Up @@ -309,7 +310,7 @@ trait ClientBase extends Logging {
// Add Xmx for AM memory
JAVA_OPTS += "-Xmx" + amMemory + "m"

val tmpDir = new Path(Environment.PWD.$(), YarnConfiguration.DEFAULT_CONTAINER_TEMP_DIR)
val tmpDir = new Path(Environment.PWD.$$(), YarnConfiguration.DEFAULT_CONTAINER_TEMP_DIR)
Copy link
Member

@srowen srowen May 28, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm just curious about what the $ vs $$ change does here.

Copy link
Author

@zeodtr zeodtr May 28, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is similar to ApplicationConstants.CLASS_PATH_SEPARATOR.
$$() wraps the string with "{{" and "}}", rather than 'hardcoding' "%" or "$" as is done by $().
And the cluster replaces it with the real script variable marker when it finds the "{{" and "}}" pair.

Copy link
Member

@srowen srowen May 28, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dumb question again that maybe someone else has -- is that going to work on Hadoop < 2.4, and I presume it works on both Windows and Linux of course.

Copy link
Author

@zeodtr zeodtr May 29, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's the same as <CPS>. Hadoop < 2.4 will not recognize it.
BTW, CLASS_PATH_SEPARATOR and $$() are annotated with @Unstable.

Copy link
Contributor

@vanzin vanzin Jun 5, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps the best compromise here is to use reflection?

Check whether those constants exist, and use them if they do. Otherwise maintain current behavior. That way someone who has a 2.4-based installation will get the benefits, and things will keep working as they do today for those who don't.

Copy link
Author

@zeodtr zeodtr Jul 22, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we have a new spark configuration variable, like "spark.yarn.submit.crossplatform"?
If so, the code here can be applied conditionally "hard-coding" required separators, without using reflection.
(But currently I my knowledge on Spark source code and Scala is limited to implement it myself.)

Copy link
Contributor

@tgravescs tgravescs Sep 3, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not quite sure what you mean by a config option. you mean build time? Otherwise I would expect anything <2.4 to fail the to build.

Copy link
Author

@zeodtr zeodtr Sep 4, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tgravescs No, I mean run time. To apply the config option idea, the code I requested must be modified to use hard-coding approach that does not use 2.4-only methods.

Copy link
Contributor

@andrewor14 andrewor14 Sep 5, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems the only way to special case this is through reflection. Otherwise it won't even compile for < 2.4. Also, I'm not sure if we need a config here. We can just try to load the $$ method, and if it's not there we fallback to $. Same goes for ApplicationConstants.CLASS_PATH_SEPARATOR.

Copy link
Author

@zeodtr zeodtr Sep 10, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@andrewor14 What I meant for hard-coding is to use string value "{{" and "}}" directly without even calling $$() in the Spark code when Hadoop version is >= 2.4 (or config is set). But I agree that the reflection is cleaner solution.

JAVA_OPTS += "-Djava.io.tmpdir=" + tmpDir

// TODO: Remove once cpuset version is pushed out.
Expand Down Expand Up @@ -357,7 +358,7 @@ trait ClientBase extends Logging {
JAVA_OPTS += ClientBase.getLog4jConfiguration(localResources)

// Command for the ApplicationMaster
val commands = Seq(Environment.JAVA_HOME.$() + "/bin/java", "-server") ++
val commands = Seq(Environment.JAVA_HOME.$$() + "/bin/java", "-server") ++
JAVA_OPTS ++
Seq(args.amClass, "--class", args.userClass, "--jar ", args.userJar,
userArgsToString(args),
Expand Down Expand Up @@ -396,7 +397,7 @@ object ClientBase {
if (classpathEntries != null) {
for (c <- classpathEntries) {
YarnSparkHadoopUtil.addToEnvironment(env, Environment.CLASSPATH.name, c.trim,
File.pathSeparator)
ApplicationConstants.CLASS_PATH_SEPARATOR)
}
}

Expand All @@ -406,7 +407,7 @@ object ClientBase {
if (mrClasspathEntries != null) {
for (c <- mrClasspathEntries) {
YarnSparkHadoopUtil.addToEnvironment(env, Environment.CLASSPATH.name, c.trim,
File.pathSeparator)
ApplicationConstants.CLASS_PATH_SEPARATOR)
}
}
}
Expand Down Expand Up @@ -473,15 +474,15 @@ object ClientBase {
if (localPath != null) {
val parentPath = new File(localPath).getParent()
YarnSparkHadoopUtil.addToEnvironment(env, Environment.CLASSPATH.name, parentPath,
File.pathSeparator)
ApplicationConstants.CLASS_PATH_SEPARATOR)
Copy link
Member

@srowen srowen May 28, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As you say, this value only appeared in Hadoop 2.4.0:

http://hadoop.apache.org/docs/r2.4.0/api/org/apache/hadoop/yarn/api/ApplicationConstants.html
http://hadoop.apache.org/docs/r2.3.0/api/org/apache/hadoop/yarn/api/ApplicationConstants.html

File.pathSeparator should already be ":" vs ";", which is what you intend right?
http://docs.oracle.com/javase/7/docs/api/java/io/File.html#pathSeparator

I'm missing what this changes then. I understand that the char may vary on the client vs cluster, and that's why it's right to reference a symbolic constant, but these seem to be the same in that respect.

Copy link
Author

@zeodtr zeodtr May 28, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The value of ApplicationConstants.CLASS_PATH_SEPARATOR is "<CPS>" - neither ":" nor ";".
The point is that the separator will be chosen by the cluster(in my case, linux machine), rather than the client(in my case, Windows machine) if ApplicationConstants.CLASS_PATH_SEPARATOR is used.
That is, the server hadoop module will find "<CPS>" string in the path string and replace it with the real separator appropriate to its OS.
But current Spark 1.0 code 'hardcodes' the separator on the client side, by using File.pathSeparator. Then the Windows-style path string (that contains ';' which confuses the linux shell script interpreter) will be sent to the linux cluster, in my case.

Copy link
Author

@zeodtr zeodtr May 28, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BTW, maybe this is a 'yarn-client' mode-only problem.
I have not tested the 'yarn-cluster' mode.

Copy link
Member

@srowen srowen May 28, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh I get it. Because the client forms the path but sends it to the server, so it's produced and consumed in different places. The bad news is that you can't use this constant, but, I suppose you can literally specify <CPS>. But will that fail for Hadoop < 2.4?

Copy link
Author

@zeodtr zeodtr May 29, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it will fail for Hadoop < 2.4.
The hadoop server code that recognizes <CPS> was committed at Mar 17 2014, to resolve YARN-1824. The code is in org.apache.hadoop.yarn.server.nodemanager.containermanager.launcher.ContainerLaunch. (see this github link)
So, maybe the best solution that does not require Hadoop 2.4.0 is to build the environment variables on the cluster side. (I don't know how to do that - is it even possible?)

}
}

/** Add entry to the classpath. */
def addClasspathEntry(path: String) = YarnSparkHadoopUtil.addToEnvironment(env,
Environment.CLASSPATH.name, path, File.pathSeparator)
Environment.CLASSPATH.name, path, ApplicationConstants.CLASS_PATH_SEPARATOR)
/** Add entry to the classpath. Interpreted as a path relative to the working directory. */
def addPwdClasspathEntry(entry: String) = addClasspathEntry(Environment.PWD.$() + Path.SEPARATOR + entry)
def addPwdClasspathEntry(entry: String) = addClasspathEntry(Environment.PWD.$$() + Path.SEPARATOR + entry)

extraClassPath.foreach(addClasspathEntry)

Expand All @@ -500,7 +501,7 @@ object ClientBase {
cachedSecondaryJarLinks.foreach(addPwdClasspathEntry)
}
// Append all class files and jar files under the working directory to the classpath.
addClasspathEntry(Environment.PWD.$())
addClasspathEntry(Environment.PWD.$$())
addPwdClasspathEntry("*")
}

Expand Down
4 changes: 2 additions & 2 deletions yarn/common/src/main/scala/org/apache/spark/deploy/yarn/ExecutorRunnableUtil.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ trait ExecutorRunnableUtil extends Logging {
}

JAVA_OPTS += "-Djava.io.tmpdir=" +
new Path(Environment.PWD.$(), YarnConfiguration.DEFAULT_CONTAINER_TEMP_DIR)
new Path(Environment.PWD.$$(), YarnConfiguration.DEFAULT_CONTAINER_TEMP_DIR)
JAVA_OPTS += ClientBase.getLog4jConfiguration(localResources)

// Certain configs need to be passed here because they are needed before the Executor
Expand Down Expand Up @@ -96,7 +96,7 @@ trait ExecutorRunnableUtil extends Logging {
}
*/

val commands = Seq(Environment.JAVA_HOME.$() + "/bin/java",
val commands = Seq(Environment.JAVA_HOME.$$() + "/bin/java",
"-server",
// Kill if OOM is raised - leverage yarn's failure handling to cause rescheduling.
// Not killing the task leaves various aspects of the executor and (to some extent) the jvm in
Expand Down