You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by vanzin <gi...@git.apache.org> on 2015/04/14 04:52:16 UTC
[GitHub] spark pull request: [SPARK-6890] [core] Fix launcher lib work with...
GitHub user vanzin opened a pull request:
https://github.com/apache/spark/pull/5504
[SPARK-6890] [core] Fix launcher lib work with SPARK_PREPEND_CLASSES.
The fix for SPARK-6406 broke the case where sub-processes are launched
when SPARK_PREPEND_CLASSES is set, because the code now would only add
the launcher's build directory to the sub-process's classpath instead
of the complete assembly.
This patch fixes the problem by having the launch scripts stash the
assembly's location in an environment variable. This is not the prettiest
solution, but it avoids having to plumb that location all the way through
the Worker code that launches executors. The env variable is always
set by the launch scripts, so users cannot override it.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/vanzin/spark SPARK-6890
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/spark/pull/5504.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 #5504
----
commit 31d3ce857c3e8b481706770a88efc222a6655f5d
Author: Marcelo Vanzin <va...@cloudera.com>
Date: 2015-04-14T02:47:32Z
[SPARK-6890] [core] Fix launcher lib work with SPARK_PREPEND_CLASSES.
The fix for SPARK-6406 broke the case where sub-processes are launched
when SPARK_PREPEND_CLASSES is set, because the code now would only add
the launcher's build directory to the sub-process's classpath instead
of the complete assembly.
This patch fixes the problem by having the launch scripts stash the
assembly's location in an environment variable. This is not the prettiest
solution, but it avoids having to plumb that location all the way through
the Worker code that launches executors. The env variable is always
set by the launch scripts, so users cannot override it.
----
---
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-6890] [core] Fix launcher lib work with...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/5504#issuecomment-93088266
[Test build #30281 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/30281/consoleFull) for PR 5504 at commit [`7aec921`](https://github.com/apache/spark/commit/7aec9216d770d4e7b5ea8e2b67741f0e06ddf723).
---
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-6890] [core] Fix launcher lib work with...
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on the pull request:
https://github.com/apache/spark/pull/5504#issuecomment-93062464
All tests fail with an NPE in lines that are calling `SparkEnv.get`... shouldn't be caused by this change, but well, I'll take a look anyway.
---
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-6890] [core] Fix launcher lib work with...
Posted by andrewor14 <gi...@git.apache.org>.
Github user andrewor14 commented on the pull request:
https://github.com/apache/spark/pull/5504#issuecomment-93041199
This works locally. I wonder why it's failing random MLlib tests.
---
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-6890] [core] Fix launcher lib work with...
Posted by andrewor14 <gi...@git.apache.org>.
Github user andrewor14 commented on a diff in the pull request:
https://github.com/apache/spark/pull/5504#discussion_r28530167
--- Diff: launcher/src/main/java/org/apache/spark/launcher/AbstractCommandBuilder.java ---
@@ -186,12 +186,24 @@ void addOptionString(List<String> cmd, String options) {
addToClassPath(cp, String.format("%s/core/target/jars/*", sparkHome));
}
- final String assembly = AbstractCommandBuilder.class.getProtectionDomain().getCodeSource().
- getLocation().getPath();
+ // We can't rely on the ENV_SPARK_ASSEMBLY variable to be set. Certain situations, such as
+ // when running unit tests, or user code that embeds Spark and creates a SparkContext
+ // with a local or local-cluster master, will cause this code to be called from an
+ // environment where that env variable is not guaranteed to exist.
+ //
+ // For the testing case, we rely on the test code to set and propagate the test classpath
+ // appropriately.
+ //
+ // For the user code case, we fall back to looking for the Spark assembly under SPARK_HOME.
+ // That duplicates some of the code in the shell scripts that look for the assembly, though.
+ String assembly = getenv(ENV_SPARK_ASSEMBLY);
+ if (assembly == null && isEmpty(getenv("SPARK_TESTING"))) {
+ assembly = findAssembly();
+ }
--- End diff --
we can always push a hot fix
---
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-6890] [core] Fix launcher lib work with...
Posted by andrewor14 <gi...@git.apache.org>.
Github user andrewor14 commented on the pull request:
https://github.com/apache/spark/pull/5504#issuecomment-93024572
The changes LGTM. I'm testing this out locally.
---
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-6890] [core] Fix launcher lib work with...
Posted by nishkamravi2 <gi...@git.apache.org>.
Github user nishkamravi2 commented on the pull request:
https://github.com/apache/spark/pull/5504#issuecomment-93608990
@vanzin I'm ok with the introduction of an spark assembly env var but it does seem unnecessary. The logic could have been as simple as:
if (SPARK_PREPEND_CLASSES)
find_assembly()
else
getLocation().getPath()
---
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-6890] [core] Fix launcher lib work with...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/5504#issuecomment-92589474
Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/30214/
Test FAILed.
---
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-6890] [core] Fix launcher lib work with...
Posted by andrewor14 <gi...@git.apache.org>.
Github user andrewor14 commented on the pull request:
https://github.com/apache/spark/pull/5504#issuecomment-93144025
Alright, I tested the latest changes locally again and can verify that this patch does fix the problem. Now that it's also passing tests I will merge this into master. Thanks for fixing this so promptly @vanzin.
---
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-6890] [core] Fix launcher lib work with...
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on the pull request:
https://github.com/apache/spark/pull/5504#issuecomment-93068968
Actually, it might be caused by this change.
---
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-6890] [core] Fix launcher lib work with...
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on the pull request:
https://github.com/apache/spark/pull/5504#issuecomment-93796383
The reason is explained in the code comments and in my comments above. Please don't fix what doesn't need 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: [SPARK-6890] [core] Fix launcher lib work with...
Posted by nishkamravi2 <gi...@git.apache.org>.
Github user nishkamravi2 commented on the pull request:
https://github.com/apache/spark/pull/5504#issuecomment-93795238
Maintaining a non-null value for spark_assembly is a good thing unless there is a good reason to not do so.
---
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-6890] [core] Fix launcher lib work with...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/5504#issuecomment-93044431
[Test build #30273 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/30273/consoleFull) for PR 5504 at commit [`ff87a60`](https://github.com/apache/spark/commit/ff87a60c2d00ab9e00a44d5f656f905401f47d69).
---
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-6890] [core] Fix launcher lib work with...
Posted by nishkamravi2 <gi...@git.apache.org>.
Github user nishkamravi2 commented on a diff in the pull request:
https://github.com/apache/spark/pull/5504#discussion_r28536990
--- Diff: launcher/src/main/java/org/apache/spark/launcher/AbstractCommandBuilder.java ---
@@ -186,12 +186,24 @@ void addOptionString(List<String> cmd, String options) {
addToClassPath(cp, String.format("%s/core/target/jars/*", sparkHome));
}
- final String assembly = AbstractCommandBuilder.class.getProtectionDomain().getCodeSource().
- getLocation().getPath();
+ // We can't rely on the ENV_SPARK_ASSEMBLY variable to be set. Certain situations, such as
+ // when running unit tests, or user code that embeds Spark and creates a SparkContext
+ // with a local or local-cluster master, will cause this code to be called from an
+ // environment where that env variable is not guaranteed to exist.
+ //
+ // For the testing case, we rely on the test code to set and propagate the test classpath
+ // appropriately.
+ //
+ // For the user code case, we fall back to looking for the Spark assembly under SPARK_HOME.
+ // That duplicates some of the code in the shell scripts that look for the assembly, though.
+ String assembly = getenv(ENV_SPARK_ASSEMBLY);
+ if (assembly == null && isEmpty(getenv("SPARK_TESTING"))) {
+ assembly = findAssembly();
+ }
--- End diff --
I don't think this check 'ensures' that those tests work, this check 'requires' that those tests work if the assembly is not there (more like an assert). I don't feel strongly for or against it, but it does seem unnecessary.
---
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-6890] [core] Fix launcher lib work with...
Posted by nishkamravi2 <gi...@git.apache.org>.
Github user nishkamravi2 commented on a diff in the pull request:
https://github.com/apache/spark/pull/5504#discussion_r28488106
--- Diff: launcher/src/main/java/org/apache/spark/launcher/AbstractCommandBuilder.java ---
@@ -186,12 +186,24 @@ void addOptionString(List<String> cmd, String options) {
addToClassPath(cp, String.format("%s/core/target/jars/*", sparkHome));
}
- final String assembly = AbstractCommandBuilder.class.getProtectionDomain().getCodeSource().
- getLocation().getPath();
+ // We can't rely on the ENV_SPARK_ASSEMBLY variable to be set. Certain situations, such as
+ // when running unit tests, or user code that embeds Spark and creates a SparkContext
+ // with a local or local-cluster master, will cause this code to be called from an
+ // environment where that env variable is not guaranteed to exist.
+ //
+ // For the testing case, we rely on the test code to set and propagate the test classpath
+ // appropriately.
+ //
+ // For the user code case, we fall back to looking for the Spark assembly under SPARK_HOME.
+ // That duplicates some of the code in the shell scripts that look for the assembly, though.
+ String assembly = getenv(ENV_SPARK_ASSEMBLY);
+ if (assembly == null && isEmpty(getenv("SPARK_TESTING"))) {
+ assembly = findAssembly();
+ }
--- End diff --
if (assembly == null) findAssembly() ?
---
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-6890] [core] Fix launcher lib work with...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on the pull request:
https://github.com/apache/spark/pull/5504#issuecomment-93793071
I think @vanzin is saying the logic is correct? at least the intent is to not bother searching for the assembly when testing. Do you mean you think that has to change?
---
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-6890] [core] Fix launcher lib work with...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/5504#issuecomment-93054274
Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/30273/
Test FAILed.
---
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-6890] [core] Fix launcher lib work with...
Posted by andrewor14 <gi...@git.apache.org>.
Github user andrewor14 commented on the pull request:
https://github.com/apache/spark/pull/5504#issuecomment-93043848
retest 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-6890] [core] Fix launcher lib work with...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/5504#issuecomment-93054262
[Test build #30273 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/30273/consoleFull) for PR 5504 at commit [`ff87a60`](https://github.com/apache/spark/commit/ff87a60c2d00ab9e00a44d5f656f905401f47d69).
* This patch **fails Spark unit tests**.
* This patch merges cleanly.
* This patch adds no public classes.
* This patch does not change any dependencies.
---
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-6890] [core] Fix launcher lib work with...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/spark/pull/5504
---
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-6890] [core] Fix launcher lib work with...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/5504#issuecomment-93040918
Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/30271/
Test FAILed.
---
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-6890] [core] Fix launcher lib work with...
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on a diff in the pull request:
https://github.com/apache/spark/pull/5504#discussion_r28524856
--- Diff: launcher/src/main/java/org/apache/spark/launcher/AbstractCommandBuilder.java ---
@@ -186,12 +186,24 @@ void addOptionString(List<String> cmd, String options) {
addToClassPath(cp, String.format("%s/core/target/jars/*", sparkHome));
}
- final String assembly = AbstractCommandBuilder.class.getProtectionDomain().getCodeSource().
- getLocation().getPath();
+ // We can't rely on the ENV_SPARK_ASSEMBLY variable to be set. Certain situations, such as
+ // when running unit tests, or user code that embeds Spark and creates a SparkContext
+ // with a local or local-cluster master, will cause this code to be called from an
+ // environment where that env variable is not guaranteed to exist.
+ //
+ // For the testing case, we rely on the test code to set and propagate the test classpath
+ // appropriately.
+ //
+ // For the user code case, we fall back to looking for the Spark assembly under SPARK_HOME.
+ // That duplicates some of the code in the shell scripts that look for the assembly, though.
+ String assembly = getenv(ENV_SPARK_ASSEMBLY);
+ if (assembly == null && isEmpty(getenv("SPARK_TESTING"))) {
+ assembly = findAssembly();
+ }
--- End diff --
This code has already been pushed.
---
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-6890] [core] Fix launcher lib work with...
Posted by nishkamravi2 <gi...@git.apache.org>.
Github user nishkamravi2 commented on the pull request:
https://github.com/apache/spark/pull/5504#issuecomment-93792328
Will submit a hot fix in a bit
---
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-6890] [core] Fix launcher lib work with...
Posted by andrewor14 <gi...@git.apache.org>.
Github user andrewor14 commented on the pull request:
https://github.com/apache/spark/pull/5504#issuecomment-93023005
Test output was weird. Jenkins retest 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-6890] [core] Fix launcher lib work with...
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on a diff in the pull request:
https://github.com/apache/spark/pull/5504#discussion_r28483980
--- Diff: launcher/src/main/java/org/apache/spark/launcher/AbstractCommandBuilder.java ---
@@ -186,12 +186,24 @@ void addOptionString(List<String> cmd, String options) {
addToClassPath(cp, String.format("%s/core/target/jars/*", sparkHome));
}
- final String assembly = AbstractCommandBuilder.class.getProtectionDomain().getCodeSource().
- getLocation().getPath();
+ // We can't rely on the ENV_SPARK_ASSEMBLY variable to be set. Certain situations, such as
+ // when running unit tests, or user code that embeds Spark and creates a SparkContext
+ // with a local or local-cluster master, will cause this code to be called from an
+ // environment where that env variable is not guaranteed to exist.
+ //
+ // For the testing case, we rely on the test code to set and propagate the test classpath
+ // appropriately.
+ //
+ // For the user code case, we fall back to looking for the Spark assembly under SPARK_HOME.
+ // That duplicates some of the code in the shell scripts that look for the assembly, though.
+ String assembly = getenv(ENV_SPARK_ASSEMBLY);
+ if (assembly == null && isEmpty(getenv("SPARK_TESTING"))) {
+ assembly = findAssembly();
+ }
--- End diff --
This has already been pushed, but it's explained in the big comment right before the 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: [SPARK-6890] [core] Fix launcher lib work with...
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on the pull request:
https://github.com/apache/spark/pull/5504#issuecomment-93055047
Let me see if I can reproduce it locally.
---
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-6890] [core] Fix launcher lib work with...
Posted by andrewor14 <gi...@git.apache.org>.
Github user andrewor14 commented on the pull request:
https://github.com/apache/spark/pull/5504#issuecomment-93054875
The test results are really flaky, and I don't believe these tests are failing outside of this PR. For this reason I don't think we should merge this patch as is yet, but I haven't dug into why the MLlib tests are failing.
---
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-6890] [core] Fix launcher lib work with...
Posted by nishkamravi2 <gi...@git.apache.org>.
Github user nishkamravi2 commented on a diff in the pull request:
https://github.com/apache/spark/pull/5504#discussion_r28477271
--- Diff: launcher/src/main/java/org/apache/spark/launcher/AbstractCommandBuilder.java ---
@@ -186,12 +186,24 @@ void addOptionString(List<String> cmd, String options) {
addToClassPath(cp, String.format("%s/core/target/jars/*", sparkHome));
}
- final String assembly = AbstractCommandBuilder.class.getProtectionDomain().getCodeSource().
- getLocation().getPath();
+ // We can't rely on the ENV_SPARK_ASSEMBLY variable to be set. Certain situations, such as
+ // when running unit tests, or user code that embeds Spark and creates a SparkContext
+ // with a local or local-cluster master, will cause this code to be called from an
+ // environment where that env variable is not guaranteed to exist.
+ //
+ // For the testing case, we rely on the test code to set and propagate the test classpath
+ // appropriately.
+ //
+ // For the user code case, we fall back to looking for the Spark assembly under SPARK_HOME.
+ // That duplicates some of the code in the shell scripts that look for the assembly, though.
+ String assembly = getenv(ENV_SPARK_ASSEMBLY);
+ if (assembly == null && isEmpty(getenv("SPARK_TESTING"))) {
+ assembly = findAssembly();
+ }
--- End diff --
Why do we need "&& isEmpty(getenv("SPARK_TESTING")" ?
---
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-6890] [core] Fix launcher lib work with...
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on the pull request:
https://github.com/apache/spark/pull/5504#issuecomment-92570963
I tested on Linux with and without SPARK_PREPEND_CLASSES. I'll try it on Windows tomorrow just to make sure I didn't break anything there.
/cc @davies @andrewor14
---
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-6890] [core] Fix launcher lib work with...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/5504#issuecomment-93024033
[Test build #30271 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/30271/consoleFull) for PR 5504 at commit [`ff87a60`](https://github.com/apache/spark/commit/ff87a60c2d00ab9e00a44d5f656f905401f47d69).
---
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-6890] [core] Fix launcher lib work with...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/5504#issuecomment-93118603
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/30281/
Test PASSed.
---
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-6890] [core] Fix launcher lib work with...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/5504#issuecomment-92995093
[Test build #30258 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/30258/consoleFull) for PR 5504 at commit [`ff87a60`](https://github.com/apache/spark/commit/ff87a60c2d00ab9e00a44d5f656f905401f47d69).
* This patch **fails MiMa tests**.
* This patch merges cleanly.
* This patch adds no public classes.
* This patch does not change any dependencies.
---
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-6890] [core] Fix launcher lib work with...
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on the pull request:
https://github.com/apache/spark/pull/5504#issuecomment-93087355
The latest change should fix the tests. I realize that now there's some duplication, and that because I had to restore the Java code to look for the assembly (see comment in patch), we don't necessarily need to expose it in an env variable. But being able to control the assembly's location from the shell scripts was the whole point of #5085, so I guess if we want to support that use case, the env variable is still needed.
---
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-6890] [core] Fix launcher lib work with...
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on a diff in the pull request:
https://github.com/apache/spark/pull/5504#discussion_r28531865
--- Diff: launcher/src/main/java/org/apache/spark/launcher/AbstractCommandBuilder.java ---
@@ -186,12 +186,24 @@ void addOptionString(List<String> cmd, String options) {
addToClassPath(cp, String.format("%s/core/target/jars/*", sparkHome));
}
- final String assembly = AbstractCommandBuilder.class.getProtectionDomain().getCodeSource().
- getLocation().getPath();
+ // We can't rely on the ENV_SPARK_ASSEMBLY variable to be set. Certain situations, such as
+ // when running unit tests, or user code that embeds Spark and creates a SparkContext
+ // with a local or local-cluster master, will cause this code to be called from an
+ // environment where that env variable is not guaranteed to exist.
+ //
+ // For the testing case, we rely on the test code to set and propagate the test classpath
+ // appropriately.
+ //
+ // For the user code case, we fall back to looking for the Spark assembly under SPARK_HOME.
+ // That duplicates some of the code in the shell scripts that look for the assembly, though.
+ String assembly = getenv(ENV_SPARK_ASSEMBLY);
+ if (assembly == null && isEmpty(getenv("SPARK_TESTING"))) {
+ assembly = findAssembly();
+ }
--- End diff --
But there is nothing to fix.
We don't want to look for the assembly when tests are running because it may not exist. Tests do a lot of "new SparkContext()" with master = `local-cluster[blah]`, and this check ensures those tests work even if the assembly is not there.
---
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-6890] [core] Fix launcher lib work with...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/5504#issuecomment-92572834
[Test build #30214 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/30214/consoleFull) for PR 5504 at commit [`31d3ce8`](https://github.com/apache/spark/commit/31d3ce857c3e8b481706770a88efc222a6655f5d).
---
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-6890] [core] Fix launcher lib work with...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/5504#issuecomment-93040899
[Test build #30271 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/30271/consoleFull) for PR 5504 at commit [`ff87a60`](https://github.com/apache/spark/commit/ff87a60c2d00ab9e00a44d5f656f905401f47d69).
* This patch **fails Spark unit tests**.
* This patch merges cleanly.
* This patch adds no public classes.
* This patch does not change any dependencies.
---
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-6890] [core] Fix launcher lib work with...
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on the pull request:
https://github.com/apache/spark/pull/5504#issuecomment-92990862
Update: tested on Windows too, looks fine.
---
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-6890] [core] Fix launcher lib work with...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/5504#issuecomment-93118587
[Test build #30281 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/30281/consoleFull) for PR 5504 at commit [`7aec921`](https://github.com/apache/spark/commit/7aec9216d770d4e7b5ea8e2b67741f0e06ddf723).
* This patch **passes all tests**.
* This patch merges cleanly.
* This patch adds no public classes.
* This patch **adds the following new dependencies:**
* `snappy-java-1.1.1.7.jar`
* This patch **removes the following dependencies:**
* `snappy-java-1.1.1.6.jar`
---
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-6890] [core] Fix launcher lib work with...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/5504#issuecomment-92991930
[Test build #30258 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/30258/consoleFull) for PR 5504 at commit [`ff87a60`](https://github.com/apache/spark/commit/ff87a60c2d00ab9e00a44d5f656f905401f47d69).
---
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-6890] [core] Fix launcher lib work with...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/5504#issuecomment-92589472
[Test build #30214 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/30214/consoleFull) for PR 5504 at commit [`31d3ce8`](https://github.com/apache/spark/commit/31d3ce857c3e8b481706770a88efc222a6655f5d).
* This patch **fails Spark unit tests**.
* This patch merges cleanly.
* This patch adds the following public classes _(experimental)_:
* `case class MinOf(left: Expression, right: Expression) extends Expression `
* This patch does not change any dependencies.
---
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-6890] [core] Fix launcher lib work with...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/5504#issuecomment-92995114
Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/30258/
Test FAILed.
---
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