[SPARK-18160][CORE][YARN] spark.files & spark.jars should not be passed to driver in yarn mode #15669

zjffdu · 2016-10-28T07:52:53Z

What changes were proposed in this pull request?

spark.files is still passed to driver in yarn mode, so SparkContext will still handle it which cause the error in the jira desc.

How was this patch tested?

Tested manually in a 5 node cluster. As this issue only happens in multiple node cluster, so I didn't write test for it.

…luster mode

SparkQA · 2016-10-28T10:17:36Z

Test build #67699 has finished for PR 15669 at commit 67a5ccf.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

zjffdu · 2016-10-28T12:30:01Z

@vanzin @JoshRosen @yanboliang @brkyvz Please help review.

tgravescs · 2016-10-28T15:49:31Z

I thought we had fixed this in 2.0 to make it consistent that anytime --files is specified with yarn it goes through distributed cache. It shouldn't be using the spark addFiles method to distribute. Perhaps that broke? @vanzin do you remember

jerryshao · 2016-10-30T03:44:32Z

Quite strange --files in yarn-cluster will fall into SparkContext? AFAIK it will be handled by yarn#client to add to distributed cache, it will not involve SparkContext's addFiles.

zjffdu · 2016-10-31T08:53:58Z

@jerryshao spark.files is always passed to driver so SparkContext.addFile is called in yarn-cluster.

spark/core/src/main/scala/org/apache/spark/deploy/SparkSubmit.scala

Line 609 in 7bf8a40

// Load any properties specified through --conf and the default properties file

    // Load any properties specified through --conf and the default properties file
    for ((k, v) <- args.sparkProperties) {
      sysProps.getOrElseUpdate(k, v)
    }

Seems the issue is that spark.files don't need to be passed to driver in yarn-cluster mode. In that case it can be fixed in SparkSubmit.scala, another thing is that I notice some suspicious code in SparkContext.addJar, Is the following code still needed ?

spark/core/src/main/scala/org/apache/spark/SparkContext.scala

Line 1710 in 39e2bad

