You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by WangTaoTheTonic <gi...@git.apache.org> on 2014/12/02 16:10:18 UTC

[GitHub] spark pull request: [SPARK-4697]System properties should override ...

GitHub user WangTaoTheTonic opened a pull request:

    https://github.com/apache/spark/pull/3557

    [SPARK-4697]System properties should override environment variables

    I found some arguments in yarn module take environment variables before system properties while the latter override the former in core module.

You can merge this pull request into a Git repository by running:

    $ git pull https://github.com/WangTaoTheTonic/spark SPARK4697

Alternatively you can review and apply these changes as the patch at:

    https://github.com/apache/spark/pull/3557.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

    This closes #3557
    
----
commit 5f43f453df19c1b281aa0a0e74d06b543a6e2d95
Author: WangTao <ba...@aliyun.com>
Date:   2014-12-02T15:02:21Z

    System property can override environment variable

----


---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [SPARK-4697][YARN]System properties should ove...

Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on a diff in the pull request:

    https://github.com/apache/spark/pull/3557#discussion_r21328217
  
    --- Diff: yarn/common/src/main/scala/org/apache/spark/scheduler/cluster/YarnClientSchedulerBackend.scala ---
    @@ -79,10 +79,10 @@ private[spark] class YarnClientSchedulerBackend(
             ("--name", "SPARK_YARN_APP_NAME", "spark.app.name")
           )
         optionTuples.foreach { case (optionName, envVar, sparkProp) =>
    -      if (System.getenv(envVar) != null) {
    -        extraArgs += (optionName, System.getenv(envVar))
    -      } else if (sc.getConf.contains(sparkProp)) {
    +      if (sc.getConf.contains(sparkProp)) {
             extraArgs += (optionName, sc.getConf.get(sparkProp))
    +      } else if (System.getenv(envVar) != null) {
    +        extraArgs += (optionName, System.getenv(envVar))
    --- End diff --
    
    The method you're modifying (`getExtraClientArguments`) is the one that defines the `--name` argument for `ClientArguments`. And you're inverting the priority here, so that `spark.app.name` > `SPARK_YARN_APP_NAME`. So basically, since `spark.app.name` is mandatory, `SPARK_YARN_APP_NAME` becomes useless.
    
    But please test it; make sure both work, both in client and cluster mode. Something might have changed since those fixes went in, although I kinda doubt it.


---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [SPARK-4697][YARN]System properties should ove...

Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:

    https://github.com/apache/spark/pull/3557#issuecomment-65372587
  
      [Test build #24082 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/24082/consoleFull) for   PR 3557 at commit [`40934b4`](https://github.com/apache/spark/commit/40934b45e6c3c820321cd721b18844f4b90fda1b).
     * This patch merges cleanly.


---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [SPARK-4697]System properties should override ...

Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:

    https://github.com/apache/spark/pull/3557#issuecomment-65259859
  
    Test PASSed.
    Refer to this link for build results (access rights to CI server needed): 
    https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/24045/
    Test PASSed.


---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [SPARK-4697][YARN]System properties should ove...

Posted by WangTaoTheTonic <gi...@git.apache.org>.
Github user WangTaoTheTonic commented on the pull request:

    https://github.com/apache/spark/pull/3557#issuecomment-69282649
  
    @vanzin
    Note what I note :-)
    
    >Note: In test cases I didn't use SparkConf.setAppName in application code.


---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [SPARK-4697][YARN]System properties should ove...

Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:

    https://github.com/apache/spark/pull/3557#issuecomment-69534797
  
      [Test build #25402 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/25402/consoleFull) for   PR 3557 at commit [`836b9ef`](https://github.com/apache/spark/commit/836b9ef13ef442109ba978da23309f7679405e2f).
     * This patch merges cleanly.


---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [SPARK-4697][YARN]System properties should ove...

Posted by WangTaoTheTonic <gi...@git.apache.org>.
Github user WangTaoTheTonic commented on a diff in the pull request:

    https://github.com/apache/spark/pull/3557#discussion_r21218022
  
    --- Diff: yarn/common/src/main/scala/org/apache/spark/scheduler/cluster/YarnClientSchedulerBackend.scala ---
    @@ -79,11 +79,8 @@ private[spark] class YarnClientSchedulerBackend(
             ("--name", "SPARK_YARN_APP_NAME", "spark.app.name")
           )
         optionTuples.foreach { case (optionName, envVar, sparkProp) =>
    -      if (System.getenv(envVar) != null) {
    --- End diff --
    
    Good point, that sounds more efficient.


---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [SPARK-4697][YARN]System properties should ove...

Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:

    https://github.com/apache/spark/pull/3557#issuecomment-69538383
  
      [Test build #25402 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/25402/consoleFull) for   PR 3557 at commit [`836b9ef`](https://github.com/apache/spark/commit/836b9ef13ef442109ba978da23309f7679405e2f).
     * This patch **passes all tests**.
     * This patch merges cleanly.
     * This patch adds no public classes.


---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [SPARK-4697][YARN]System properties should ove...

Posted by tgravescs <gi...@git.apache.org>.
Github user tgravescs commented on a diff in the pull request:

    https://github.com/apache/spark/pull/3557#discussion_r21232550
  
    --- Diff: yarn/common/src/main/scala/org/apache/spark/scheduler/cluster/YarnClientSchedulerBackend.scala ---
    @@ -79,11 +79,8 @@ private[spark] class YarnClientSchedulerBackend(
             ("--name", "SPARK_YARN_APP_NAME", "spark.app.name")
           )
         optionTuples.foreach { case (optionName, envVar, sparkProp) =>
    -      if (System.getenv(envVar) != null) {
    --- End diff --
    
    So this order was done on purpose. There were a few issues  if they are the other way.
    
    See commit https://github.com/apache/spark/commit/3f779d872d8459b262b3db9e4d12b011910b6ce9 and associated jira https://issues.apache.org/jira/browse/SPARK-1631.
    
    cc @vanzin
    
    I know this was brought up once before but I don't know if anyone fully investigated to see if there is another solution


---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [SPARK-4697]System properties should override ...

Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:

    https://github.com/apache/spark/pull/3557#issuecomment-65259844
  
      [Test build #24045 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/24045/consoleFull) for   PR 3557 at commit [`5f43f45`](https://github.com/apache/spark/commit/5f43f453df19c1b281aa0a0e74d06b543a6e2d95).
     * This patch **passes all tests**.
     * This patch merges cleanly.
     * This patch adds no public classes.


---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [SPARK-4697][YARN]System properties should ove...

Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on the pull request:

    https://github.com/apache/spark/pull/3557#issuecomment-69372058
  
    I see. Still, the third line of the "before patch" table is unexpected.


---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [SPARK-4697][YARN]System properties should ove...

Posted by WangTaoTheTonic <gi...@git.apache.org>.
Github user WangTaoTheTonic commented on the pull request:

    https://github.com/apache/spark/pull/3557#issuecomment-69431870
  
    Oh, I see. But after this patch `SPARK_YARN_APP_NAME` becomes useless. It will make behavior in client and cluster mode to be same.
    Note that happens when we don't set app name in SparkConf. Otherwise it is a different issue  described in [SPARK-3678](https://issues.apache.org/jira/browse/SPARK-3678). Perhaps we should file another separate PR to solve that.


---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [SPARK-4697][YARN]System properties should ove...

Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on the pull request:

    https://github.com/apache/spark/pull/3557#issuecomment-69431972
  
    >  But after this patch SPARK_YARN_APP_NAME becomes useless
    
    That's why we should not commit this patch.


---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [SPARK-4697][YARN]System properties should ove...

Posted by WangTaoTheTonic <gi...@git.apache.org>.
Github user WangTaoTheTonic commented on the pull request:

    https://github.com/apache/spark/pull/3557#issuecomment-69430615
  
    I am afraid it is not.
    In cluster mode, in `SparkSubmitArguments.scala` will be assigned with `mainClass` if `spark.app.name` or `--name` is not specified as it will not read `SPARK_YARN_APP_NAME`.
    Then in `SparkSubmit.scala` it will pass `args.name` in format of `spark.app.name` and `--name`  to `org.apache.spark.deploy.yarn.Client`.
    
     `yarn.Client.scala` transform the args to a ClientArguments object, in which `appName` will only get  its value from `--name`.
    
    So, in the progress, the app name would never get value from the env `SPARK_YARN_APP_NAME`. It is only used in client mode.


---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [SPARK-4697][YARN]System properties should ove...

Posted by tgravescs <gi...@git.apache.org>.
Github user tgravescs commented on the pull request:

    https://github.com/apache/spark/pull/3557#issuecomment-69578562
  
    So it used to be (0.8 and 0.9) that spark on yarn was ran like this.  We kept this work in the 1.0 release as well for backwards compatibility.
    
    spark-class org.apache.spark.deploy.yarn.Client \
            --jar spark-examples.jar \
            --class org.apache.spark.examples.SparkPi --args yarn-standalone --num-workers 3 --worker-memory 2g --queue unfunded
    
    does moving those to spark submit break the app name for this?   I don't know of anyone still using that but if we break it we should consciously decide that is unsupported.


---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [SPARK-4697][YARN]System properties should ove...

Posted by tgravescs <gi...@git.apache.org>.
Github user tgravescs commented on the pull request:

    https://github.com/apache/spark/pull/3557#issuecomment-65632939
  
    Did you test it to see if they still work?


---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [SPARK-4697][YARN]System properties should ove...

Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:

    https://github.com/apache/spark/pull/3557#issuecomment-69538386
  
    Test PASSed.
    Refer to this link for build results (access rights to CI server needed): 
    https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/25402/
    Test PASSed.


---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [SPARK-4697][YARN]System properties should ove...

Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:

    https://github.com/apache/spark/pull/3557#issuecomment-65383144
  
    Test PASSed.
    Refer to this link for build results (access rights to CI server needed): 
    https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/24082/
    Test PASSed.


---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [SPARK-4697][YARN]System properties should ove...

Posted by tgravescs <gi...@git.apache.org>.
Github user tgravescs commented on the pull request:

    https://github.com/apache/spark/pull/3557#issuecomment-69033274
  
    @WangTaoTheTonic did you have a chance to test this?  


---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [SPARK-4697][YARN]System properties should ove...

Posted by WangTaoTheTonic <gi...@git.apache.org>.
Github user WangTaoTheTonic commented on the pull request:

    https://github.com/apache/spark/pull/3557#issuecomment-69534968
  
    @andrewor14 @vanzin 
    After moving the logic to `SparkSubmitArguments`, I tested with no `SparkConf.setAppName` in application code, here is the results:
    
    Setting | name in client mode | name in cluster mode
    --------- | ------------------------- | ---------------------------
    none | org.apache.spark.examples.SparkPi | org.apache.spark.examples.SparkPi
    spark.app.name | spark.app.name | spark.app.name
    SPARK_YARN_APP_NAME | SPARK_YARN_APP_NAME | SPARK_YARN_APP_NAME 
    spark.app.name and SPARK_YARN_APP_NAME | spark.app.name | spark.app.name
    
    The names on Driver's UI and RM's UI is consistent.


---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [SPARK-4697][YARN]System properties should ove...

Posted by sryza <gi...@git.apache.org>.
Github user sryza commented on the pull request:

    https://github.com/apache/spark/pull/3557#issuecomment-69642800
  
    I'm strongly in favor of deprecating the env vars as well.
    
    Until we do that, keeping the semantics consistent (and special casing if need be) seems reasonable to me.


---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [SPARK-4697][YARN]System properties should ove...

Posted by WangTaoTheTonic <gi...@git.apache.org>.
Github user WangTaoTheTonic commented on the pull request:

    https://github.com/apache/spark/pull/3557#issuecomment-69426504
  
    @vanzin  Which one do you mean? Client mode or Cluster mode?
    @tgravescs I looked the name on RM's UI.
    I checked SPARK-3678 and realized that if no `spark.app.name` in configuration file or `--name` in command args, in cluster mode it will use `mainClass`.
    But in client mode, cause usually we use `SparkConf.setAppName` in application code, so on RM's UI it will show what we set. 


---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [SPARK-4697][YARN]System properties should ove...

Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on the pull request:

    https://github.com/apache/spark/pull/3557#issuecomment-69625652
  
    Personally, since there have been 3 stable releases already that have the current semantics, and we effectively treat env variables as deprecated, I'd just not change anything, document that env variables are deprecated, and remove them in a future release (1.4 or later).
    
    But if you guys really think this is worth doing, the latest patch looks ok, I guess.


---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [SPARK-4697][YARN]System properties should ove...

Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:

    https://github.com/apache/spark/pull/3557#issuecomment-65383135
  
      [Test build #24082 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/24082/consoleFull) for   PR 3557 at commit [`40934b4`](https://github.com/apache/spark/commit/40934b45e6c3c820321cd721b18844f4b90fda1b).
     * This patch **passes all tests**.
     * This patch merges cleanly.
     * This patch adds no public classes.


---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [SPARK-4697][YARN]System properties should ove...

Posted by andrewor14 <gi...@git.apache.org>.
Github user andrewor14 commented on a diff in the pull request:

    https://github.com/apache/spark/pull/3557#discussion_r22878597
  
    --- Diff: yarn/src/main/scala/org/apache/spark/scheduler/cluster/YarnClientSchedulerBackend.scala ---
    @@ -88,18 +87,19 @@ private[spark] class YarnClientSchedulerBackend(
         // Do the same for deprecated properties: property -> suggestion
         val deprecatedProps = Map("spark.master.memory" -> "--driver-memory through spark-submit")
         optionTuples.foreach { case (optionName, envVar, sparkProp) =>
    -      if (System.getenv(envVar) != null) {
    -        extraArgs += (optionName, System.getenv(envVar))
    -        if (deprecatedEnvVars.contains(envVar)) {
    -          logWarning(s"NOTE: $envVar is deprecated. Use ${deprecatedEnvVars(envVar)} instead.")
    -        }
    -      } else if (sc.getConf.contains(sparkProp)) {
    +      if (sc.getConf.contains(sparkProp)) {
             extraArgs += (optionName, sc.getConf.get(sparkProp))
             if (deprecatedProps.contains(sparkProp)) {
               logWarning(s"NOTE: $sparkProp is deprecated. Use ${deprecatedProps(sparkProp)} instead.")
             }
    +      } else if (System.getenv(envVar) != null) {
    +        extraArgs += (optionName, System.getenv(envVar))
    +        if (deprecatedEnvVars.contains(envVar)) {
    +          logWarning(s"NOTE: $envVar is deprecated. Use ${deprecatedEnvVars(envVar)} instead.")
    +        }
           }
         }
    +    sc.getConf.getOption("spark.app.name").foreach(v => extraArgs += ("--name", v))
    --- End diff --
    
    We need to add a comment here to explain why it's a special case


---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [SPARK-4697][YARN]System properties should ove...

Posted by andrewor14 <gi...@git.apache.org>.
Github user andrewor14 commented on a diff in the pull request:

    https://github.com/apache/spark/pull/3557#discussion_r22878529
  
    --- Diff: core/src/main/scala/org/apache/spark/deploy/SparkSubmitArguments.scala ---
    @@ -149,6 +149,10 @@ private[spark] class SparkSubmitArguments(args: Seq[String], env: Map[String, St
         // Global defaults. These should be keep to minimum to avoid confusing behavior.
         master = Option(master).getOrElse("local[*]")
     
    +    // In yarn mode, app name can be set via SPARK_YARN_APP_NAME
    +    if (master.contains("yarn")) {
    --- End diff --
    
    should be `startsWith`, also in the comments we generally capitalize YARN


---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [SPARK-4697][YARN]System properties should ove...

Posted by WangTaoTheTonic <gi...@git.apache.org>.
Github user WangTaoTheTonic commented on the pull request:

    https://github.com/apache/spark/pull/3557#issuecomment-69177980
  
    I've tested it and results are:
    Before this patch:
    Setting|name in client mode|name in cluster mode
    ---------|-------------------------|---------------------------
    none|org.apache.spark.examples.SparkPi|org.apache.spark.examples.SparkPi
    spark.app.name|spark.app.name|spark.app.name
    SPARK_YARN_APP_NAME|SPARK_YARN_APP_NAME|org.apache.spark.examples.SparkPi
    spark.app.name and SPARK_YARN_APP_NAME|SPARK_YARN_APP_NAME|spark.app.name
    
    After this patch:
    Setting|name in client mode|name in cluster mode
    ---------|-------------------------|---------------------------
    none|org.apache.spark.examples.SparkPi|org.apache.spark.examples.SparkPi
    spark.app.name|spark.app.name|spark.app.name
    SPARK_YARN_APP_NAME|org.apache.spark.examples.SparkPi|org.apache.spark.examples.SparkPi
    spark.app.name and SPARK_YARN_APP_NAME|spark.app.name|spark.app.name
    
    As @vanzin said, the `SPARK_YARN_APP_NAME` becomes useless since `spark.app.name` is mandatory.
    
    Note: In test cases I didn't use `SparkConf.setAppName` in application code.


---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [SPARK-4697][YARN]System properties should ove...

Posted by sryza <gi...@git.apache.org>.
Github user sryza commented on a diff in the pull request:

    https://github.com/apache/spark/pull/3557#discussion_r22878622
  
    --- Diff: core/src/main/scala/org/apache/spark/deploy/SparkSubmitArguments.scala ---
    @@ -149,6 +149,10 @@ private[spark] class SparkSubmitArguments(args: Seq[String], env: Map[String, St
         // Global defaults. These should be keep to minimum to avoid confusing behavior.
         master = Option(master).getOrElse("local[*]")
     
    +    // In yarn mode, app name can be set via SPARK_YARN_APP_NAME
    +    if (master.contains("yarn")) {
    --- End diff --
    
    Woops didn't see your wrote comments


---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [SPARK-4697][YARN]System properties should ove...

Posted by andrewor14 <gi...@git.apache.org>.
Github user andrewor14 commented on the pull request:

    https://github.com/apache/spark/pull/3557#issuecomment-69634420
  
    @vanzin I'm not sure if it's on our roadmap to deprecate the environment variables altogether, since there are many others for other modes that are being actively used. That said I would also prefer to remove them eventually, but until then we should keep the semantics in YARN consistent with other modes (i.e. system properties should override env variables). Unfortunately we can't do anything about the app name but to special case it.
    
    @sryza @tgravescs any thoughts?


---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [SPARK-4697][YARN]System properties should ove...

Posted by tgravescs <gi...@git.apache.org>.
Github user tgravescs commented on the pull request:

    https://github.com/apache/spark/pull/3557#issuecomment-69378505
  
    where were you looking to get the application name?  On the RM or in spark or both? 
    
    https://issues.apache.org/jira/browse/SPARK-3678


---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [SPARK-4697][YARN]System properties should ove...

Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:

    https://github.com/apache/spark/pull/3557#issuecomment-69177938
  
      [Test build #25224 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/25224/consoleFull) for   PR 3557 at commit [`bee9447`](https://github.com/apache/spark/commit/bee944746bf8615fbcddfe83f13a988dbc215511).
     * This patch **passes all tests**.
     * This patch merges cleanly.
     * This patch adds no public classes.


---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [SPARK-4697][YARN]System properties should ove...

Posted by andrewor14 <gi...@git.apache.org>.
Github user andrewor14 commented on the pull request:

    https://github.com/apache/spark/pull/3557#issuecomment-69432664
  
    Although in general we should honor Spark properties over environment variables, the app name has been a special case and should remain so for backward compatibility. For this PR, I think the goal is to maintain behavior in the "before" table by making more changes in `YarnClientSchedulerBackend`.
    
    Additionally, it is not intuitive that if you set both `SPARK_YARN_APP_NAME` and `spark.app.name`, the behavior is inconsistent between client mode and cluster mode. I think the app name should be a special case for both deploy modes, but we can fix that in a separate PR.


---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [SPARK-4697][YARN]System properties should ove...

Posted by andrewor14 <gi...@git.apache.org>.
Github user andrewor14 commented on the pull request:

    https://github.com/apache/spark/pull/3557#issuecomment-69784969
  
    Hey @WangTaoTheTonic I think this LGTM. By the way, when I merge this I will file a new JIRA that makes the app name behavior consistent across cluster and client modes and add it to the commit title. I'll also fix the few minor comments I left.


---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [SPARK-4697][YARN]System properties should ove...

Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on the pull request:

    https://github.com/apache/spark/pull/3557#issuecomment-69427688
  
    I mean it's unexpected because they're different when they should be the same (in that case, the value of `SPARK_YARN_APP_NAME`).


---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [SPARK-4697][YARN]System properties should ove...

Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on the pull request:

    https://github.com/apache/spark/pull/3557#issuecomment-69430819
  
    I understand all that. I'm saying that's unexpected, in that I'd expect both modes to behave the same. So if there's anything to fix here, that's it.


---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [SPARK-4697][YARN]System properties should ove...

Posted by WangTaoTheTonic <gi...@git.apache.org>.
Github user WangTaoTheTonic commented on the pull request:

    https://github.com/apache/spark/pull/3557#issuecomment-69586378
  
    I am afraid in that case user can only set app name via `spark.app.name` or `--name`, `SPARK_YARN_APP_NAME` will not be used. 



---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [SPARK-4697][YARN]System properties should ove...

Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:

    https://github.com/apache/spark/pull/3557


---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [SPARK-4697][YARN]System properties should ove...

Posted by tgravescs <gi...@git.apache.org>.
Github user tgravescs commented on the pull request:

    https://github.com/apache/spark/pull/3557#issuecomment-69783065
  
    I'm in favor of deprecating too but I don't think its good to do it in minor release.  Probably 2.0 would be good time.
    
    Lets just leave the app name special case.  


---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [SPARK-4697][YARN]System properties should ove...

Posted by andrewor14 <gi...@git.apache.org>.
Github user andrewor14 commented on a diff in the pull request:

    https://github.com/apache/spark/pull/3557#discussion_r22878615
  
    --- Diff: core/src/main/scala/org/apache/spark/deploy/SparkSubmitArguments.scala ---
    @@ -149,6 +149,10 @@ private[spark] class SparkSubmitArguments(args: Seq[String], env: Map[String, St
         // Global defaults. These should be keep to minimum to avoid confusing behavior.
         master = Option(master).getOrElse("local[*]")
     
    +    // In yarn mode, app name can be set via SPARK_YARN_APP_NAME
    +    if (master.contains("yarn")) {
    --- End diff --
    
    yeah


---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [SPARK-4697][YARN]System properties should ove...

Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:

    https://github.com/apache/spark/pull/3557#issuecomment-69171546
  
      [Test build #25224 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/25224/consoleFull) for   PR 3557 at commit [`bee9447`](https://github.com/apache/spark/commit/bee944746bf8615fbcddfe83f13a988dbc215511).
     * This patch merges cleanly.


---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [SPARK-4697][YARN]System properties should ove...

Posted by sryza <gi...@git.apache.org>.
Github user sryza commented on the pull request:

    https://github.com/apache/spark/pull/3557#issuecomment-65343033
  
    @andrewor14 @tgravescs


---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [SPARK-4697][YARN]System properties should ove...

Posted by WangTaoTheTonic <gi...@git.apache.org>.
Github user WangTaoTheTonic commented on the pull request:

    https://github.com/apache/spark/pull/3557#issuecomment-69432493
  
    Or we could just make SPARK_YARN_APP_NAME disappear? As the env variable is not recommended and it will cause different behavior. User can still use `spark.app.name`.
    
    Or we should make SPARK_YARN_APP_NAME and spark.app.name a special case in `YarnClientSchedulerBackend.scala` ?
    
    What do you two think? @tgravescs @andrewor14 


---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [SPARK-4697]System properties should override ...

Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:

    https://github.com/apache/spark/pull/3557#issuecomment-65245329
  
      [Test build #24045 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/24045/consoleFull) for   PR 3557 at commit [`5f43f45`](https://github.com/apache/spark/commit/5f43f453df19c1b281aa0a0e74d06b543a6e2d95).
     * This patch merges cleanly.


---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [SPARK-4697][YARN]System properties should ove...

Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on the pull request:

    https://github.com/apache/spark/pull/3557#issuecomment-69218980
  
    Actually that's a little bit surprising. SparkPi.scala sets the app name to "Spark Pi", so I'd expect that to show up when the user does not explicitly override it. Also, there seems to be some disprepancies between client and cluster mode in handling the env variable.


---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [SPARK-4697][YARN]System properties should ove...

Posted by sryza <gi...@git.apache.org>.
Github user sryza commented on a diff in the pull request:

    https://github.com/apache/spark/pull/3557#discussion_r22878585
  
    --- Diff: core/src/main/scala/org/apache/spark/deploy/SparkSubmitArguments.scala ---
    @@ -149,6 +149,10 @@ private[spark] class SparkSubmitArguments(args: Seq[String], env: Map[String, St
         // Global defaults. These should be keep to minimum to avoid confusing behavior.
         master = Option(master).getOrElse("local[*]")
     
    +    // In yarn mode, app name can be set via SPARK_YARN_APP_NAME
    +    if (master.contains("yarn")) {
    --- End diff --
    
    Just to be clear, you mean capitalize YARN in the comment, right?


---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [SPARK-4697][YARN]System properties should ove...

Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:

    https://github.com/apache/spark/pull/3557#issuecomment-69177950
  
    Test PASSed.
    Refer to this link for build results (access rights to CI server needed): 
    https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/25224/
    Test PASSed.


---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [SPARK-4697][YARN]System properties should ove...

Posted by sryza <gi...@git.apache.org>.
Github user sryza commented on a diff in the pull request:

    https://github.com/apache/spark/pull/3557#discussion_r21209175
  
    --- Diff: yarn/common/src/main/scala/org/apache/spark/scheduler/cluster/YarnClientSchedulerBackend.scala ---
    @@ -79,11 +79,8 @@ private[spark] class YarnClientSchedulerBackend(
             ("--name", "SPARK_YARN_APP_NAME", "spark.app.name")
           )
         optionTuples.foreach { case (optionName, envVar, sparkProp) =>
    -      if (System.getenv(envVar) != null) {
    --- End diff --
    
    Can we maintain the structure here and just switch the blocks?


---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [SPARK-4697][YARN]System properties should ove...

Posted by sryza <gi...@git.apache.org>.
Github user sryza commented on the pull request:

    https://github.com/apache/spark/pull/3557#issuecomment-65342974
  
    Had one nit, otherwise this change looks right to me.


---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [SPARK-4697][YARN]System properties should ove...

Posted by WangTaoTheTonic <gi...@git.apache.org>.
Github user WangTaoTheTonic commented on the pull request:

    https://github.com/apache/spark/pull/3557#issuecomment-65644328
  
    @tgravescs Ok if you mean whether the app name could still be shown correctly, I will test it.


---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org