[SPARK-9672][MESOS] Don’t include SPARK_ENV_LOADED when passing env vars #7979
Conversation
|
/cc @tnachen and @andrewor14 for Mesos-related-patch triage. |
|
Can we not filter anything MESOS_ as well? Users set the MESOS_NATIVE_JAVA_LIBRARY in spark_env.sh and this could filter this out |
|
Thanks for the review @tnachen Anything that is in spark-env.sh shouldn't be filtered now that SPARK_ENV_LOADED isn't being sent (since it will be re-sourced on the driver side). I added some code to make sure MESOS_* vars are passed through anyways so that they could be set elsewhere. |
|
@tnachen any follow ups on this? |
|
This LGTM |
|
ok to test |
| /** | ||
| * Filter non-spark environment variables from any environment. | ||
| */ | ||
| def filterSystemEnvironment(env: Map[String, String]): Map[String, String] = { |
There was a problem hiding this comment.
please make this private
There was a problem hiding this comment.
There is a test for it, so I'd need to make it visible to the test somehow or remove the test.
I've changed it to protected[rest]. LMK if you have an alternate solution or would like me to make it private and remove the test.
|
Test build #41959 has finished for PR 7979 at commit
|
|
Thanks for the review @andrewor14. |
|
Test build #41966 has finished for PR 7979 at commit
|
| /** | ||
| * Filter non-spark environment variables from any environment. | ||
| */ | ||
| protected[rest] def filterSystemEnvironment(env: Map[String, String]): Map[String, String] = { |
There was a problem hiding this comment.
this can be private[rest], I've fixed that on merge.
There was a problem hiding this comment.
Thanks for helping to guide this through. Apologies for lack of Scala knowledge.
There was a problem hiding this comment.
no worries, hope to see more patches from you!
This contribution is my original work and I license the work to the project under the project's open source license.