You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by zeodtr <gi...@git.apache.org> on 2014/05/28 06:56:39 UTC
[GitHub] spark pull request: fixes https://issues.apache.org/jira/browse/SP...
GitHub user zeodtr opened a pull request:
https://github.com/apache/spark/pull/899
fixes https://issues.apache.org/jira/browse/SPARK-1825
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.
Tow more problems reported in SPARK-18259(SPARK_HOME, %HADOOP_MAPRED_HOME%) were gone, perhaps by other commits that took after rc5.
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.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/zeodtr/spark jira1825
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/spark/pull/899.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 #899
----
commit b768fc652e74e402c480b35efacb36c71c7e2f7c
Author: zeodtr <ze...@naver.com>
Date: 2014-05-28T04:30:04Z
fixes https://issues.apache.org/jira/browse/SPARK-1825
----
---
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-1825] Fixes cross-platform submit probl...
Posted by andrewor14 <gi...@git.apache.org>.
Github user andrewor14 commented on the pull request:
https://github.com/apache/spark/pull/899#issuecomment-73424410
I didn't fix it, @tsudukim did, so thank him :)
---
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.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: fixes https://issues.apache.org/jira/browse/SP...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on a diff in the pull request:
https://github.com/apache/spark/pull/899#discussion_r13127330
--- Diff: yarn/common/src/main/scala/org/apache/spark/deploy/yarn/ClientBase.scala ---
@@ -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)
--- End diff --
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?
---
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-1825] Fixes cross-platform submit probl...
Posted by zeodtr <gi...@git.apache.org>.
Github user zeodtr commented on the pull request:
https://github.com/apache/spark/pull/899#issuecomment-54700088
@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.
Sorry if this PR confuses you.
---
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.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-1825] Fixes cross-platform submit probl...
Posted by zeodtr <gi...@git.apache.org>.
Github user zeodtr commented on the pull request:
https://github.com/apache/spark/pull/899#issuecomment-54565600
@andrewor14 Updated the title.
---
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.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-1825] Fixes cross-platform submit probl...
Posted by andrewor14 <gi...@git.apache.org>.
Github user andrewor14 commented on a diff in the pull request:
https://github.com/apache/spark/pull/899#discussion_r17195370
--- Diff: yarn/common/src/main/scala/org/apache/spark/deploy/yarn/ClientBase.scala ---
@@ -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)
--- End diff --
It seems the only way to special case this is through reflection. Otherwise it won't even compile for < 2.4. I'm not sure if we need a config here; it seems we can just try to load the `$$` method, and if it's not there we fallback to `$`. Same goes for `ApplicationConstants.CLASS_PATH_SEPARATOR`
---
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.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: fixes crossplatform submit problem (see https:...
Posted by zeodtr <gi...@git.apache.org>.
Github user zeodtr commented on the pull request:
https://github.com/apache/spark/pull/899#issuecomment-54399087
@andrewor14 Updated the title.
---
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.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: fixes https://issues.apache.org/jira/browse/SP...
Posted by zeodtr <gi...@git.apache.org>.
Github user zeodtr commented on a diff in the pull request:
https://github.com/apache/spark/pull/899#discussion_r13126157
--- Diff: yarn/common/src/main/scala/org/apache/spark/deploy/yarn/ClientBase.scala ---
@@ -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)
--- End diff --
BTW, maybe this is a 'yarn-client' mode-only case.
I have not tested the 'yarn-cluster' mode.
---
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: fixes https://issues.apache.org/jira/browse/SP...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/899#issuecomment-54352517
[QA tests have started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/19680/consoleFull) for PR 899 at commit [`b768fc6`](https://github.com/apache/spark/commit/b768fc652e74e402c480b35efacb36c71c7e2f7c).
* This patch merges cleanly.
---
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.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: fixes crossplatform submit problem (see https:...
Posted by zeodtr <gi...@git.apache.org>.
Github user zeodtr commented on a diff in the pull request:
https://github.com/apache/spark/pull/899#discussion_r17093941
--- Diff: yarn/common/src/main/scala/org/apache/spark/deploy/yarn/ClientBase.scala ---
@@ -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)
--- End diff --
@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.
---
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.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: fixes https://issues.apache.org/jira/browse/SP...
Posted by zeodtr <gi...@git.apache.org>.
Github user zeodtr commented on a diff in the pull request:
https://github.com/apache/spark/pull/899#discussion_r13123900
--- Diff: yarn/common/src/main/scala/org/apache/spark/deploy/yarn/ClientBase.scala ---
@@ -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)
--- End diff --
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.
---
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: fixes https://issues.apache.org/jira/browse/SP...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on a diff in the pull request:
https://github.com/apache/spark/pull/899#discussion_r13127349
--- Diff: yarn/common/src/main/scala/org/apache/spark/deploy/yarn/ClientBase.scala ---
@@ -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)
--- End diff --
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.
---
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-1825] Fixes cross-platform submit probl...
Posted by andrewor14 <gi...@git.apache.org>.
Github user andrewor14 commented on the pull request:
https://github.com/apache/spark/pull/899#issuecomment-54697571
ok to test
---
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.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-1825] Fixes cross-platform submit probl...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/899#issuecomment-54698949
[QA tests have started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/19884/consoleFull) for PR 899 at commit [`b768fc6`](https://github.com/apache/spark/commit/b768fc652e74e402c480b35efacb36c71c7e2f7c).
* This patch merges cleanly.
---
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.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: fixes https://issues.apache.org/jira/browse/SP...
Posted by andrewor14 <gi...@git.apache.org>.
Github user andrewor14 commented on the pull request:
https://github.com/apache/spark/pull/899#issuecomment-53980987
test this please
---
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.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-1825] Fixes cross-platform submit probl...
Posted by zeodtr <gi...@git.apache.org>.
Github user zeodtr commented on the pull request:
https://github.com/apache/spark/pull/899#issuecomment-73406987
@andrewor14 Ok, Thanks for fixing!
---
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.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: fixes https://issues.apache.org/jira/browse/SP...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on a diff in the pull request:
https://github.com/apache/spark/pull/899#discussion_r13121954
--- Diff: yarn/common/src/main/scala/org/apache/spark/deploy/yarn/ClientBase.scala ---
@@ -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)
--- End diff --
I'm just curious about what the $ vs $$ change does here.
---
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: fixes https://issues.apache.org/jira/browse/SP...
Posted by andrewor14 <gi...@git.apache.org>.
Github user andrewor14 commented on the pull request:
https://github.com/apache/spark/pull/899#issuecomment-54351766
ok to test
---
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.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: fixes crossplatform submit problem (see https:...
Posted by andrewor14 <gi...@git.apache.org>.
Github user andrewor14 commented on the pull request:
https://github.com/apache/spark/pull/899#issuecomment-54406446
Sorry, could you update it to use this format `[SPARK-1825] Fixes cross platform...`? We use a tool that parses the JIRA number to link Spark PRs by the ticket number. 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.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-1825] Fixes cross-platform submit probl...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/899#issuecomment-54700786
[QA tests have finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/19884/consoleFull) for PR 899 at commit [`b768fc6`](https://github.com/apache/spark/commit/b768fc652e74e402c480b35efacb36c71c7e2f7c).
* This patch **passes** unit tests.
* This patch merges cleanly.
* This patch adds no public classes.
---
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.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-1825] Fixes cross-platform submit probl...
Posted by andrewor14 <gi...@git.apache.org>.
Github user andrewor14 commented on the pull request:
https://github.com/apache/spark/pull/899#issuecomment-54676819
@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?
---
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.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-1825] Fixes cross-platform submit probl...
Posted by andrewor14 <gi...@git.apache.org>.
Github user andrewor14 commented on the pull request:
https://github.com/apache/spark/pull/899#issuecomment-73301694
Hey @zeodtr I believe this is fixed in #3924 would you mind closing 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.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: fixes https://issues.apache.org/jira/browse/SP...
Posted by zeodtr <gi...@git.apache.org>.
Github user zeodtr commented on a diff in the pull request:
https://github.com/apache/spark/pull/899#discussion_r13124009
--- Diff: yarn/common/src/main/scala/org/apache/spark/deploy/yarn/ClientBase.scala ---
@@ -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)
--- End diff --
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.
---
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: fixes https://issues.apache.org/jira/browse/SP...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on a diff in the pull request:
https://github.com/apache/spark/pull/899#discussion_r13121927
--- Diff: yarn/common/src/main/scala/org/apache/spark/deploy/yarn/ClientBase.scala ---
@@ -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)
--- End diff --
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.
---
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: fixes https://issues.apache.org/jira/browse/SP...
Posted by andrewor14 <gi...@git.apache.org>.
Github user andrewor14 commented on the pull request:
https://github.com/apache/spark/pull/899#issuecomment-54351795
@zeodtr Can you update the title to make it something more descriptive?
---
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.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: fixes https://issues.apache.org/jira/browse/SP...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/899#issuecomment-54365245
[QA tests have finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/19680/consoleFull) for PR 899 at commit [`b768fc6`](https://github.com/apache/spark/commit/b768fc652e74e402c480b35efacb36c71c7e2f7c).
* This patch **passes** unit tests.
* This patch merges cleanly.
* This patch adds no public classes.
---
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.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-1825] Fixes cross-platform submit probl...
Posted by andrewor14 <gi...@git.apache.org>.
Github user andrewor14 commented on the pull request:
https://github.com/apache/spark/pull/899#issuecomment-54723174
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.
---
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.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: fixes https://issues.apache.org/jira/browse/SP...
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on a diff in the pull request:
https://github.com/apache/spark/pull/899#discussion_r13418342
--- Diff: yarn/common/src/main/scala/org/apache/spark/deploy/yarn/ClientBase.scala ---
@@ -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)
--- End diff --
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.
---
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-1825] Fixes cross-platform submit probl...
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on the pull request:
https://github.com/apache/spark/pull/899#issuecomment-54652280
@zeodtr does this compile with anything < hadoop 2.4? If it doesn't, this is a no-go.
---
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.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-1825] Fixes cross-platform submit probl...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/899#issuecomment-54694693
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.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-1825] Fixes cross-platform submit probl...
Posted by zeodtr <gi...@git.apache.org>.
Github user zeodtr closed the pull request at:
https://github.com/apache/spark/pull/899
---
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.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: fixes https://issues.apache.org/jira/browse/SP...
Posted by zeodtr <gi...@git.apache.org>.
Github user zeodtr commented on a diff in the pull request:
https://github.com/apache/spark/pull/899#discussion_r13164860
--- Diff: yarn/common/src/main/scala/org/apache/spark/deploy/yarn/ClientBase.scala ---
@@ -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)
--- End diff --
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](https://github.com/apache/hadoop-common/blob/9f02e3a4a70d4affd5342e8fa61666bd8bf59bbb/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager/src/main/java/org/apache/hadoop/yarn/server/nodemanager/containermanager/launcher/ContainerLaunch.java))
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?)
---
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: fixes https://issues.apache.org/jira/browse/SP...
Posted by zeodtr <gi...@git.apache.org>.
Github user zeodtr commented on a diff in the pull request:
https://github.com/apache/spark/pull/899#discussion_r15206281
--- Diff: yarn/common/src/main/scala/org/apache/spark/deploy/yarn/ClientBase.scala ---
@@ -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)
--- End diff --
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 separator, without using reflection.
(But currently I my knowledge on Spark source code and Scala is limited to implement it myself.)
---
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-1825] Fixes cross-platform submit probl...
Posted by andrewor14 <gi...@git.apache.org>.
Github user andrewor14 commented on the pull request:
https://github.com/apache/spark/pull/899#issuecomment-54676894
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.
---
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.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-1825] Fixes cross-platform submit probl...
Posted by zeodtr <gi...@git.apache.org>.
Github user zeodtr commented on a diff in the pull request:
https://github.com/apache/spark/pull/899#discussion_r17338037
--- Diff: yarn/common/src/main/scala/org/apache/spark/deploy/yarn/ClientBase.scala ---
@@ -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)
--- End diff --
@andrewor14 What I meant for hard-coding is to use string value "{{" and "}}" directly without event calling $$() in the Spark code when Hadoop version is >= 2.4 (or config is set). But I agree that the reflection is cleaner solution.
---
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.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: fixes https://issues.apache.org/jira/browse/SP...
Posted by zeodtr <gi...@git.apache.org>.
Github user zeodtr commented on a diff in the pull request:
https://github.com/apache/spark/pull/899#discussion_r13164961
--- Diff: yarn/common/src/main/scala/org/apache/spark/deploy/yarn/ClientBase.scala ---
@@ -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)
--- End diff --
It's the same as `<CPS>`. Hadoop < 2.4 will not recognize it.
BTW, CLASS_PATH_SEPARATOR and $$() are annotated with `@Unstable`.
---
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: fixes https://issues.apache.org/jira/browse/SP...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/899#issuecomment-44365780
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: fixes https://issues.apache.org/jira/browse/SP...
Posted by tgravescs <gi...@git.apache.org>.
Github user tgravescs commented on a diff in the pull request:
https://github.com/apache/spark/pull/899#discussion_r17049948
--- Diff: yarn/common/src/main/scala/org/apache/spark/deploy/yarn/ClientBase.scala ---
@@ -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)
--- End diff --
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.
---
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.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org