You are viewing a plain text version of this content. The canonical link for it is here.

Posted to reviews@spark.apache.org by witgo <gi...@git.apache.org> on 2014/06/04 19:06:43 UTC

[GitHub] spark pull request: [WIP] In yarn.ClientBase spark.yarn.dist.* do ...

GitHub user witgo opened a pull request:

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

    [WIP] In yarn.ClientBase spark.yarn.dist.* do not work

    

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

    $ git pull https://github.com/witgo/spark yarn_ClientBase

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

    https://github.com/apache/spark/pull/969.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 #969
    
----
commit 836248956ff3ef17d44cea37b357e3616b054d64
Author: witgo <wi...@qq.com>
Date:   2014-06-04T16:50:12Z

    yarn.ClientBase spark.yarn.dist.* do not 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.
---

[GitHub] spark pull request: [SPARK-2051]In yarn.ClientBase spark.yarn.dist...

Posted by witgo <gi...@git.apache.org>.

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

    https://github.com/apache/spark/pull/969#discussion_r13738885
  
    --- Diff: yarn/common/src/main/scala/org/apache/spark/deploy/yarn/ClientBase.scala ---
    @@ -220,10 +220,21 @@ trait ClientBase extends Logging {
           }
         }
     
    +    def getArg(arg: String, envVar: String, sysProp: String): String = {
    +      if (arg != null && !arg.isEmpty) {
    +        arg
    +      } else if (System.getenv(envVar) != null && !System.getenv(envVar).isEmpty) {
    +        System.getenv(envVar)
    +      } else {
    +        sparkConf.getOption(sysProp).orNull
    +      }
    +    }
         var cachedSecondaryJarLinks = ListBuffer.empty[String]
    -    val fileLists = List( (args.addJars, LocalResourceType.FILE, true),
    -      (args.files, LocalResourceType.FILE, false),
    -      (args.archives, LocalResourceType.ARCHIVE, false) )
    +    val fileLists = List((args.addJars, LocalResourceType.FILE, true),
    +      (getArg(args.files, "SPARK_YARN_DIST_FILES", "spark.yarn.dist.files"),
    +        LocalResourceType.FILE, false),
    +      (getArg(args.archives, "SPARK_YARN_DIST_ARCHIVES", "spark.yarn.dist.archives"),
    --- End diff --
    
    I see, Weekend, I will test your branch, Please merge the master changes.


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

[GitHub] spark pull request: [SPARK-2051]In yarn.ClientBase spark.yarn.dist...

Posted by AmplabJenkins <gi...@git.apache.org>.

Github user AmplabJenkins commented on the pull request:

    https://github.com/apache/spark/pull/969#issuecomment-46030346
  
    Merged build finished. All automated tests 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.
---

[GitHub] spark pull request: [SPARK-2051]In yarn.ClientBase spark.yarn.dist...

Posted by AmplabJenkins <gi...@git.apache.org>.

Github user AmplabJenkins commented on the pull request:

    https://github.com/apache/spark/pull/969#issuecomment-45983110
  
     Merged build triggered. 


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

[GitHub] spark pull request: [SPARK-2051]In yarn.ClientBase spark.yarn.dist...

Posted by tgravescs <gi...@git.apache.org>.

Github user tgravescs commented on the pull request:

    https://github.com/apache/spark/pull/969#issuecomment-46009163
  
    Can you please be more specific on your description of this problem?  Does it only occur in yarn cluster mode (or is it both cluster and client mode)?  YarnClientSchedulerBackend I believe should handle loading that config in client mode so I believe you are only seeing the problem in yarn-cluster mode.
    
    I think the reason is that it was put in to replace the environment variables for client mode (SPARK_YARN_DIST_ARCHIVES) and also when spark-submit was added.  Originally there was no config for specifying this in yarn-cluster mode.  I'm definitely fine with adding that support but I think your jira should be changed to an improvement.  I also had just filed SPARK-2139 but I think that should just be combined with this and it documented here. 
    



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

[GitHub] spark pull request: [SPARK-2051]In yarn.ClientBase spark.yarn.dist...

Posted by tgravescs <gi...@git.apache.org>.

Github user tgravescs commented on the pull request:

    https://github.com/apache/spark/pull/969#issuecomment-46324631
  
    note I think it woudl be easiest just to have YarnClientSchedulerBackend ignore the configs spark.yarn.dist.archives/spark.yarn.dist.files  and only do the backwards compatibility for the ENV variables. Then your changes to clientArguments would handle calling resolveURI on them.


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

[GitHub] spark pull request: [SPARK-2051]In yarn.ClientBase spark.yarn.dist...

Posted by witgo <gi...@git.apache.org>.

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

    https://github.com/apache/spark/pull/969#discussion_r13777641
  
    --- Diff: yarn/common/src/main/scala/org/apache/spark/deploy/yarn/ClientArguments.scala ---
    @@ -45,6 +44,12 @@ class ClientArguments(val args: Array[String], val sparkConf: SparkConf) {
     
       parseArgs(args.toList)
     
    +  files = Option(files).getOrElse(sparkConf.getOption("spark.yarn.dist.files").orNull)
    +  files = Option(files).map(p => Utils.resolveURIs(p)).orNull
    --- End diff --
    
      files = Option(files).getOrElse(sparkConf.getOption("spark.yarn.dist.files").
        map(p => Utils.resolveURIs(p)).orNull)
    As you say the code should look like this
    ```scala
      files = Option(files).getOrElse(sparkConf.getOption("spark.yarn.dist.files").
        map(p => Utils.resolveURIs(p)).orNull)
    
      archives = Option(archives).getOrElse(sparkConf.getOption("spark.yarn.dist.archives").
        map(p => Utils.resolveURIs(p)).orNull)
    ``` 
    `spark.yarn.dist.*`  and `--archives/--files` behavior is different, this is a bit strange.



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

[GitHub] spark pull request: [SPARK-2051]In yarn.ClientBase spark.yarn.dist...

Posted by witgo <gi...@git.apache.org>.

Github user witgo commented on the pull request:

    https://github.com/apache/spark/pull/969#issuecomment-46040404
  
    @tgravescs [SPARK-1951](https://issues.apache.org/jira/browse/SPARK-1951) should be related to the issue you said


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

[GitHub] spark pull request: [SPARK-2051]In yarn.ClientBase spark.yarn.dist...

Posted by AmplabJenkins <gi...@git.apache.org>.

Github user AmplabJenkins commented on the pull request:

    https://github.com/apache/spark/pull/969#issuecomment-46259088
  
    Merged build started. 


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

[GitHub] spark pull request: [SPARK-2051]In yarn.ClientBase spark.yarn.dist...

Posted by AmplabJenkins <gi...@git.apache.org>.

Github user AmplabJenkins commented on the pull request:

    https://github.com/apache/spark/pull/969#issuecomment-46259357
  
    Merged build started. 


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

[GitHub] spark pull request: [SPARK-2051]In yarn.ClientBase spark.yarn.dist...

Posted by vanzin <gi...@git.apache.org>.

Github user vanzin commented on the pull request:

    https://github.com/apache/spark/pull/969#issuecomment-46032726
  
    Hey @tgravescs, my issue with this change is that I'm pretty sure it's the same issue I fixed in the last commit pushed to PR#560 (at least half of it).
    
    The other half seems to be that SparkSubmit is not correctly handling some of these options for yarn-cluster mode; but it's been several days since I looked at that part so I don't remember the details. But I'm pretty sure about the first half above.


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

[GitHub] spark pull request: [WIP] [SPARK-2051]In yarn.ClientBase spark.yar...

Posted by AmplabJenkins <gi...@git.apache.org>.

Github user AmplabJenkins commented on the pull request:

    https://github.com/apache/spark/pull/969#issuecomment-45750854
  
    All automated tests passed.
    Refer to this link for build results: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/15680/


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

[GitHub] spark pull request: [SPARK-2051]In yarn.ClientBase spark.yarn.dist...

Posted by AmplabJenkins <gi...@git.apache.org>.

Github user AmplabJenkins commented on the pull request:

    https://github.com/apache/spark/pull/969#issuecomment-46202989
  
     Merged build triggered. 


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

[GitHub] spark pull request: [SPARK-2051]In yarn.ClientBase spark.yarn.dist...

Posted by AmplabJenkins <gi...@git.apache.org>.

Github user AmplabJenkins commented on the pull request:

    https://github.com/apache/spark/pull/969#issuecomment-46027790
  
     Merged build triggered. 


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

[GitHub] spark pull request: [SPARK-2051]In yarn.ClientBase spark.yarn.dist...

Posted by AmplabJenkins <gi...@git.apache.org>.

Github user AmplabJenkins commented on the pull request:

    https://github.com/apache/spark/pull/969#issuecomment-45983120
  
    Merged build started. 


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

[GitHub] spark pull request: [WIP] [SPARK-2051]In yarn.ClientBase spark.yar...

Posted by vanzin <gi...@git.apache.org>.

Github user vanzin commented on the pull request:

    https://github.com/apache/spark/pull/969#issuecomment-45805466
  
    @witgo You didn't address my comments. You're duplicating logic that's already mostly there in SparkSubmit, just needs to be fixed a little for yarn cluster mode. See my previous comment.
    
    As for your test, see: https://github.com/apache/spark/pull/560, more specifically:  https://github.com/vanzin/spark/commit/08268f0b7091f42a661b7b0d6fde256980140ff9



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

[GitHub] spark pull request: [SPARK-2051]In yarn.ClientBase spark.yarn.dist...

Posted by tgravescs <gi...@git.apache.org>.

Github user tgravescs commented on the pull request:

    https://github.com/apache/spark/pull/969#issuecomment-46052395
  
    @witgo  what is your usecase for needing these as configs? Can you just use the spark-submit options? Like I mention above these were never supported as straight up configs.  The more I look at this, the more complexity it is adding and I'm not sure its adding much benefit.


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

[GitHub] spark pull request: [SPARK-2051]In yarn.ClientBase spark.yarn.dist...

Posted by AmplabJenkins <gi...@git.apache.org>.

Github user AmplabJenkins commented on the pull request:

    https://github.com/apache/spark/pull/969#issuecomment-46262113
  
    All automated tests passed.
    Refer to this link for build results: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/15835/


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

[GitHub] spark pull request: [SPARK-2051]In yarn.ClientBase spark.yarn.dist...

Posted by AmplabJenkins <gi...@git.apache.org>.

Github user AmplabJenkins commented on the pull request:

    https://github.com/apache/spark/pull/969#issuecomment-46324466
  
    Merged build started. 


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

[GitHub] spark pull request: [SPARK-2051]In yarn.ClientBase spark.yarn.dist...

Posted by witgo <gi...@git.apache.org>.

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

    https://github.com/apache/spark/pull/969#discussion_r13784281
  
    --- Diff: yarn/common/src/main/scala/org/apache/spark/deploy/yarn/ClientArguments.scala ---
    @@ -45,6 +44,12 @@ class ClientArguments(val args: Array[String], val sparkConf: SparkConf) {
     
       parseArgs(args.toList)
     
    +  files = Option(files).getOrElse(sparkConf.getOption("spark.yarn.dist.files").orNull)
    +  files = Option(files).map(p => Utils.resolveURIs(p)).orNull
    --- End diff --
    
    This `Utils.resolveURIs(p)` will change the behavior of `YarnClientSchedulerBackend` ,should be removed .


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

[GitHub] spark pull request: [SPARK-2051]In yarn.ClientBase spark.yarn.dist...

Posted by AmplabJenkins <gi...@git.apache.org>.

Github user AmplabJenkins commented on the pull request:

    https://github.com/apache/spark/pull/969#issuecomment-46018045
  
    Merged build started. 


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

[GitHub] spark pull request: [SPARK-2051]In yarn.ClientBase spark.yarn.dist...

Posted by tgravescs <gi...@git.apache.org>.

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

    https://github.com/apache/spark/pull/969#discussion_r13755813
  
    --- Diff: yarn/common/src/main/scala/org/apache/spark/deploy/yarn/ClientArguments.scala ---
    @@ -45,6 +44,14 @@ class ClientArguments(val args: Array[String], val sparkConf: SparkConf) {
     
       parseArgs(args.toList)
     
    +  files = Option(files).getOrElse(sys.env.get("SPARK_YARN_DIST_FILES").orNull)
    --- End diff --
    
    I don't want to add support for the env variables for yarn-cluster mode.  We only support them on yarn-client mode for backwards compatibility. Can you remove 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.
---

[GitHub] spark pull request: [SPARK-2051]In yarn.ClientBase spark.yarn.dist...

Posted by tgravescs <gi...@git.apache.org>.

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

    https://github.com/apache/spark/pull/969#discussion_r13865835
  
    --- Diff: yarn/common/src/main/scala/org/apache/spark/deploy/yarn/ClientArguments.scala ---
    @@ -45,6 +44,25 @@ class ClientArguments(val args: Array[String], val sparkConf: SparkConf) {
     
       parseArgs(args.toList)
     
    +  // -archives/--files via spark submit or yarn-client defaults to use file:// if not specified
    +  if (sys.props.contains("SPARK_SUBMIT") || (sparkConf.getOption("spark.master").isDefined &&
    --- End diff --
    
    this logic is already handled by spark-submit, no reason to duplicate


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

[GitHub] spark pull request: [WIP] [SPARK-2051]In yarn.ClientBase spark.yar...

Posted by AmplabJenkins <gi...@git.apache.org>.

Github user AmplabJenkins commented on the pull request:

    https://github.com/apache/spark/pull/969#issuecomment-45458837
  
    Merged build finished. All automated tests 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.
---

[GitHub] spark pull request: [SPARK-2051]In yarn.ClientBase spark.yarn.dist...

Posted by AmplabJenkins <gi...@git.apache.org>.

Github user AmplabJenkins commented on the pull request:

    https://github.com/apache/spark/pull/969#issuecomment-46259077
  
     Merged build triggered. 


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

[GitHub] spark pull request: [SPARK-2051]In yarn.ClientBase spark.yarn.dist...

Posted by AmplabJenkins <gi...@git.apache.org>.

Github user AmplabJenkins commented on the pull request:

    https://github.com/apache/spark/pull/969#issuecomment-46025354
  
     Merged build triggered. 


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