spark-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From vanzin <...@git.apache.org>
Subject [GitHub] spark pull request #14185: [SPARK-16511][SUBMIT] Expose SparkLauncher's Proc...
Date Thu, 14 Jul 2016 00:18:51 GMT
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 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


Mime
View raw message