Github user andrewor14 commented on the pull request:
https://github.com/apache/spark/pull/2711#issuecomment-60807773
Hey @witgo thanks for the refactor. I find the new interface much less
confusing, i.e. if the developer accidentally uses `command.environment` he/she
won't leave out the library path. However, I think the existing code still has
a potential source of confusion. There is a `buildCommandSeq` and a
`buildLocalCommand`, and it's not intuitive at all which on we should use. It
seems that your code frequently just calls one after the other, and I think we
can just combine these into one function to simplify things. Other than that I
left a few naming and comment suggestions.
I think we shouldn't block on Windows for this patch. AFAIK very few people
run Spark on Windows on Yarn, and they probably run into other existing
problems setting it up before trying to set library paths anyway. That said
this is something we should fix this in the future.
Also, could you remove `[WIP]` from the title? I think this is pretty close
to being merged.
---
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]
