You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by andreweduffy <gi...@git.apache.org> on 2016/07/14 13:17:42 UTC
[GitHub] spark pull request #14201: [SPARK-14702] Expose SparkLauncher's ProcessBuild...
GitHub user andreweduffy opened a pull request:
https://github.com/apache/spark/pull/14201
[SPARK-14702] Expose SparkLauncher's ProcessBuilder for user flexibility
## What changes were proposed in this pull request?
Adds a few public methods to `SparkLauncher` to allow configuring some extra features of the `ProcessBuilder`, including the working directory, output and error stream redirection.
## How was this patch tested?
Unit testing + simple Spark driver programs
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/andreweduffy/spark feature/launcher
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/spark/pull/14201.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 #14201
----
commit 5a35963bc4661193d8c169c7f98fffe24b181124
Author: Andrew Duffy <ad...@palantir.com>
Date: 2016-07-14T11:57:15Z
Make environment of SparkLauncher launched process more configurable
----
---
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 issue #14201: [SPARK-14702] Make environment of SparkLauncher launched...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/14201
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/62392/
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 issue #14201: [SPARK-14702] Make environment of SparkLauncher launched...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/14201
**[Test build #62481 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/62481/consoleFull)** for PR 14201 at commit [`bde3fa7`](https://github.com/apache/spark/commit/bde3fa72af5117398fe561f54e8dac2c8c996176).
---
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 issue #14201: [SPARK-14702] Expose SparkLauncher's ProcessBuilder for ...
Posted by andreweduffy <gi...@git.apache.org>.
Github user andreweduffy commented on the issue:
https://github.com/apache/spark/pull/14201
Replaces #14185, @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 issue #14201: [SPARK-14702] Expose SparkLauncher's ProcessBuilder for ...
Posted by andreweduffy <gi...@git.apache.org>.
Github user andreweduffy commented on the issue:
https://github.com/apache/spark/pull/14201
I'll go ahead and replace use of Throwables with throw new RTE, not clear why it can't find Guava on the classpath however
---
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 issue #14201: [SPARK-14702] Make environment of SparkLauncher launched...
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on the issue:
https://github.com/apache/spark/pull/14201
LGTM pending a few remaining style issues.
@andreweduffy our merge scripts squash PRs into a single commit, so history stays clean regardless of how you update the 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 #14201: [SPARK-14702] Make environment of SparkLauncher l...
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on a diff in the pull request:
https://github.com/apache/spark/pull/14201#discussion_r71012517
--- Diff: launcher/src/main/java/org/apache/spark/launcher/SparkLauncher.java ---
@@ -359,6 +369,83 @@ public SparkLauncher setVerbose(boolean verbose) {
}
/**
+ * Sets the working directory of spark-submit.
+ *
+ * @param dir The directory to set as spark-submit's working directory.
+ * @return This launcher.
+ */
+ public SparkLauncher directory(File dir) {
+ workingDir = dir;
+ return this;
+ }
+
+ /**
+ * Specifies that stderr in spark-submit should be redirected to stdout.
+ *
+ * @return This launcher.
+ */
+ public SparkLauncher redirectError() {
+ redirectErrorStream = true;
+ return this;
+ }
+
+ /**
+ * Redirects error output to the specified Redirect.
+ *
+ * @param to The method of redirection.
+ * @return This launcher.
+ */
+ public SparkLauncher redirectError(ProcessBuilder.Redirect to) {
+ errorStream = to;
+ return this;
+ }
+
+ /**
+ * Redirects standard output to the specified Redirect.
+ *
+ * @param to The method of redirection.
+ * @return This launcher.
+ */
+ public SparkLauncher redirectOutput(ProcessBuilder.Redirect to) {
+ outputStream = to;
+ return this;
+ }
+
+ /**
+ * Redirects error output to the specified File.
+ *
+ * @param errFile The file to which stderr is written.
+ * @return This launcher.
+ */
+ public SparkLauncher redirectError(File errFile) {
+ errorStream = ProcessBuilder.Redirect.to(errFile);
+ return this;
+ }
+
+ /**
+ * Redirects error output to the specified File.
+ *
+ * @param outFile The file to which stdout is written.
+ * @return This launcher.
+ */
+ public SparkLauncher redirectOutput(File outFile) {
+ outputStream = ProcessBuilder.Redirect.to(outFile);
+ return this;
+ }
+
+ /**
+ * Sets all output to be logged and redirected to a logger with the specified name.
+ *
+ * @param loggerName The name of the logger to log stdout and stderr.
+ * @return This launcher.
+ */
--- End diff --
nit: alignment is off
---
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 issue #14201: [SPARK-14702] Make environment of SparkLauncher launched...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/14201
**[Test build #62376 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/62376/consoleFull)** for PR 14201 at commit [`2357c07`](https://github.com/apache/spark/commit/2357c07de07d1a4eb31bbde2e7851e297987ea4e).
---
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 #14201: [SPARK-14702] Make environment of SparkLauncher l...
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on a diff in the pull request:
https://github.com/apache/spark/pull/14201#discussion_r71049864
--- Diff: core/src/test/java/org/apache/spark/launcher/SparkLauncherSuite.java ---
@@ -40,10 +41,15 @@
private static final Logger LOG = LoggerFactory.getLogger(SparkLauncherSuite.class);
private static final NamedThreadFactory TF = new NamedThreadFactory("SparkLauncherSuite-%d");
+ private static SparkLauncher launcher;
--- End diff --
minor: this doesn't need to be the `static` from what I can see.
---
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 issue #14201: [SPARK-14702] Expose SparkLauncher's ProcessBuilder for ...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/14201
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 #14201: [SPARK-14702] Make environment of SparkLauncher l...
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on a diff in the pull request:
https://github.com/apache/spark/pull/14201#discussion_r71008794
--- Diff: core/src/test/java/org/apache/spark/launcher/SparkLauncherSuite.java ---
@@ -86,13 +93,62 @@ public void testSparkArgumentHandling() throws Exception {
}
@Test
+ public void testRedirectTwiceFails() throws Exception {
+ try {
+ launcher.setAppResource("fake-resource.jar")
+ .setMainClass("my.fake.class.Fake")
--- End diff --
nit: we don't align like this, rather just indent the next line.
---
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 issue #14201: [SPARK-14702] Make environment of SparkLauncher launched...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/14201
Merged build finished. 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 issue #14201: [SPARK-14702] Expose SparkLauncher's ProcessBuilder for ...
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on the issue:
https://github.com/apache/spark/pull/14201
@andreweduffy can you change the title of the PR? The title of your commit (https://github.com/apache/spark/pull/14201/commits/deca998b725eeb7df2f12999c8bbac3c1ffc5161) is a better one.
A few minor things to take care of, but looking good.
---
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 issue #14201: [SPARK-14702] Make environment of SparkLauncher launched...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/14201
Merged build finished. 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 issue #14201: [SPARK-14702] Expose SparkLauncher's ProcessBuilder for ...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/14201
**[Test build #62334 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/62334/consoleFull)** for PR 14201 at commit [`deca998`](https://github.com/apache/spark/commit/deca998b725eeb7df2f12999c8bbac3c1ffc5161).
---
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 issue #14201: [SPARK-14702] Expose SparkLauncher's ProcessBuilder for ...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/14201
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/62334/
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 issue #14201: [SPARK-14702] Expose SparkLauncher's ProcessBuilder for ...
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on the issue:
https://github.com/apache/spark/pull/14201
> not clear why it can't find Guava on the classpath however
This module does not depend on Guava (nor should 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 #14201: [SPARK-14702] Make environment of SparkLauncher l...
Posted by andreweduffy <gi...@git.apache.org>.
Github user andreweduffy commented on a diff in the pull request:
https://github.com/apache/spark/pull/14201#discussion_r71031747
--- Diff: launcher/src/main/java/org/apache/spark/launcher/SparkLauncher.java ---
@@ -359,6 +369,83 @@ public SparkLauncher setVerbose(boolean verbose) {
}
/**
+ * Sets the working directory of spark-submit.
+ *
+ * @param dir The directory to set as spark-submit's working directory.
+ * @return This launcher.
+ */
+ public SparkLauncher directory(File dir) {
+ workingDir = dir;
+ return this;
+ }
+
+ /**
+ * Specifies that stderr in spark-submit should be redirected to stdout.
+ *
+ * @return This launcher.
+ */
+ public SparkLauncher redirectError() {
+ redirectErrorStream = true;
+ return this;
+ }
+
+ /**
+ * Redirects error output to the specified Redirect.
+ *
+ * @param to The method of redirection.
+ * @return This launcher.
+ */
+ public SparkLauncher redirectError(ProcessBuilder.Redirect to) {
+ errorStream = to;
+ return this;
+ }
+
+ /**
+ * Redirects standard output to the specified Redirect.
+ *
+ * @param to The method of redirection.
+ * @return This launcher.
+ */
+ public SparkLauncher redirectOutput(ProcessBuilder.Redirect to) {
+ outputStream = to;
+ return this;
+ }
+
+ /**
+ * Redirects error output to the specified File.
+ *
+ * @param errFile The file to which stderr is written.
+ * @return This launcher.
+ */
+ public SparkLauncher redirectError(File errFile) {
+ errorStream = ProcessBuilder.Redirect.to(errFile);
+ return this;
+ }
+
+ /**
+ * Redirects error output to the specified File.
+ *
+ * @param outFile The file to which stdout is written.
+ * @return This launcher.
+ */
+ public SparkLauncher redirectOutput(File outFile) {
+ outputStream = ProcessBuilder.Redirect.to(outFile);
+ return this;
+ }
+
+ /**
+ * Sets all output to be logged and redirected to a logger with the specified name.
+ *
+ * @param loggerName The name of the logger to log stdout and stderr.
+ * @return This launcher.
+ */
+ public SparkLauncher redirectToLog(String loggerName) {
--- End diff --
So I had an extra check in the last commit, thought there was a comment that it should be removed though, maybe I should just clarify here. I see 2 possible checks that we should be doing here:
* Ensure that only one of `redirectError(...)` and `redirectError()` can be used (done)
* Ensure that `redirectToLog()` is never used with any other `redirect*` method (removed in this diff, can be added back)
Are there others that I'm missing?
---
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 #14201: [SPARK-14702] Make environment of SparkLauncher l...
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on a diff in the pull request:
https://github.com/apache/spark/pull/14201#discussion_r70912587
--- Diff: launcher/src/main/java/org/apache/spark/launcher/SparkLauncher.java ---
@@ -359,6 +364,82 @@ public SparkLauncher setVerbose(boolean verbose) {
}
/**
+ * Sets the working directory of the driver process.
+ * @param dir The directory to set as the driver's working directory.
+ * @return This launcher.
+ */
+ public SparkLauncher directory(File dir) {
+ builder.workingDir = dir;
+ return this;
+ }
+
+ /**
+ * Specifies that stderr in the driver should be redirected to stdout.
+ * @return This launcher.
+ */
+ public SparkLauncher redirectError() {
+ builder.redirectErrorStream = true;
+ return this;
+ }
+
+ /**
+ * Redirects error output to the specified Redirect.
+ * @param to The method of redirection.
+ * @return This launcher.
+ */
+ public SparkLauncher redirectError(ProcessBuilder.Redirect to) {
+ builder.errorStream = to;
+ return this;
+ }
+
+ /**
+ * Redirects standard output to the specified Redirect.
+ * @param to The method of redirection.
+ * @return This launcher.
+ */
+ public SparkLauncher redirectOutput(ProcessBuilder.Redirect to) {
+ builder.outputStream = to;
+ return this;
+ }
+
+ /**
+ * Redirects error output to the specified File.
+ * @param errFile The file to which stderr is written.
+ * @return This launcher.
+ */
+ public SparkLauncher redirectError(File errFile) {
+ builder.errorStream = ProcessBuilder.Redirect.to(errFile);
+ return this;
+ }
+
+ /**
+ * Redirects error output to the specified File.
+ * @param outFile The file to which stdout is written.
+ * @return This launcher.
+ */
+ public SparkLauncher redirectOutput(File outFile) {
+ builder.outputStream = ProcessBuilder.Redirect.to(outFile);
+ return this;
+ }
+
+ /**
+ * Sets all output to be logged and redirected to a logger with the specified name.
+ * @param loggerName The name of the logger to log stdout and stderr.
+ * @return This launcher.
+ */
+ public SparkLauncher redirectToLog(String loggerName) {
+ try {
+ // NOTE: the below ordering is important, so builder.redirectToLog is only set to true iff
+ // the preceding put() finishes without exception.
+ builder.getEffectiveConfig().put(CHILD_PROCESS_LOGGER_NAME, loggerName);
--- End diff --
No, `getEffectiveConfig()` is updated whenever you modify the configuration (e.g. via `setConf`).
---
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 #14201: [SPARK-14702] Make environment of SparkLauncher l...
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on a diff in the pull request:
https://github.com/apache/spark/pull/14201#discussion_r71015468
--- Diff: launcher/src/main/java/org/apache/spark/launcher/SparkLauncher.java ---
@@ -418,9 +511,16 @@ public SparkAppHandle startApplication(SparkAppHandle.Listener... listeners) thr
}
}
- String loggerPrefix = getClass().getPackage().getName();
- String loggerName = String.format("%s.app.%s", loggerPrefix, appName);
- ProcessBuilder pb = createBuilder().redirectErrorStream(true);
+ String loggerName = builder.getEffectiveConfig().get(CHILD_PROCESS_LOGGER_NAME);
+ ProcessBuilder pb = createBuilder();
+ // Only setup stderr + stdout to logger redirection if user has not otherwise configured output
+ // redirection.
+ if (!redirectErrorStream && !redirectToLog && errorStream == null && outputStream == null) {
+ String loggerPrefix = getClass().getPackage().getName();
+ loggerName = String.format("%s.app.%s", loggerPrefix, appName);
--- End diff --
You can move the code that computes `appName` inside this condition.
---
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 issue #14201: [SPARK-14702] Make environment of SparkLauncher launched...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/14201
**[Test build #62378 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/62378/consoleFull)** for PR 14201 at commit [`fa59b49`](https://github.com/apache/spark/commit/fa59b498a417ad51207314698e92c178c942dc5f).
---
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 issue #14201: [SPARK-14702] Make environment of SparkLauncher launched...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/14201
**[Test build #62392 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/62392/consoleFull)** for PR 14201 at commit [`83d5b2b`](https://github.com/apache/spark/commit/83d5b2bd941c07e445144f0570a07b87fe032db2).
---
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 issue #14201: [SPARK-14702] Make environment of SparkLauncher launched...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/14201
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/62393/
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 issue #14201: [SPARK-14702] Make environment of SparkLauncher launched...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/14201
**[Test build #62376 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/62376/consoleFull)** for PR 14201 at commit [`2357c07`](https://github.com/apache/spark/commit/2357c07de07d1a4eb31bbde2e7851e297987ea4e).
* This patch **fails Spark 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 issue #14201: [SPARK-14702] Make environment of SparkLauncher launched...
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on the issue:
https://github.com/apache/spark/pull/14201
Merging to master, 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 issue #14201: [SPARK-14702] Make environment of SparkLauncher launched...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/14201
**[Test build #62377 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/62377/consoleFull)** for PR 14201 at commit [`0538f9a`](https://github.com/apache/spark/commit/0538f9a4b0a5508ff7ebba5300c9b27d5146b192).
* This patch **fails Spark 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 issue #14201: [SPARK-14702] Make environment of SparkLauncher launched...
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on the issue:
https://github.com/apache/spark/pull/14201
I think I remember now why I asked for the checks to be removed before (I was thinking of mimicking the PB behavior, in that calling `redirectOutput` after `redirectToLog` would disable `redirectToLog`), but I think the current behavior is less confusing in the end.
LGTM pending some style issues. Also, as a hint, it's better if you just push new commits to your PRs instead of using amend + force push. Then people can see what has changed incrementally.
---
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 #14201: [SPARK-14702] Make environment of SparkLauncher l...
Posted by andreweduffy <gi...@git.apache.org>.
Github user andreweduffy commented on a diff in the pull request:
https://github.com/apache/spark/pull/14201#discussion_r70905418
--- Diff: launcher/src/main/java/org/apache/spark/launcher/SparkLauncher.java ---
@@ -359,6 +364,82 @@ public SparkLauncher setVerbose(boolean verbose) {
}
/**
+ * Sets the working directory of the driver process.
+ * @param dir The directory to set as the driver's working directory.
+ * @return This launcher.
+ */
+ public SparkLauncher directory(File dir) {
+ builder.workingDir = dir;
+ return this;
+ }
+
+ /**
+ * Specifies that stderr in the driver should be redirected to stdout.
+ * @return This launcher.
+ */
+ public SparkLauncher redirectError() {
+ builder.redirectErrorStream = true;
+ return this;
+ }
+
+ /**
+ * Redirects error output to the specified Redirect.
+ * @param to The method of redirection.
+ * @return This launcher.
+ */
+ public SparkLauncher redirectError(ProcessBuilder.Redirect to) {
+ builder.errorStream = to;
+ return this;
+ }
+
+ /**
+ * Redirects standard output to the specified Redirect.
+ * @param to The method of redirection.
+ * @return This launcher.
+ */
+ public SparkLauncher redirectOutput(ProcessBuilder.Redirect to) {
+ builder.outputStream = to;
+ return this;
+ }
+
+ /**
+ * Redirects error output to the specified File.
+ * @param errFile The file to which stderr is written.
+ * @return This launcher.
+ */
+ public SparkLauncher redirectError(File errFile) {
+ builder.errorStream = ProcessBuilder.Redirect.to(errFile);
+ return this;
+ }
+
+ /**
+ * Redirects error output to the specified File.
+ * @param outFile The file to which stdout is written.
+ * @return This launcher.
+ */
+ public SparkLauncher redirectOutput(File outFile) {
+ builder.outputStream = ProcessBuilder.Redirect.to(outFile);
+ return this;
+ }
+
+ /**
+ * Sets all output to be logged and redirected to a logger with the specified name.
+ * @param loggerName The name of the logger to log stdout and stderr.
+ * @return This launcher.
+ */
+ public SparkLauncher redirectToLog(String loggerName) {
+ try {
+ // NOTE: the below ordering is important, so builder.redirectToLog is only set to true iff
+ // the preceding put() finishes without exception.
+ builder.getEffectiveConfig().put(CHILD_PROCESS_LOGGER_NAME, loggerName);
--- End diff --
Should I also modify `startApplication` to read from builder.conf? It appears to use `builder.getEffectiveConfig()` which as far as I can tell is sourced from a properties file
---
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 #14201: [SPARK-14702] Make environment of SparkLauncher l...
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on a diff in the pull request:
https://github.com/apache/spark/pull/14201#discussion_r71012974
--- Diff: launcher/src/main/java/org/apache/spark/launcher/SparkLauncher.java ---
@@ -359,6 +369,83 @@ public SparkLauncher setVerbose(boolean verbose) {
}
/**
+ * Sets the working directory of spark-submit.
+ *
+ * @param dir The directory to set as spark-submit's working directory.
+ * @return This launcher.
+ */
+ public SparkLauncher directory(File dir) {
+ workingDir = dir;
+ return this;
+ }
+
+ /**
+ * Specifies that stderr in spark-submit should be redirected to stdout.
+ *
+ * @return This launcher.
+ */
+ public SparkLauncher redirectError() {
+ redirectErrorStream = true;
+ return this;
+ }
+
+ /**
+ * Redirects error output to the specified Redirect.
+ *
+ * @param to The method of redirection.
+ * @return This launcher.
+ */
+ public SparkLauncher redirectError(ProcessBuilder.Redirect to) {
+ errorStream = to;
+ return this;
+ }
+
+ /**
+ * Redirects standard output to the specified Redirect.
+ *
+ * @param to The method of redirection.
+ * @return This launcher.
+ */
+ public SparkLauncher redirectOutput(ProcessBuilder.Redirect to) {
+ outputStream = to;
+ return this;
+ }
+
+ /**
+ * Redirects error output to the specified File.
+ *
+ * @param errFile The file to which stderr is written.
+ * @return This launcher.
+ */
+ public SparkLauncher redirectError(File errFile) {
+ errorStream = ProcessBuilder.Redirect.to(errFile);
+ return this;
+ }
+
+ /**
+ * Redirects error output to the specified File.
+ *
+ * @param outFile The file to which stdout is written.
+ * @return This launcher.
+ */
+ public SparkLauncher redirectOutput(File outFile) {
+ outputStream = ProcessBuilder.Redirect.to(outFile);
+ return this;
+ }
+
+ /**
+ * Sets all output to be logged and redirected to a logger with the specified name.
+ *
+ * @param loggerName The name of the logger to log stdout and stderr.
+ * @return This launcher.
+ */
+ public SparkLauncher redirectToLog(String loggerName) {
+ setConf(CHILD_PROCESS_LOGGER_NAME, loggerName);
+ redirectToLog = true;
--- End diff --
Nevermind, I see this is handled when you create the builder.
---
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 issue #14201: [SPARK-14702] Expose SparkLauncher's ProcessBuilder for ...
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on the issue:
https://github.com/apache/spark/pull/14201
Also, can you update the javadoc for `startApplication` to mention that output redirection is only done if it hasn't been configured by the user?
---
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 issue #14201: [SPARK-14702] Expose SparkLauncher's ProcessBuilder for ...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/14201
**[Test build #62334 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/62334/consoleFull)** for PR 14201 at commit [`deca998`](https://github.com/apache/spark/commit/deca998b725eeb7df2f12999c8bbac3c1ffc5161).
* This patch passes all 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 #14201: [SPARK-14702] Make environment of SparkLauncher l...
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on a diff in the pull request:
https://github.com/apache/spark/pull/14201#discussion_r71012383
--- Diff: launcher/src/main/java/org/apache/spark/launcher/SparkLauncher.java ---
@@ -359,6 +369,83 @@ public SparkLauncher setVerbose(boolean verbose) {
}
/**
+ * Sets the working directory of spark-submit.
+ *
+ * @param dir The directory to set as spark-submit's working directory.
+ * @return This launcher.
+ */
--- End diff --
nit: alignment is off
---
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 #14201: [SPARK-14702] Make environment of SparkLauncher l...
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on a diff in the pull request:
https://github.com/apache/spark/pull/14201#discussion_r71189126
--- Diff: core/src/test/java/org/apache/spark/launcher/SparkLauncherSuite.java ---
@@ -85,14 +91,67 @@ public void testSparkArgumentHandling() throws Exception {
assertEquals("bar", launcher.builder.conf.get("spark.foo"));
}
+ @Test(expected=IllegalStateException.class)
+ public void testRedirectTwiceFails() throws Exception {
+ launcher.setAppResource("fake-resource.jar")
+ .setMainClass("my.fake.class.Fake")
+ .redirectError()
+ .redirectError(ProcessBuilder.Redirect.PIPE)
+ .launch();
+ }
+
+ @Test(expected=IllegalStateException.class)
+ public void testRedirectToLogWithOthersFails() throws Exception {
+ launcher.setAppResource("fake-resource.jar")
+ .setMainClass("my.fake.class.Fake")
--- End diff --
nit: alignment...
---
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 #14201: [SPARK-14702] Expose SparkLauncher's ProcessBuild...
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on a diff in the pull request:
https://github.com/apache/spark/pull/14201#discussion_r70869464
--- Diff: launcher/src/main/java/org/apache/spark/launcher/SparkLauncher.java ---
@@ -455,6 +542,36 @@ private ProcessBuilder createBuilder() {
for (Map.Entry<String, String> e : builder.childEnv.entrySet()) {
pb.environment().put(e.getKey(), e.getValue());
}
+
+ if (builder.workingDir != null) {
+ pb.directory(builder.workingDir);
+ }
+
+ // Only one of redirectError, redirectError(...) can be specified.
+ // If redirectToLog() is specified, none of the other redirect options can be specified
+ if (builder.redirectToLog &&
--- End diff --
I think this should follow the semantics of `ProcessBuilder`, which is "last one wins". Try this out:
```
new ProcessBuilder("ls").redirectOutput(ProcessBuilder.Redirect.PIPE).redirectOutput(ProcessBuilder.Redirect.INHERIT).start()
```
---
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 #14201: [SPARK-14702] Make environment of SparkLauncher l...
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on a diff in the pull request:
https://github.com/apache/spark/pull/14201#discussion_r71189166
--- Diff: core/src/test/java/org/apache/spark/launcher/SparkLauncherSuite.java ---
@@ -85,14 +91,67 @@ public void testSparkArgumentHandling() throws Exception {
assertEquals("bar", launcher.builder.conf.get("spark.foo"));
}
+ @Test(expected=IllegalStateException.class)
+ public void testRedirectTwiceFails() throws Exception {
+ launcher.setAppResource("fake-resource.jar")
+ .setMainClass("my.fake.class.Fake")
+ .redirectError()
+ .redirectError(ProcessBuilder.Redirect.PIPE)
+ .launch();
+ }
+
+ @Test(expected=IllegalStateException.class)
+ public void testRedirectToLogWithOthersFails() throws Exception {
+ launcher.setAppResource("fake-resource.jar")
+ .setMainClass("my.fake.class.Fake")
+ .redirectToLog("fakeLog")
+ .redirectError(ProcessBuilder.Redirect.PIPE)
+ .launch();
+ }
+
+ @Test
+ public void testRedirectErrorToOutput() throws Exception {
+ launcher.redirectError();
+ assertTrue(launcher.redirectErrorStream);
+ }
+
+ @Test
+ public void testRedirectsSimple() throws Exception {
+ launcher.redirectError(ProcessBuilder.Redirect.PIPE);
+ assertNotNull(launcher.errorStream);
+ assertEquals(launcher.errorStream.type(), ProcessBuilder.Redirect.Type.PIPE);
+
+ launcher.redirectOutput(ProcessBuilder.Redirect.PIPE);
+ assertNotNull(launcher.outputStream);
+ assertEquals(launcher.outputStream.type(), ProcessBuilder.Redirect.Type.PIPE);
+ }
+
+ @Test
+ public void testRedirectLastWins() throws Exception {
+ launcher.redirectError(ProcessBuilder.Redirect.PIPE)
+ .redirectError(ProcessBuilder.Redirect.INHERIT);
+ assertEquals(launcher.errorStream.type(), ProcessBuilder.Redirect.Type.INHERIT);
+
+ launcher.redirectOutput(ProcessBuilder.Redirect.PIPE)
+ .redirectOutput(ProcessBuilder.Redirect.INHERIT);
--- End diff --
nit: alignment
---
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 issue #14201: [SPARK-14702] Make environment of SparkLauncher launched...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/14201
**[Test build #62397 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/62397/consoleFull)** for PR 14201 at commit [`5b5cb42`](https://github.com/apache/spark/commit/5b5cb4287f5445430ec10ca0679e9684b9821c0f).
---
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 #14201: [SPARK-14702] Make environment of SparkLauncher l...
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on a diff in the pull request:
https://github.com/apache/spark/pull/14201#discussion_r71050089
--- Diff: launcher/src/main/java/org/apache/spark/launcher/ChildProcAppHandle.java ---
@@ -127,7 +124,7 @@ String getSecret() {
void setChildProc(Process childProc, String loggerName) {
this.childProc = childProc;
this.redirector = new OutputRedirector(childProc.getInputStream(), loggerName,
- REDIRECTOR_FACTORY);
+ SparkLauncher.REDIRECTOR_FACTORY);
--- End diff --
nit: indented too far (see previous 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 #14201: [SPARK-14702] Make environment of SparkLauncher l...
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on a diff in the pull request:
https://github.com/apache/spark/pull/14201#discussion_r71189151
--- Diff: core/src/test/java/org/apache/spark/launcher/SparkLauncherSuite.java ---
@@ -85,14 +91,67 @@ public void testSparkArgumentHandling() throws Exception {
assertEquals("bar", launcher.builder.conf.get("spark.foo"));
}
+ @Test(expected=IllegalStateException.class)
+ public void testRedirectTwiceFails() throws Exception {
+ launcher.setAppResource("fake-resource.jar")
+ .setMainClass("my.fake.class.Fake")
+ .redirectError()
+ .redirectError(ProcessBuilder.Redirect.PIPE)
+ .launch();
+ }
+
+ @Test(expected=IllegalStateException.class)
+ public void testRedirectToLogWithOthersFails() throws Exception {
+ launcher.setAppResource("fake-resource.jar")
+ .setMainClass("my.fake.class.Fake")
+ .redirectToLog("fakeLog")
+ .redirectError(ProcessBuilder.Redirect.PIPE)
+ .launch();
+ }
+
+ @Test
+ public void testRedirectErrorToOutput() throws Exception {
+ launcher.redirectError();
+ assertTrue(launcher.redirectErrorStream);
+ }
+
+ @Test
+ public void testRedirectsSimple() throws Exception {
+ launcher.redirectError(ProcessBuilder.Redirect.PIPE);
+ assertNotNull(launcher.errorStream);
+ assertEquals(launcher.errorStream.type(), ProcessBuilder.Redirect.Type.PIPE);
+
+ launcher.redirectOutput(ProcessBuilder.Redirect.PIPE);
+ assertNotNull(launcher.outputStream);
+ assertEquals(launcher.outputStream.type(), ProcessBuilder.Redirect.Type.PIPE);
+ }
+
+ @Test
+ public void testRedirectLastWins() throws Exception {
+ launcher.redirectError(ProcessBuilder.Redirect.PIPE)
+ .redirectError(ProcessBuilder.Redirect.INHERIT);
--- End diff --
nit: alignment
---
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 issue #14201: [SPARK-14702] Make environment of SparkLauncher launched...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/14201
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/62378/
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 issue #14201: [SPARK-14702] Make environment of SparkLauncher launched...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/14201
Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/62377/
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 #14201: [SPARK-14702] Make environment of SparkLauncher l...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/spark/pull/14201
---
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 #14201: [SPARK-14702] Expose SparkLauncher's ProcessBuild...
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on a diff in the pull request:
https://github.com/apache/spark/pull/14201#discussion_r70870422
--- Diff: launcher/src/main/java/org/apache/spark/launcher/SparkLauncher.java ---
@@ -82,8 +83,12 @@
/** Used internally to create unique logger names. */
private static final AtomicInteger COUNTER = new AtomicInteger();
+ public static final ThreadFactory REDIRECTOR_FACTORY = new NamedThreadFactory("launcher-proc-%d");
--- End diff --
It would be nice if the same factory were used by `ChildProcAppHandle` also. And this shouldn't be public.
---
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 issue #14201: [SPARK-14702] Make environment of SparkLauncher launched...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/14201
Merged build finished. 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 #14201: [SPARK-14702] Make environment of SparkLauncher l...
Posted by andreweduffy <gi...@git.apache.org>.
Github user andreweduffy commented on a diff in the pull request:
https://github.com/apache/spark/pull/14201#discussion_r70904921
--- Diff: launcher/src/main/java/org/apache/spark/launcher/SparkLauncher.java ---
@@ -82,8 +83,12 @@
/** Used internally to create unique logger names. */
private static final AtomicInteger COUNTER = new AtomicInteger();
+ public static final ThreadFactory REDIRECTOR_FACTORY = new NamedThreadFactory("launcher-proc-%d");
--- End diff --
How should this be shared do you think without making it public static in either SparkLauncher or ChildProcApphandle?
---
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 issue #14201: [SPARK-14702] Make environment of SparkLauncher launched...
Posted by andreweduffy <gi...@git.apache.org>.
Github user andreweduffy commented on the issue:
https://github.com/apache/spark/pull/14201
Gotcha, sorry about the force overwrites, in some projects I've worked on that's preferred to keep clean history, though for review I can see why separate units is easier. Thanks again for taking the time to review this.
---
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 #14201: [SPARK-14702] Make environment of SparkLauncher l...
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on a diff in the pull request:
https://github.com/apache/spark/pull/14201#discussion_r71050048
--- Diff: core/src/test/java/org/apache/spark/launcher/SparkLauncherSuite.java ---
@@ -85,14 +91,69 @@ public void testSparkArgumentHandling() throws Exception {
assertEquals("bar", launcher.builder.conf.get("spark.foo"));
}
+ @Test(expected=IllegalStateException.class)
+ public void testRedirectTwiceFails() throws Exception {
+ launcher.setAppResource("fake-resource.jar")
--- End diff --
nit: fix alignment.
```
launcher.setAppResource("fake-resource.jar")
.setMainClass("my.fake.class.Fake")
.redirectError()
.redirectError(ProcessBuilder.Redirect.PIPE)
.launch();
```
Same below.
---
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 issue #14201: [SPARK-14702] Make environment of SparkLauncher launched...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/14201
**[Test build #62393 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/62393/consoleFull)** for PR 14201 at commit [`7d33745`](https://github.com/apache/spark/commit/7d33745ad6b0b0202c94fd428d8500c540a1e0ae).
---
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 #14201: [SPARK-14702] Expose SparkLauncher's ProcessBuild...
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on a diff in the pull request:
https://github.com/apache/spark/pull/14201#discussion_r70870990
--- Diff: launcher/src/main/java/org/apache/spark/launcher/SparkLauncher.java ---
@@ -455,6 +542,36 @@ private ProcessBuilder createBuilder() {
for (Map.Entry<String, String> e : builder.childEnv.entrySet()) {
pb.environment().put(e.getKey(), e.getValue());
}
+
+ if (builder.workingDir != null) {
+ pb.directory(builder.workingDir);
+ }
+
+ // Only one of redirectError, redirectError(...) can be specified.
+ // If redirectToLog() is specified, none of the other redirect options can be specified
+ if (builder.redirectToLog &&
+ (builder.redirectErrorStream
+ || (builder.errorStream != null)
+ || (builder.outputStream != null))) {
+ throw new IllegalStateException("Cannot specify redirectToLog() in conjunction with other " +
+ "redirection operations.");
+ }
+
+ // Only one of redirectError and redirectError(...) can be specified
+ if (builder.redirectErrorStream && builder.errorStream != null) {
--- End diff --
Probably ok to leave this one, but `ProcessBuilder` allows it even if it doesn't do what you'd expect.
```
new ProcessBuilder(java.util.Arrays.asList("ls", "/foo")).redirectErrorStream(true).redirectError(ProcessBuilder.Redirect.INHERIT).start()
```
---
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 issue #14201: [SPARK-14702] Make environment of SparkLauncher launched...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/14201
**[Test build #62392 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/62392/consoleFull)** for PR 14201 at commit [`83d5b2b`](https://github.com/apache/spark/commit/83d5b2bd941c07e445144f0570a07b87fe032db2).
* This patch passes all 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 #14201: [SPARK-14702] Make environment of SparkLauncher l...
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on a diff in the pull request:
https://github.com/apache/spark/pull/14201#discussion_r71032896
--- Diff: launcher/src/main/java/org/apache/spark/launcher/SparkLauncher.java ---
@@ -359,6 +369,83 @@ public SparkLauncher setVerbose(boolean verbose) {
}
/**
+ * Sets the working directory of spark-submit.
+ *
+ * @param dir The directory to set as spark-submit's working directory.
+ * @return This launcher.
+ */
+ public SparkLauncher directory(File dir) {
+ workingDir = dir;
+ return this;
+ }
+
+ /**
+ * Specifies that stderr in spark-submit should be redirected to stdout.
+ *
+ * @return This launcher.
+ */
+ public SparkLauncher redirectError() {
+ redirectErrorStream = true;
+ return this;
+ }
+
+ /**
+ * Redirects error output to the specified Redirect.
+ *
+ * @param to The method of redirection.
+ * @return This launcher.
+ */
+ public SparkLauncher redirectError(ProcessBuilder.Redirect to) {
+ errorStream = to;
+ return this;
+ }
+
+ /**
+ * Redirects standard output to the specified Redirect.
+ *
+ * @param to The method of redirection.
+ * @return This launcher.
+ */
+ public SparkLauncher redirectOutput(ProcessBuilder.Redirect to) {
+ outputStream = to;
+ return this;
+ }
+
+ /**
+ * Redirects error output to the specified File.
+ *
+ * @param errFile The file to which stderr is written.
+ * @return This launcher.
+ */
+ public SparkLauncher redirectError(File errFile) {
+ errorStream = ProcessBuilder.Redirect.to(errFile);
+ return this;
+ }
+
+ /**
+ * Redirects error output to the specified File.
+ *
+ * @param outFile The file to which stdout is written.
+ * @return This launcher.
+ */
+ public SparkLauncher redirectOutput(File outFile) {
+ outputStream = ProcessBuilder.Redirect.to(outFile);
+ return this;
+ }
+
+ /**
+ * Sets all output to be logged and redirected to a logger with the specified name.
+ *
+ * @param loggerName The name of the logger to log stdout and stderr.
+ * @return This launcher.
+ */
+ public SparkLauncher redirectToLog(String loggerName) {
--- End diff --
Could also check that `redirectError` and `redirectOutput` fail if `redirectToLog` has been set.
(I see that I commented on a previous diff asking you to remove a check, sorry about that. I can't see the original code, though.)
---
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 #14201: [SPARK-14702] Make environment of SparkLauncher l...
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on a diff in the pull request:
https://github.com/apache/spark/pull/14201#discussion_r71049971
--- Diff: core/src/test/java/org/apache/spark/launcher/SparkLauncherSuite.java ---
@@ -85,14 +91,69 @@ public void testSparkArgumentHandling() throws Exception {
assertEquals("bar", launcher.builder.conf.get("spark.foo"));
}
+ @Test(expected=IllegalStateException.class)
+ public void testRedirectTwiceFails() throws Exception {
+ launcher.setAppResource("fake-resource.jar")
+ .setMainClass("my.fake.class.Fake")
+ .redirectError()
+ .redirectError(ProcessBuilder.Redirect.PIPE)
+ .launch();
+ fail();
--- End diff --
Not 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 issue #14201: [SPARK-14702] Expose SparkLauncher's ProcessBuilder for ...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/14201
Merged build finished. 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 issue #14201: [SPARK-14702] Make environment of SparkLauncher launched...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/14201
**[Test build #62393 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/62393/consoleFull)** for PR 14201 at commit [`7d33745`](https://github.com/apache/spark/commit/7d33745ad6b0b0202c94fd428d8500c540a1e0ae).
* This patch passes all 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 issue #14201: [SPARK-14702] Make environment of SparkLauncher launched...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/14201
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/62397/
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 #14201: [SPARK-14702] Expose SparkLauncher's ProcessBuild...
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on a diff in the pull request:
https://github.com/apache/spark/pull/14201#discussion_r70870174
--- Diff: launcher/src/main/java/org/apache/spark/launcher/SparkLauncher.java ---
@@ -359,6 +364,82 @@ public SparkLauncher setVerbose(boolean verbose) {
}
/**
+ * Sets the working directory of the driver process.
+ * @param dir The directory to set as the driver's working directory.
+ * @return This launcher.
+ */
+ public SparkLauncher directory(File dir) {
+ builder.workingDir = dir;
+ return this;
+ }
+
+ /**
+ * Specifies that stderr in the driver should be redirected to stdout.
+ * @return This launcher.
+ */
+ public SparkLauncher redirectError() {
+ builder.redirectErrorStream = true;
+ return this;
+ }
+
+ /**
+ * Redirects error output to the specified Redirect.
+ * @param to The method of redirection.
+ * @return This launcher.
+ */
+ public SparkLauncher redirectError(ProcessBuilder.Redirect to) {
+ builder.errorStream = to;
+ return this;
+ }
+
+ /**
+ * Redirects standard output to the specified Redirect.
+ * @param to The method of redirection.
+ * @return This launcher.
+ */
+ public SparkLauncher redirectOutput(ProcessBuilder.Redirect to) {
+ builder.outputStream = to;
+ return this;
+ }
+
+ /**
+ * Redirects error output to the specified File.
+ * @param errFile The file to which stderr is written.
+ * @return This launcher.
+ */
+ public SparkLauncher redirectError(File errFile) {
+ builder.errorStream = ProcessBuilder.Redirect.to(errFile);
+ return this;
+ }
+
+ /**
+ * Redirects error output to the specified File.
+ * @param outFile The file to which stdout is written.
+ * @return This launcher.
+ */
+ public SparkLauncher redirectOutput(File outFile) {
+ builder.outputStream = ProcessBuilder.Redirect.to(outFile);
+ return this;
+ }
+
+ /**
+ * Sets all output to be logged and redirected to a logger with the specified name.
+ * @param loggerName The name of the logger to log stdout and stderr.
+ * @return This launcher.
+ */
+ public SparkLauncher redirectToLog(String loggerName) {
+ try {
+ // NOTE: the below ordering is important, so builder.redirectToLog is only set to true iff
+ // the preceding put() finishes without exception.
+ builder.getEffectiveConfig().put(CHILD_PROCESS_LOGGER_NAME, loggerName);
--- End diff --
`setConf` instead of `builder.getEffectiveConfig().put`.
---
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 #14201: [SPARK-14702] Make environment of SparkLauncher l...
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on a diff in the pull request:
https://github.com/apache/spark/pull/14201#discussion_r71049986
--- Diff: core/src/test/java/org/apache/spark/launcher/SparkLauncherSuite.java ---
@@ -85,14 +91,69 @@ public void testSparkArgumentHandling() throws Exception {
assertEquals("bar", launcher.builder.conf.get("spark.foo"));
}
+ @Test(expected=IllegalStateException.class)
+ public void testRedirectTwiceFails() throws Exception {
+ launcher.setAppResource("fake-resource.jar")
+ .setMainClass("my.fake.class.Fake")
+ .redirectError()
+ .redirectError(ProcessBuilder.Redirect.PIPE)
+ .launch();
+ fail();
+ }
+
+ @Test(expected=IllegalStateException.class)
+ public void testRedirectToLogWithOthersFails() throws Exception {
+ launcher.setAppResource("fake-resource.jar")
+ .setMainClass("my.fake.class.Fake")
+ .redirectToLog("fakeLog")
+ .redirectError(ProcessBuilder.Redirect.PIPE)
+ .launch();
+ fail();
--- End diff --
Not 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 issue #14201: [SPARK-14702] Expose SparkLauncher's ProcessBuilder for ...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/14201
Merged build finished. 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 issue #14201: [SPARK-14702] Make environment of SparkLauncher launched...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/14201
Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/62376/
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 #14201: [SPARK-14702] Make environment of SparkLauncher l...
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on a diff in the pull request:
https://github.com/apache/spark/pull/14201#discussion_r71016114
--- Diff: launcher/src/main/java/org/apache/spark/launcher/SparkLauncher.java ---
@@ -359,6 +369,83 @@ public SparkLauncher setVerbose(boolean verbose) {
}
/**
+ * Sets the working directory of spark-submit.
+ *
+ * @param dir The directory to set as spark-submit's working directory.
+ * @return This launcher.
+ */
+ public SparkLauncher directory(File dir) {
+ workingDir = dir;
+ return this;
+ }
+
+ /**
+ * Specifies that stderr in spark-submit should be redirected to stdout.
+ *
+ * @return This launcher.
+ */
+ public SparkLauncher redirectError() {
+ redirectErrorStream = true;
+ return this;
+ }
+
+ /**
+ * Redirects error output to the specified Redirect.
+ *
+ * @param to The method of redirection.
+ * @return This launcher.
+ */
+ public SparkLauncher redirectError(ProcessBuilder.Redirect to) {
+ errorStream = to;
+ return this;
+ }
+
+ /**
+ * Redirects standard output to the specified Redirect.
+ *
+ * @param to The method of redirection.
+ * @return This launcher.
+ */
+ public SparkLauncher redirectOutput(ProcessBuilder.Redirect to) {
+ outputStream = to;
+ return this;
+ }
+
+ /**
+ * Redirects error output to the specified File.
+ *
+ * @param errFile The file to which stderr is written.
+ * @return This launcher.
+ */
+ public SparkLauncher redirectError(File errFile) {
+ errorStream = ProcessBuilder.Redirect.to(errFile);
+ return this;
+ }
+
+ /**
+ * Redirects error output to the specified File.
+ *
+ * @param outFile The file to which stdout is written.
+ * @return This launcher.
+ */
+ public SparkLauncher redirectOutput(File outFile) {
+ outputStream = ProcessBuilder.Redirect.to(outFile);
+ return this;
+ }
+
+ /**
+ * Sets all output to be logged and redirected to a logger with the specified name.
+ *
+ * @param loggerName The name of the logger to log stdout and stderr.
+ * @return This launcher.
+ */
+ public SparkLauncher redirectToLog(String loggerName) {
--- End diff --
What do you think about adding `checkState` checks in these methods?
Right now you can configure the launcher in a weird way:
```
launcher.redirectOutput(Redirect.INHERIT).redirectToLog(...)
```
And that will launch a process and it's not clear what will happen. You have one check for error redirection, but it would be nice to add other checks.
---
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 #14201: [SPARK-14702] Make environment of SparkLauncher l...
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on a diff in the pull request:
https://github.com/apache/spark/pull/14201#discussion_r71012823
--- Diff: launcher/src/main/java/org/apache/spark/launcher/SparkLauncher.java ---
@@ -359,6 +369,83 @@ public SparkLauncher setVerbose(boolean verbose) {
}
/**
+ * Sets the working directory of spark-submit.
+ *
+ * @param dir The directory to set as spark-submit's working directory.
+ * @return This launcher.
+ */
+ public SparkLauncher directory(File dir) {
+ workingDir = dir;
+ return this;
+ }
+
+ /**
+ * Specifies that stderr in spark-submit should be redirected to stdout.
+ *
+ * @return This launcher.
+ */
+ public SparkLauncher redirectError() {
+ redirectErrorStream = true;
+ return this;
+ }
+
+ /**
+ * Redirects error output to the specified Redirect.
+ *
+ * @param to The method of redirection.
+ * @return This launcher.
+ */
+ public SparkLauncher redirectError(ProcessBuilder.Redirect to) {
+ errorStream = to;
+ return this;
+ }
+
+ /**
+ * Redirects standard output to the specified Redirect.
+ *
+ * @param to The method of redirection.
+ * @return This launcher.
+ */
+ public SparkLauncher redirectOutput(ProcessBuilder.Redirect to) {
+ outputStream = to;
+ return this;
+ }
+
+ /**
+ * Redirects error output to the specified File.
+ *
+ * @param errFile The file to which stderr is written.
+ * @return This launcher.
+ */
+ public SparkLauncher redirectError(File errFile) {
+ errorStream = ProcessBuilder.Redirect.to(errFile);
+ return this;
+ }
+
+ /**
+ * Redirects error output to the specified File.
+ *
+ * @param outFile The file to which stdout is written.
+ * @return This launcher.
+ */
+ public SparkLauncher redirectOutput(File outFile) {
+ outputStream = ProcessBuilder.Redirect.to(outFile);
+ return this;
+ }
+
+ /**
+ * Sets all output to be logged and redirected to a logger with the specified name.
+ *
+ * @param loggerName The name of the logger to log stdout and stderr.
+ * @return This launcher.
+ */
+ public SparkLauncher redirectToLog(String loggerName) {
+ setConf(CHILD_PROCESS_LOGGER_NAME, loggerName);
+ redirectToLog = true;
--- End diff --
This should also set `redirectErrorStream` to `true`, otherwise, for example, `launch` will read the child's stdout but not stderr, leading to issues.
---
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 issue #14201: [SPARK-14702] Make environment of SparkLauncher launched...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/14201
Merged build finished. 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 issue #14201: [SPARK-14702] Make environment of SparkLauncher launched...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/14201
**[Test build #62377 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/62377/consoleFull)** for PR 14201 at commit [`0538f9a`](https://github.com/apache/spark/commit/0538f9a4b0a5508ff7ebba5300c9b27d5146b192).
---
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 issue #14201: [SPARK-14702] Make environment of SparkLauncher launched...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/14201
**[Test build #62397 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/62397/consoleFull)** for PR 14201 at commit [`5b5cb42`](https://github.com/apache/spark/commit/5b5cb4287f5445430ec10ca0679e9684b9821c0f).
* This patch passes all 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 #14201: [SPARK-14702] Make environment of SparkLauncher l...
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on a diff in the pull request:
https://github.com/apache/spark/pull/14201#discussion_r71015601
--- Diff: launcher/src/main/java/org/apache/spark/launcher/SparkLauncher.java ---
@@ -455,6 +555,26 @@ private ProcessBuilder createBuilder() {
for (Map.Entry<String, String> e : builder.childEnv.entrySet()) {
pb.environment().put(e.getKey(), e.getValue());
}
+
+ if (workingDir != null) {
+ pb.directory(workingDir);
+ }
+
+ // Only one of redirectError and redirectError(...) can be specified
+ if (redirectErrorStream && errorStream != null) {
--- End diff --
minor: you can use `checkState` (from `CommandBuilderUtils` which is already imported).
---
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 #14201: [SPARK-14702] Make environment of SparkLauncher l...
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on a diff in the pull request:
https://github.com/apache/spark/pull/14201#discussion_r71050921
--- Diff: launcher/src/main/java/org/apache/spark/launcher/SparkLauncher.java ---
@@ -455,6 +553,29 @@ private ProcessBuilder createBuilder() {
for (Map.Entry<String, String> e : builder.childEnv.entrySet()) {
pb.environment().put(e.getKey(), e.getValue());
}
+
+ if (workingDir != null) {
+ pb.directory(workingDir);
+ }
+
+ // Only one of redirectError and redirectError(...) can be specified.
+ // Similarly, if redirectToLog is specified, no other redirections should be specified.
+ checkState(!redirectErrorStream || (redirectErrorStream && errorStream == null),
+ "Cannot specify both redirectError() and redirectError(...) ");
+ checkState(!redirectToLog ||
+ (redirectToLog && !redirectErrorStream && errorStream == null && outputStream == null),
--- End diff --
similarly you can drop `redirectToLog` from the check after the `||`.
---
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 #14201: [SPARK-14702] Make environment of SparkLauncher l...
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on a diff in the pull request:
https://github.com/apache/spark/pull/14201#discussion_r71050389
--- Diff: launcher/src/main/java/org/apache/spark/launcher/SparkLauncher.java ---
@@ -455,6 +553,29 @@ private ProcessBuilder createBuilder() {
for (Map.Entry<String, String> e : builder.childEnv.entrySet()) {
pb.environment().put(e.getKey(), e.getValue());
}
+
+ if (workingDir != null) {
+ pb.directory(workingDir);
+ }
+
+ // Only one of redirectError and redirectError(...) can be specified.
+ // Similarly, if redirectToLog is specified, no other redirections should be specified.
+ checkState(!redirectErrorStream || (redirectErrorStream && errorStream == null),
+ "Cannot specify both redirectError() and redirectError(...) ");
--- End diff --
nit: indented too far
---
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 #14201: [SPARK-14702] Make environment of SparkLauncher l...
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on a diff in the pull request:
https://github.com/apache/spark/pull/14201#discussion_r71050405
--- Diff: launcher/src/main/java/org/apache/spark/launcher/SparkLauncher.java ---
@@ -455,6 +553,29 @@ private ProcessBuilder createBuilder() {
for (Map.Entry<String, String> e : builder.childEnv.entrySet()) {
pb.environment().put(e.getKey(), e.getValue());
}
+
+ if (workingDir != null) {
+ pb.directory(workingDir);
+ }
+
+ // Only one of redirectError and redirectError(...) can be specified.
+ // Similarly, if redirectToLog is specified, no other redirections should be specified.
+ checkState(!redirectErrorStream || (redirectErrorStream && errorStream == null),
+ "Cannot specify both redirectError() and redirectError(...) ");
+ checkState(!redirectToLog ||
+ (redirectToLog && !redirectErrorStream && errorStream == null && outputStream == null),
--- End diff --
nit: indented too far
---
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 issue #14201: [SPARK-14702] Make environment of SparkLauncher launched...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/14201
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/62481/
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 #14201: [SPARK-14702] Make environment of SparkLauncher l...
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on a diff in the pull request:
https://github.com/apache/spark/pull/14201#discussion_r71050862
--- Diff: launcher/src/main/java/org/apache/spark/launcher/SparkLauncher.java ---
@@ -455,6 +553,29 @@ private ProcessBuilder createBuilder() {
for (Map.Entry<String, String> e : builder.childEnv.entrySet()) {
pb.environment().put(e.getKey(), e.getValue());
}
+
+ if (workingDir != null) {
+ pb.directory(workingDir);
+ }
+
+ // Only one of redirectError and redirectError(...) can be specified.
+ // Similarly, if redirectToLog is specified, no other redirections should be specified.
+ checkState(!redirectErrorStream || (redirectErrorStream && errorStream == null),
--- End diff --
Equivalent to: `!redirectErrorStream || errorStream == null`
---
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 #14201: [SPARK-14702] Expose SparkLauncher's ProcessBuild...
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on a diff in the pull request:
https://github.com/apache/spark/pull/14201#discussion_r70870046
--- Diff: launcher/src/main/java/org/apache/spark/launcher/SparkLauncher.java ---
@@ -359,6 +364,82 @@ public SparkLauncher setVerbose(boolean verbose) {
}
/**
+ * Sets the working directory of the driver process.
--- End diff --
in the whole file: "driver" is misleading, since in cluster mode the child process is not the driver. I'd say "spark-submit" instead.
---
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 #14201: [SPARK-14702] Make environment of SparkLauncher l...
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on a diff in the pull request:
https://github.com/apache/spark/pull/14201#discussion_r71008878
--- Diff: core/src/test/java/org/apache/spark/launcher/SparkLauncherSuite.java ---
@@ -86,13 +93,62 @@ public void testSparkArgumentHandling() throws Exception {
}
@Test
--- End diff --
`@Test(expected=IllegalArgumentException.class)`
---
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 issue #14201: [SPARK-14702] Make environment of SparkLauncher launched...
Posted by andreweduffy <gi...@git.apache.org>.
Github user andreweduffy commented on the issue:
https://github.com/apache/spark/pull/14201
Yuck, sorry, switching between projects my style settings were being carried over, that should be it for indentation fixes
---
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 issue #14201: [SPARK-14702] Make environment of SparkLauncher launched...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/14201
Merged build finished. 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 #14201: [SPARK-14702] Expose SparkLauncher's ProcessBuild...
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on a diff in the pull request:
https://github.com/apache/spark/pull/14201#discussion_r70871119
--- Diff: launcher/src/main/java/org/apache/spark/launcher/SparkLauncher.java ---
@@ -359,6 +364,82 @@ public SparkLauncher setVerbose(boolean verbose) {
}
/**
+ * Sets the working directory of the driver process.
+ * @param dir The directory to set as the driver's working directory.
+ * @return This launcher.
+ */
+ public SparkLauncher directory(File dir) {
+ builder.workingDir = dir;
+ return this;
+ }
+
+ /**
+ * Specifies that stderr in the driver should be redirected to stdout.
+ * @return This launcher.
+ */
+ public SparkLauncher redirectError() {
+ builder.redirectErrorStream = true;
+ return this;
+ }
+
+ /**
+ * Redirects error output to the specified Redirect.
+ * @param to The method of redirection.
+ * @return This launcher.
+ */
+ public SparkLauncher redirectError(ProcessBuilder.Redirect to) {
+ builder.errorStream = to;
+ return this;
+ }
+
+ /**
+ * Redirects standard output to the specified Redirect.
+ * @param to The method of redirection.
+ * @return This launcher.
+ */
+ public SparkLauncher redirectOutput(ProcessBuilder.Redirect to) {
+ builder.outputStream = to;
+ return this;
+ }
+
+ /**
+ * Redirects error output to the specified File.
+ * @param errFile The file to which stderr is written.
--- End diff --
nit: can you add an empty line between the description and the param list in all the new 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 issue #14201: [SPARK-14702] Expose SparkLauncher's ProcessBuilder for ...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/14201
**[Test build #62332 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/62332/consoleFull)** for PR 14201 at commit [`5a35963`](https://github.com/apache/spark/commit/5a35963bc4661193d8c169c7f98fffe24b181124).
---
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 #14201: [SPARK-14702] Make environment of SparkLauncher l...
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on a diff in the pull request:
https://github.com/apache/spark/pull/14201#discussion_r71012332
--- Diff: launcher/src/main/java/org/apache/spark/launcher/SparkLauncher.java ---
@@ -82,8 +83,12 @@
/** Used internally to create unique logger names. */
private static final AtomicInteger COUNTER = new AtomicInteger();
+ /** Factory for creating OutputRedirector threads. **/
+ static final ThreadFactory REDIRECTOR_FACTORY = new NamedThreadFactory("launcher-proc-%d");
+
static final Map<String, String> launcherConfig = new HashMap<>();
+
--- End diff --
nit: no need to add this line
---
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 #14201: [SPARK-14702] Make environment of SparkLauncher l...
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on a diff in the pull request:
https://github.com/apache/spark/pull/14201#discussion_r71189179
--- Diff: core/src/test/java/org/apache/spark/launcher/SparkLauncherSuite.java ---
@@ -85,14 +91,67 @@ public void testSparkArgumentHandling() throws Exception {
assertEquals("bar", launcher.builder.conf.get("spark.foo"));
}
+ @Test(expected=IllegalStateException.class)
+ public void testRedirectTwiceFails() throws Exception {
+ launcher.setAppResource("fake-resource.jar")
+ .setMainClass("my.fake.class.Fake")
+ .redirectError()
+ .redirectError(ProcessBuilder.Redirect.PIPE)
+ .launch();
+ }
+
+ @Test(expected=IllegalStateException.class)
+ public void testRedirectToLogWithOthersFails() throws Exception {
+ launcher.setAppResource("fake-resource.jar")
+ .setMainClass("my.fake.class.Fake")
+ .redirectToLog("fakeLog")
+ .redirectError(ProcessBuilder.Redirect.PIPE)
+ .launch();
+ }
+
+ @Test
+ public void testRedirectErrorToOutput() throws Exception {
+ launcher.redirectError();
+ assertTrue(launcher.redirectErrorStream);
+ }
+
+ @Test
+ public void testRedirectsSimple() throws Exception {
+ launcher.redirectError(ProcessBuilder.Redirect.PIPE);
+ assertNotNull(launcher.errorStream);
+ assertEquals(launcher.errorStream.type(), ProcessBuilder.Redirect.Type.PIPE);
+
+ launcher.redirectOutput(ProcessBuilder.Redirect.PIPE);
+ assertNotNull(launcher.outputStream);
+ assertEquals(launcher.outputStream.type(), ProcessBuilder.Redirect.Type.PIPE);
+ }
+
+ @Test
+ public void testRedirectLastWins() throws Exception {
+ launcher.redirectError(ProcessBuilder.Redirect.PIPE)
+ .redirectError(ProcessBuilder.Redirect.INHERIT);
+ assertEquals(launcher.errorStream.type(), ProcessBuilder.Redirect.Type.INHERIT);
+
+ launcher.redirectOutput(ProcessBuilder.Redirect.PIPE)
+ .redirectOutput(ProcessBuilder.Redirect.INHERIT);
+ assertEquals(launcher.outputStream.type(), ProcessBuilder.Redirect.Type.INHERIT);
+ }
+
+ @Test
+ public void testRedirectToLog() throws Exception {
+ launcher.redirectToLog("fakeLogger");
+ assertTrue(launcher.redirectToLog);
+ assertTrue(launcher.builder.getEffectiveConfig()
+ .containsKey(SparkLauncher.CHILD_PROCESS_LOGGER_NAME));
--- End diff --
nit: alignment
---
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 #14201: [SPARK-14702] Make environment of SparkLauncher l...
Posted by andreweduffy <gi...@git.apache.org>.
Github user andreweduffy commented on a diff in the pull request:
https://github.com/apache/spark/pull/14201#discussion_r70888965
--- Diff: launcher/src/main/java/org/apache/spark/launcher/SparkLauncher.java ---
@@ -82,8 +83,12 @@
/** Used internally to create unique logger names. */
private static final AtomicInteger COUNTER = new AtomicInteger();
+ public static final ThreadFactory REDIRECTOR_FACTORY = new NamedThreadFactory("launcher-proc-%d");
+
static final Map<String, String> launcherConfig = new HashMap<>();
+ private OutputRedirector redirector; // Holds the possible OutputRedirector
--- End diff --
alright that was a concern of mine, will remove it in next 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 issue #14201: [SPARK-14702] Make environment of SparkLauncher launched...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/14201
Merged build finished. 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 issue #14201: [SPARK-14702] Make environment of SparkLauncher launched...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/14201
**[Test build #62390 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/62390/consoleFull)** for PR 14201 at commit [`c017334`](https://github.com/apache/spark/commit/c0173345a2e50ccb6c97f199ab9906cf7e0c8a1e).
* This patch passes all 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 #14201: [SPARK-14702] Expose SparkLauncher's ProcessBuild...
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on a diff in the pull request:
https://github.com/apache/spark/pull/14201#discussion_r70869233
--- Diff: launcher/src/main/java/org/apache/spark/launcher/AbstractCommandBuilder.java ---
@@ -52,6 +52,11 @@
final List<String> pyFiles;
final Map<String, String> childEnv;
final Map<String, String> conf;
+ File workingDir;
--- End diff --
It feels weird to add these fields to this class when they're not used by this class at all. Feels like they should be in `SparkLauncher`.
---
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 issue #14201: [SPARK-14702] Expose SparkLauncher's ProcessBuilder for ...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/14201
Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/62332/
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 issue #14201: [SPARK-14702] Make environment of SparkLauncher launched...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/14201
**[Test build #62378 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/62378/consoleFull)** for PR 14201 at commit [`fa59b49`](https://github.com/apache/spark/commit/fa59b498a417ad51207314698e92c178c942dc5f).
* This patch passes all 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 issue #14201: [SPARK-14702] Make environment of SparkLauncher launched...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/14201
Merged build finished. 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 issue #14201: [SPARK-14702] Make environment of SparkLauncher launched...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/14201
Merged build finished. 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 issue #14201: [SPARK-14702] Make environment of SparkLauncher launched...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/14201
**[Test build #62390 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/62390/consoleFull)** for PR 14201 at commit [`c017334`](https://github.com/apache/spark/commit/c0173345a2e50ccb6c97f199ab9906cf7e0c8a1e).
---
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 issue #14201: [SPARK-14702] Make environment of SparkLauncher launched...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/14201
**[Test build #62481 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/62481/consoleFull)** for PR 14201 at commit [`bde3fa7`](https://github.com/apache/spark/commit/bde3fa72af5117398fe561f54e8dac2c8c996176).
* This patch passes all 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 #14201: [SPARK-14702] Make environment of SparkLauncher l...
Posted by andreweduffy <gi...@git.apache.org>.
Github user andreweduffy commented on a diff in the pull request:
https://github.com/apache/spark/pull/14201#discussion_r71036075
--- Diff: launcher/src/main/java/org/apache/spark/launcher/SparkLauncher.java ---
@@ -359,6 +369,83 @@ public SparkLauncher setVerbose(boolean verbose) {
}
/**
+ * Sets the working directory of spark-submit.
+ *
+ * @param dir The directory to set as spark-submit's working directory.
+ * @return This launcher.
+ */
+ public SparkLauncher directory(File dir) {
+ workingDir = dir;
+ return this;
+ }
+
+ /**
+ * Specifies that stderr in spark-submit should be redirected to stdout.
+ *
+ * @return This launcher.
+ */
+ public SparkLauncher redirectError() {
+ redirectErrorStream = true;
+ return this;
+ }
+
+ /**
+ * Redirects error output to the specified Redirect.
+ *
+ * @param to The method of redirection.
+ * @return This launcher.
+ */
+ public SparkLauncher redirectError(ProcessBuilder.Redirect to) {
+ errorStream = to;
+ return this;
+ }
+
+ /**
+ * Redirects standard output to the specified Redirect.
+ *
+ * @param to The method of redirection.
+ * @return This launcher.
+ */
+ public SparkLauncher redirectOutput(ProcessBuilder.Redirect to) {
+ outputStream = to;
+ return this;
+ }
+
+ /**
+ * Redirects error output to the specified File.
+ *
+ * @param errFile The file to which stderr is written.
+ * @return This launcher.
+ */
+ public SparkLauncher redirectError(File errFile) {
+ errorStream = ProcessBuilder.Redirect.to(errFile);
+ return this;
+ }
+
+ /**
+ * Redirects error output to the specified File.
+ *
+ * @param outFile The file to which stdout is written.
+ * @return This launcher.
+ */
+ public SparkLauncher redirectOutput(File outFile) {
+ outputStream = ProcessBuilder.Redirect.to(outFile);
+ return this;
+ }
+
+ /**
+ * Sets all output to be logged and redirected to a logger with the specified name.
+ *
+ * @param loggerName The name of the logger to log stdout and stderr.
+ * @return This launcher.
+ */
+ public SparkLauncher redirectToLog(String loggerName) {
--- End diff --
Yep, that's what I'm saying with the second point above, so those two should probably be good enough
---
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 issue #14201: [SPARK-14702] Make environment of SparkLauncher launched...
Posted by andreweduffy <gi...@git.apache.org>.
Github user andreweduffy commented on the issue:
https://github.com/apache/spark/pull/14201
Thanks @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 #14201: [SPARK-14702] Make environment of SparkLauncher l...
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on a diff in the pull request:
https://github.com/apache/spark/pull/14201#discussion_r70912543
--- Diff: launcher/src/main/java/org/apache/spark/launcher/SparkLauncher.java ---
@@ -82,8 +83,12 @@
/** Used internally to create unique logger names. */
private static final AtomicInteger COUNTER = new AtomicInteger();
+ public static final ThreadFactory REDIRECTOR_FACTORY = new NamedThreadFactory("launcher-proc-%d");
--- End diff --
It doesn't need to be public. Package private (a.k.a. no modifier) is enough.
---
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 #14201: [SPARK-14702] Expose SparkLauncher's ProcessBuild...
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on a diff in the pull request:
https://github.com/apache/spark/pull/14201#discussion_r70869756
--- Diff: launcher/src/main/java/org/apache/spark/launcher/SparkLauncher.java ---
@@ -82,8 +83,12 @@
/** Used internally to create unique logger names. */
private static final AtomicInteger COUNTER = new AtomicInteger();
+ public static final ThreadFactory REDIRECTOR_FACTORY = new NamedThreadFactory("launcher-proc-%d");
+
static final Map<String, String> launcherConfig = new HashMap<>();
+ private OutputRedirector redirector; // Holds the possible OutputRedirector
--- End diff --
There's probably no need to keep this reference. `OutputRedirector` starts a thread so it won't be garbage collected.
---
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 issue #14201: [SPARK-14702] Make environment of SparkLauncher launched...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/14201
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/62390/
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 issue #14201: [SPARK-14702] Expose SparkLauncher's ProcessBuilder for ...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/14201
**[Test build #62332 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/62332/consoleFull)** for PR 14201 at commit [`5a35963`](https://github.com/apache/spark/commit/5a35963bc4661193d8c169c7f98fffe24b181124).
* This patch **fails to build**.
* 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 issue #14201: [SPARK-14702] Expose SparkLauncher's ProcessBuilder for ...
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on the issue:
https://github.com/apache/spark/pull/14201
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