Github user JoshRosen commented on the pull request:
https://github.com/apache/spark/pull/2432#issuecomment-57099001
Hey @sarutak,
Thanks for the most recent set of changes. I think this is really close to
being ready to merge.
I like your approach of having the application client send only the event
log parent directory instead of the full log path, since both the application
client and Master agree on how the full log path will be formed from the base
path + application id.
I left a couple of comments about the ApplicationId class. In general, I'm
a fan of more explicitly-typed ids like this, since they can improve
readability and help to avoid errors (like reversing the order of two adjacent
String arguments by mistake). I think we should expose ApplicationId as a
public Java class in order to make ApplicationIds easier to construct / use
from Java and through Py4J. Although the introduction of ApplicationId changes
some of the event listener APIs, I think it's fine since those APIs are
`@developerAPI` and already aren't stable across Spark releases.
After we fix this and the MiMa issue, I'll take a final review pass;
otherwise, this looks good to me. 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 [email protected] or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
