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

Reply via email to