Github user sryza commented on the pull request:
https://github.com/apache/spark/pull/3916#issuecomment-70005873
Just had a look through this. Overall, things look reasonable. Had a few
nits in the code and a few broader comments here:
Did you consider using
[ProcessBuilder](http://docs.oracle.com/javase/7/docs/api/java/lang/ProcessBuilder.html)
for building the commands in AbstractLauncher? It abstracts over the Linux
and Windows ways of launching processes and handles some of the stuff that it
looks like you're handling manually.
I think it's weird to expose constants for configuration property names in
LauncherCommon. If we want Spark to start exposing constants for these, we
should do it somewhere in core with a class name that's more specific. I'm
worried that people are going to start referring to these constants when
creating new SparkConfs, and it'll look weird to have them coming from
LauncherCommon.
---
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 [email protected] or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
