Github user vanzin commented on a diff in the pull request:

    https://github.com/apache/spark/pull/15009#discussion_r108780813
  
    --- Diff: core/src/main/scala/org/apache/spark/deploy/SparkSubmit.scala ---
    @@ -735,7 +749,12 @@ object SparkSubmit extends CommandLineUtils {
         }
     
         try {
    -      mainMethod.invoke(null, childArgs.toArray)
    +      if (isSparkApp) {
    +        val envvars = Map[String, String]() ++ sys.env
    +        mainMethod.invoke(null, childArgs.toArray, childSparkConf, 
envvars.toMap)
    --- End diff --
    
    My comment is not exactly about Spark's usage of environment variables or 
not. It's about the code being added here not serving any purpose.
    
    ```
    val envvars = Map[String, String]() ++ sys.env
    ```
    
    That means that regardless of the `SparkLauncher` configuration, you'll 
always be using the process's environment. So this whole exercise in adding an 
argument to specify the env map is futile.
    
    So there are two options here:
    
    - actually pass env variables customized by `SparkLauncher` here. this is 
not happening currently.
    - remove all this code and declare that "the env map passed to the 
`SparkLauncher` constructor has no effect when running Spark in the same 
process".
    
    I'm fine with either of those. But currently the PR is adding a bunch of 
dead code that is just not necessary.



---
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]

Reply via email to