-
Notifications
You must be signed in to change notification settings - Fork 29k
[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.
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
|
Can one of the admins verify this patch? |
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.
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.
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.
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.
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.
BTW, maybe this is a 'yarn-client' mode-only problem.
I have not tested the 'yarn-cluster' mode.
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.
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?
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 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?)
|
ok to test |
|
@zeodtr Can you update the title to make it something more descriptive? |
|
QA tests have started for PR 899 at commit
|
|
QA tests have finished for PR 899 at commit
|
|
@andrewor14 Updated the title. |
|
Sorry, could you update the title to use this format |
|
@andrewor14 Updated the title. |
|
@zeodtr does this compile with anything < hadoop 2.4? If it doesn't, this is a no-go. |
|
@zeodtr Thanks for updating the title. Just so I understand the issue, for HDP 2.1 on Windows we need these changes for Spark to run, is that correct? However, with this patch other hadoop versions < 2.4 won't even compile, so it seems that we need to do figure out which fields to call during runtime through reflection. When you have a chance can you fix this up so it's compatible across other hadoop versions as well? |
|
Also, I notice that this is opened against branch-1.0. It would be better if you could open it against the master branch so the latest Spark releases will also benefit from your changes. |
|
Can one of the admins verify this patch? |
|
ok to test |
|
QA tests have started for PR 899 at commit
|
|
@andrewor14 @vanzin As I already mentioned in another comment, This won't compile on Hadoop < 2.4. So I suggested another method, but currently I have not enough knowledge to implement the idea. |
|
QA tests have finished for PR 899 at commit
|
|
@zeodtr No worries. We can build on top of your patch to make this work for hadoop versions < 2.4. Thanks for digging through this code. |
|
@andrewor14 Ok, Thanks for fixing! |
|
I didn't fix it, @tsudukim did, so thank him :) |
This is my attempt to fix https://issues.apache.org/jira/browse/SPARK-1825.
Tested on Windows 7 and Hortonworks HDP 2.1 Sandbox, and it works.
Two more problems reported in SPARK-1825(SPARK_HOME, %HADOOP_MAPRED_HOME%) were gone, perhaps by other commits that took after rc5.
WARNING:
But this fix is Hadoop 2.4.0+ only, since it uses new APIs introduced by https://issues.apache.org/jira/browse/YARN-1824.
So, version checking may be needed, but my knowledge for the Spark source code is limited, so I don't know how to do it.