[GitHub] spark pull request #14185: [SPARK-16511][SUBMIT] Expose SparkLauncher's Proc...

2016-07-13 Thread vanzin 
Github user vanzin commented on a diff in the pull request:

https://github.com/apache/spark/pull/14185#discussion_r70731128
  
--- Diff: 
launcher/src/main/java/org/apache/spark/launcher/SparkLauncher.java ---
@@ -418,14 +414,26 @@ 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);
+ProcessBuilder pb = createProcessBuilder().redirectErrorStream(true);
+return startApplication(appName, pb, listeners);
+  }
+
+  public SparkAppHandle startApplication(String appName, ProcessBuilder pb,
+ SparkAppHandle.Listener... 
listeners) throws IOException {
+ChildProcAppHandle handle = LauncherServer.newAppHandle();
+for (SparkAppHandle.Listener l : listeners) {
+  handle.addListener(l);
+}
+
 pb.environment().put(LauncherProtocol.ENV_LAUNCHER_PORT,
   String.valueOf(LauncherServer.getServerInstance().getPort()));
 pb.environment().put(LauncherProtocol.ENV_LAUNCHER_SECRET, 
handle.getSecret());
+
+String loggerPrefix = getClass().getPackage().getName();
--- End diff --

There's an issue now that the user might call `.redirectOutput` on the 
`ProcessBuilder` and this code might break. So the log redirection really 
should only happen if the `ProcessBuilder` is configured to redirect to 
`ProcessBuilder.Redirect.PIPE`. And you'd need to make sure that 
`redirectErrorStream()` is set in that case.

An alternative approach would be to only do this log redirection from the 
existing `startApplication` call that does not take a `ProcessBuilder`. In that 
case, output of the child process for the new `startApplication` should be 
handled by the caller, just like for the old `launch()` API.

I really wish you could create new `ProcessBuilder.Redirect` values so that 
we could create a new `LOG` one and fix this properly, but it doesn't look like 
that's possible.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request #14185: [SPARK-16511][SUBMIT] Expose SparkLauncher's Proc...

2016-07-13 Thread vanzin 
Github user vanzin commented on a diff in the pull request:

https://github.com/apache/spark/pull/14185#discussion_r70730228
  
--- Diff: 
launcher/src/main/java/org/apache/spark/launcher/SparkLauncher.java ---
@@ -418,14 +414,26 @@ 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);
+ProcessBuilder pb = createProcessBuilder().redirectErrorStream(true);
+return startApplication(appName, pb, listeners);
+  }
+
+  public SparkAppHandle startApplication(String appName, ProcessBuilder pb,
+ SparkAppHandle.Listener... 
listeners) throws IOException {
+ChildProcAppHandle handle = LauncherServer.newAppHandle();
+for (SparkAppHandle.Listener l : listeners) {
+  handle.addListener(l);
+}
+
 pb.environment().put(LauncherProtocol.ENV_LAUNCHER_PORT,
   String.valueOf(LauncherServer.getServerInstance().getPort()));
 pb.environment().put(LauncherProtocol.ENV_LAUNCHER_SECRET, 
handle.getSecret());
+
+String loggerPrefix = getClass().getPackage().getName();
+String loggerName = String.format("%s.app.%s", loggerPrefix, appName);
 try {
-  handle.setChildProc(pb.start(), loggerName);
+  Process proc = pb.start();
--- End diff --

This change is not necessary.


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request #14185: [SPARK-16511][SUBMIT] Expose SparkLauncher's Proc...

2016-07-13 Thread vanzin 
Github user vanzin commented on a diff in the pull request:

https://github.com/apache/spark/pull/14185#discussion_r70729812
  
--- Diff: 
launcher/src/main/java/org/apache/spark/launcher/SparkLauncher.java ---
@@ -418,14 +414,26 @@ 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);
+ProcessBuilder pb = createProcessBuilder().redirectErrorStream(true);
+return startApplication(appName, pb, listeners);
+  }
+
+  public SparkAppHandle startApplication(String appName, ProcessBuilder pb,
--- End diff --

Style is:

```
public Type method(
Type arg1,
Type arg2,
...) {
```


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request #14185: [SPARK-16511][SUBMIT] Expose SparkLauncher's Proc...

2016-07-13 Thread vanzin 
Github user vanzin commented on a diff in the pull request:

https://github.com/apache/spark/pull/14185#discussion_r70729733
  
--- Diff: 
core/src/test/scala/org/apache/spark/launcher/LauncherBackendSuite.scala ---
@@ -17,15 +17,16 @@
 
 package org.apache.spark.launcher
 
+
--- End diff --

don't add this extra 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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request #14185: [SPARK-16511][SUBMIT] Expose SparkLauncher's Proc...

2016-07-13 Thread vanzin 
Github user vanzin commented on a diff in the pull request:

https://github.com/apache/spark/pull/14185#discussion_r70729726
  
--- Diff: 
core/src/test/scala/org/apache/spark/launcher/LauncherBackendSuite.scala ---
@@ -17,15 +17,16 @@
 
 package org.apache.spark.launcher
 
+
 import java.util.concurrent.TimeUnit
 
 import scala.concurrent.duration._
 import scala.language.postfixOps
 
-import org.scalatest.Matchers
 import org.scalatest.concurrent.Eventually._
+import org.scalatest.Matchers
--- End diff --

Please avoid making unnecessary changes to imports. This one, in 
particular, is wrong and would trigger a style checker error if the style 
checker didn't have a bug...


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request #14185: [SPARK-16511][SUBMIT] Expose SparkLauncher's Proc...

2016-07-13 Thread andreweduffy 
GitHub user andreweduffy opened a pull request:

https://github.com/apache/spark/pull/14185

[SPARK-16511][SUBMIT] Expose SparkLauncher's ProcessBuilder for user

## What changes were proposed in this pull request?

Expose the `ProcessBuilder` in `SparkLauncher`, as well as opening up 
`SparkLauncher#startApplication` to allow accepting arbitrary ProcessBuilders.

For more info see [https://issues.apache.org/jira/browse/SPARK-16511](this 
JIRA ticket)

Also covers [https://issues.apache.org/jira/browse/SPARK-14702](SPARK-14702)

## How was this patch tested?

Unit tests + spark shell




You can merge this pull request into a Git repository by running:

$ git pull https://github.com/andreweduffy/spark feature/processbuilder

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/spark/pull/14185.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 #14185


commit 7fe36f5970e7e577a47d8b6a7534cc95d22a94c2
Author: Andrew Duffy 
Date:   2016-07-13T14:20:54Z

[SPARK-16511][SUBMIT] Expose SparkLauncher's ProcessBuilder for user
flexibility

Also covers https://issues.apache.org/jira/browse/SPARK-16511




---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org