if (master == "yarn" && deployMode == "cluster") {

 if (master == "yarn" && deployMode == "cluster") {
              // In order for this to work in yarn cluster mode the user must specify the
              // --addJars option to the client to upload the file into the distributed cache
              // of the AM to make it show up in the current working directory.
              val fileName = new Path(uri.getPath).getName()
              try {
                env.rpcEnv.fileServer.addJar(new File(fileName))
              } catch {
                case e: Exception =>
                  // For now just log an error but allow to go through so spark examples work.
                  // The spark examples don't really need the jar distributed since its also
                  // the app jar.
                  logError("Error adding jar (" + e + "), was the --addJars option used?")
                  null
              }
            } else {

jerryshao · 2016-10-31T09:01:49Z

From the code what I can see is that spark.files will not be set in yarn mode.

      OptionAssigner(args.files, LOCAL | STANDALONE | MESOS, ALL_DEPLOY_MODES,
        sysProp = "spark.files"),

zjffdu · 2016-10-31T12:06:00Z

spark.files would still be passed to driver even in yarn-cluster if you check the following code.

spark/core/src/main/scala/org/apache/spark/deploy/SparkSubmit.scala

Line 609 in 7bf8a40

// Load any properties specified through --conf and the default properties file

    // Load any properties specified through --conf and the default properties file
    for ((k, v) <- args.sparkProperties) {
      sysProps.getOrElseUpdate(k, v)
    }

tgravescs · 2016-10-31T13:47:37Z

yeah so I think the fix here should be that we don't pass it through or don't handle it in sparkContext. All the yarn stuff should go through the distributed cache.

mridulm · 2016-10-31T15:50:35Z

I agree with Tom, for yarn cluster mode we should rely on distributed cache

vanzin · 2016-10-31T21:53:31Z

If I understand the issue here, the problem is not --files but setting spark.files through --conf (or the config file). The former is translated into spark.yarn.dist.files by SparkSubmit but it seems the latter isn't, and to me that's the bug. So basically what Tom and Mridul have said.

I think the correct fix would be in either SparkSubmit.scala or YARN's Client.scala - probably the latter to keep the YARN-specific stuff in one place.

zjffdu · 2016-10-31T23:16:21Z

that's correct, it is due to spark.files, jira has been updated. Will update the PR soon.

…n yarn-cluster mode" This reverts commit 67a5ccf.

zjffdu · 2016-11-01T04:00:58Z

core/src/main/scala/org/apache/spark/SparkContext.scala

-                case exc: FileNotFoundException =>
-                  logError(s"Jar not found at $path")
-                  null
-              }


These are obsoleted code IMO, so I remove them.

SparkQA · 2016-11-01T06:04:39Z

Test build #67868 has finished for PR 15669 at commit 230d56c.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2016-11-01T12:24:23Z

Test build #67885 has finished for PR 15669 at commit b8aa1fb.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

jerryshao · 2016-11-01T13:22:06Z

I think this issue also existed in yarn client mode, files will be added twice (once with Spark's file server, another with distributed cache), because both file server and yarn#client could find this file, then there's no exception. But this will be failed in yarn cluster mode since spark file server could not find out file with specified path.

zjffdu · 2016-11-01T14:27:17Z

That's correct, this PR will also fix the yarn-client case. PR title is updated.

vanzin · 2016-11-01T17:17:56Z

yarn/src/main/scala/org/apache/spark/deploy/yarn/Client.scala

@@ -1202,7 +1202,9 @@ private object Client extends Logging {
    // Note that any env variable with the SPARK_ prefix gets propagated to all (remote) processes
    System.setProperty("SPARK_YARN_MODE", "true")
    val sparkConf = new SparkConf
-
+    // yarn would use dist cache to distribute files & jars, so remove them from sparkConf


Can you update this comment to mention that SparkSubmit uses the YARN cache to distribute these in YARN mode? It looked weird at first that these were just being removed, but the code in SparkSubmit/SparkSubmitArguments takes care of setting the right config.

vanzin · 2016-11-01T17:18:16Z

One minor comment update, otherwise LGTM.

jerryshao · 2016-11-02T01:07:21Z

I'm wondering is it enough to address issue in yarn client mode, since we will have to properties "spark.files" and "spark.yarn.dist.files", the former will be handled by SparkContext and the latter will be handled by yarn#client, while yarn#client is created after SparkContext is launched, so is it too late to remove "spark.conf" in yarn#client, also yarn#client only honors "spark.yarn.dist.files", this will still add files again.

zjffdu · 2016-11-02T01:47:51Z

hmm, noticed spark.files is still passed to SparkContext in yarn-client mode, seems I need to do that in SparkSubmit

vanzin · 2016-11-02T02:06:41Z

I think it would be better to treat that as a separate issue. spark.files has always worked like this in client mode; the behavior being changed here fixes an actual bug in cluster mode.

I'd consider using the cache for spark.files in client mode a separate enhancement.

SparkQA · 2016-11-02T03:24:21Z

Test build #67941 has finished for PR 15669 at commit 0dd5486.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

vanzin · 2016-11-02T18:47:10Z

Merging to master / 2.0.

…ed to driver in yarn mode ## What changes were proposed in this pull request? spark.files is still passed to driver in yarn mode, so SparkContext will still handle it which cause the error in the jira desc. ## How was this patch tested? Tested manually in a 5 node cluster. As this issue only happens in multiple node cluster, so I didn't write test for it. Author: Jeff Zhang <zjffdu@apache.org> Closes #15669 from zjffdu/SPARK-18160. (cherry picked from commit 3c24299) Signed-off-by: Marcelo Vanzin <vanzin@cloudera.com>

vanzin · 2016-11-02T18:49:08Z

Looks like there's a branch-2.1 now, so I merge into that branch too.

…ed to driver in yarn mode ## What changes were proposed in this pull request? spark.files is still passed to driver in yarn mode, so SparkContext will still handle it which cause the error in the jira desc. ## How was this patch tested? Tested manually in a 5 node cluster. As this issue only happens in multiple node cluster, so I didn't write test for it. Author: Jeff Zhang <zjffdu@apache.org> Closes apache#15669 from zjffdu/SPARK-18160.

[SPARK-18160][CORE][YARN] SparkContext.addFile doesn't work in yarn-c…

67a5ccf

…luster mode

Revert "[SPARK-18160][CORE][YARN] SparkContext.addFile doesn't work i…

8033bd1

…n yarn-cluster mode" This reverts commit 67a5ccf.

zjffdu changed the title ~~[SPARK-18160][CORE][YARN] SparkContext.addFile doesn't work in yarn-cluster mode~~ [SPARK-18160][CORE][YARN] spark.files should not be passed to driver in yarn-cluster mode Nov 1, 2016

zjffdu commented Nov 1, 2016

View reviewed changes

zjffdu closed this Nov 1, 2016

zjffdu reopened this Nov 1, 2016

remove spark.files & spark.jars from SparkConf in yarn mode

b8aa1fb

zjffdu force-pushed the SPARK-18160 branch from 230d56c to b8aa1fb Compare November 1, 2016 09:58

zjffdu changed the title ~~[SPARK-18160][CORE][YARN] spark.files should not be passed to driver in yarn-cluster mode~~ [SPARK-18160][CORE][YARN] spark.files should not be passed to driver in yarn mode Nov 1, 2016

zjffdu changed the title ~~[SPARK-18160][CORE][YARN] spark.files should not be passed to driver in yarn mode~~ [SPARK-18160][CORE][YARN] spark.files & spark.jars should not be passed to driver in yarn mode Nov 1, 2016

vanzin reviewed Nov 1, 2016

View reviewed changes

update comment

0dd5486

asfgit closed this in 3c24299 Nov 2, 2016

[SPARK-18160][CORE][YARN] spark.files & spark.jars should not be passed to driver in yarn mode #15669

[SPARK-18160][CORE][YARN] spark.files & spark.jars should not be passed to driver in yarn mode #15669

Uh oh!

Conversation

zjffdu commented Oct 28, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

SparkQA commented Oct 28, 2016

Uh oh!

zjffdu commented Oct 28, 2016

Uh oh!

tgravescs commented Oct 28, 2016

Uh oh!

jerryshao commented Oct 30, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zjffdu commented Oct 31, 2016

Uh oh!

jerryshao commented Oct 31, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zjffdu commented Oct 31, 2016

Uh oh!

tgravescs commented Oct 31, 2016

Uh oh!

mridulm commented Oct 31, 2016

Uh oh!

vanzin commented Oct 31, 2016

Uh oh!

zjffdu commented Oct 31, 2016

Uh oh!

zjffdu Nov 1, 2016

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Nov 1, 2016

Uh oh!

SparkQA commented Nov 1, 2016

Uh oh!

jerryshao commented Nov 1, 2016

Uh oh!

zjffdu commented Nov 1, 2016

Uh oh!

vanzin Nov 1, 2016

Choose a reason for hiding this comment

Uh oh!

vanzin commented Nov 1, 2016

Uh oh!

jerryshao commented Nov 2, 2016

Uh oh!

zjffdu commented Nov 2, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vanzin commented Nov 2, 2016

Uh oh!

SparkQA commented Nov 2, 2016

Uh oh!

vanzin commented Nov 2, 2016

Uh oh!

vanzin commented Nov 2, 2016

Uh oh!

Uh oh!

zjffdu commented Oct 28, 2016 •

edited

Loading

jerryshao commented Oct 30, 2016 •

edited

Loading

jerryshao commented Oct 31, 2016 •

edited

Loading

zjffdu commented Nov 2, 2016 •

edited

Loading