Github user kanzhang commented on the pull request:
https://github.com/apache/spark/pull/6676#issuecomment-111321235
Thanks. So it's not just 3 lines. :-) Honestly, if you finish it up and add
similar tests as I did, you will end up with less but not substantially less
code. Apart from a few straight-forward convenience methods in SparkConf and
Utils, the rest of my non-test changes are minimal per file or cosmetic. Most
of the patch is either tests or refactoring of code chunks into their own
methods to support testing (e.g., in `Client`, `SparkDeploySchedulerBackend`).
I found a simpler way to fix it if I don't introduce the concept of per-app
key, which is to do all the filtering and setting env variables in
`buildLocalCommand`. Thank you for bringing it to my attention earlier. It
allows me handle all 3 cases in one place. I just need one set of tests instead
of 3, and no code refactoring is needed for testing. I'll submit a new PR. You
are welcome to review it.
Let's leave this PR for collecting feedbacks on my stdin approach. I admit
I'm guilty of trying to squeeze in more code than necessary to fix the issue in
the current model (the `appSecret` stuff). But I was gearing toward per-app
secrets with my stdin approach.
---
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]
