[SPARK-24195][Core] Ignore the files with "local" scheme in SparkContext.addFile #21533

xuanyuanking · 2018-06-12T01:56:53Z

What changes were proposed in this pull request?

In Spark "local" scheme means resources are already on the driver/executor nodes, this pr ignore the files with "local" scheme in SparkContext.addFile for fixing potential bug.

How was this patch tested?

Existing tests.

xuanyuanking · 2018-06-12T01:57:45Z

cc @felixcheung. Please take a look about this when you have time. Thanks.

SparkQA · 2018-06-12T06:24:30Z

Test build #91682 has finished for PR 21533 at commit f922fd8.

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

gatorsmile · 2018-06-12T06:28:07Z

cc @jiangxb1987 @jerryshao

HyukjinKwon · 2018-06-12T16:49:17Z

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

-      case null | "local" => new File(path).getCanonicalFile.toURI.toString
+      case null | "local" =>
+        // SPARK-24195: Local is not a valid scheme for FileSystem, we should only keep path here.
+        uri = new Path(uri.getPath).toUri


Why is this needed? Can't we just do new File(uri.getPath).getCanonicalFile.toURI.toString without this line?

Yes, same question. The above line seems not useful.

it changes uri - which is referenced again below.

Yes, just as @felixcheung said, this because we will use uri in https://github.com/apache/spark/pull/21533/files/f922fd8c995164cada4a8b72e92c369a827def16#diff-364713d7776956cb8b0a771e9b62f82dR1557, if the uri with local scheme, we'll get an exception cause local is not a valid scheme for FileSystem.

I mean we getPath doesn't include scheme:

scala> new Path("local:///a/b/c") res0: org.apache.hadoop.fs.Path = local:/a/b/c scala> new Path("local:///a/b/c").toUri res1: java.net.URI = local:///a/b/c scala> new Path("local:///a/b/c").toUri.getScheme res2: String = local scala> new Path("local:///a/b/c").toUri.getPath res3: String = /a/b/c

why should we do this again?

scala> new Path(new Path("local:///a/b/c").toUri.getPath).toUri.getPath res4: String = /a/b/c

Yea we can simplify this.

@HyukjinKwon @jiangxb1987
Thanks for your explain, I think I know what's your meaning about we getPath doesn't include scheme. Actually the purpose of this code uri = new Path(uri.getPath).toUri, is to reassign the var in +1520, we don't want the uri including local scheme.

Can't we just do new File(uri.getPath).getCanonicalFile.toURI.toString without this line?

We can't because like I explained above, if we didn't do uri = new Path(uri.getPath).toUri, will get a exception like below:

No FileSystem for scheme: local java.io.IOException: No FileSystem for scheme: local at org.apache.hadoop.fs.FileSystem.getFileSystemClass(FileSystem.java:2586) at org.apache.hadoop.fs.FileSystem.createFileSystem(FileSystem.java:2593) at org.apache.hadoop.fs.FileSystem.access$200(FileSystem.java:91) at org.apache.hadoop.fs.FileSystem$Cache.getInternal(FileSystem.java:2632) at org.apache.hadoop.fs.FileSystem$Cache.get(FileSystem.java:2614) at org.apache.hadoop.fs.FileSystem.get(FileSystem.java:370) at org.apache.spark.util.Utils$.getHadoopFileSystem(Utils.scala:1830) at org.apache.spark.util.Utils$.doFetchFile(Utils.scala:690) at org.apache.spark.util.Utils$.fetchFile(Utils.scala:486) at org.apache.spark.SparkContext.addFile(SparkContext.scala:1557)

I mean, at least we can do:

val a = new File(uri.getPath).getCanonicalFile.toURI.toString uri = new Path(uri.getPath).toUri a

new Path(uri.getPath).toUri for trimming the scheme looks not quite clean though. It's a-okay at least to me.

Ah, I see, thanks. I'll do this in the next commit. Thanks for your patient explain.

HyukjinKwon · 2018-06-12T16:50:01Z

core/src/test/scala/org/apache/spark/SparkContextSuite.scala


+    // file and absolute path for path with local scheme
+    val file3 = File.createTempFile("someprefix3", "somesuffix3", dir)
+    val localPath = "local://" + file3.getParent + "/../" + file3.getParentFile.getName +


Let's use string interpolation.

Got it, thanks.

HyukjinKwon · 2018-06-12T16:50:25Z

core/src/test/scala/org/apache/spark/SparkContextSuite.scala

+      sc.addFile(file1.getAbsolutePath)
+      sc.addFile(relativePath)
+      sc.addFile(localPath)
+      sc.parallelize(Array(1), 1).map(x => {


nit:

map { x => ... }

Got it, fix in next commit.

HyukjinKwon · 2018-06-12T16:51:20Z

Seems fine to me.

felixcheung

can you give some examples of what the output for local:/ looks like in the change of addFile()?

felixcheung · 2018-06-13T04:28:42Z

core/src/test/scala/org/apache/spark/SparkContextSuite.scala

        }
-        if (absolutePath2 == gotten2.getAbsolutePath) {
-          throw new SparkException("file should have been copied : " + absolutePath2)
-        }


can we not change the existing test?

Actually I keep all existing test and just do clean work for reducing common code line by adding a function checkGottenFile in https://github.com/apache/spark/pull/21533/files/f922fd8c995164cada4a8b72e92c369a827def16#diff-8d5858d578a2dda1a2edb0d8cefa4f24R139. If you think it's unnecessary, I just change it back.

felixcheung · 2018-06-13T04:28:46Z

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

-      case null | "local" => new File(path).getCanonicalFile.toURI.toString
+      case null | "local" =>
+        // SPARK-24195: Local is not a valid scheme for FileSystem, we should only keep path here.
+        uri = new Path(uri.getPath).toUri


it changes uri - which is referenced again below.

SparkQA · 2018-06-13T20:07:36Z

Test build #91780 has finished for PR 21533 at commit 797cefe.

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

jiangxb1987 · 2018-06-14T06:10:47Z

LGTM except for the comment from @HyukjinKwon

jerryshao · 2018-06-15T08:13:37Z

Just take another look on this issue. I think the fix is just to make it work, but not make it work correctly.

The fix here actually treats scheme "local" to "file", actually they're different in Spark.

In Spark "local" scheme means resources are already on the driver/executor nodes, which means Spark doesn't need to ship resources from driver to executors via fileserver. But here it treats as "file" which will be shipped via fileserver to executors. This is semantically not correct.

I think for "local" scheme, the fix should:

Make it accessible both from driver and executors via SparkFiles#get. By copying resource to the folders.
It should not be added into fileServer.

HyukjinKwon · 2018-06-15T08:16:42Z

Exactly I agree with ^. It should be the best to implement that logic. I thought it seems we have never implemented "local" logic as described so far. So, I thought it's kind of okay.

jerryshao · 2018-06-15T08:19:23Z

"local" scheme was supported long ago for users who already deploy jars on every node. HDI heavily uses this feature.

HyukjinKwon · 2018-06-15T08:25:38Z

Ah, then maybe I missed some histories in the transitive changes.

xuanyuanking · 2018-06-17T14:43:59Z

@jerryshao Great thanks for your review and detailed explain, based on your guidance, I found the behavior about the file in local scheme added in fileServer was introduced by the PR c4b1108, and before that, the file in local scheme is treated as local files: c4b1108#diff-364713d7776956cb8b0a771e9b62f82dL1023.
So my last commit try to fix this as your guidance, keep the same behavior of local scheme file before c4b1108, please help me check whether it is the correct semantics we want. Thanks!

SparkQA · 2018-06-17T18:16:56Z

Test build #92001 has finished for PR 21533 at commit 5daf804.

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

xuanyuanking · 2018-06-18T11:29:13Z

retest this please.

SparkQA · 2018-06-18T16:32:40Z

Test build #92027 has finished for PR 21533 at commit 5daf804.

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

jerryshao · 2018-06-21T09:29:53Z

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

+        val tmpPath = new File(uri.getPath).getCanonicalFile.toURI.toString
+        // SPARK-24195: Local is not a valid scheme for FileSystem, we should only keep path here.
+        uri = new Path(uri.getPath).toUri
+        tmpPath


I think the change here make file with "local" scheme a no-op. This makes me think whether supporting "local" scheme in addFile is meaningful or not? Because file with "local" scheme is already existed on every node, also it should be aware by the user, so adding it seems not meaingful.

By looking at the similar method addJar, there "local" jar is properly treated without adding to fileServer, and properly convert to the right scheme used by classloader.

This makes me think whether supporting "local" scheme in addFile is meaningful or not? Because file with "local" scheme is already existed on every node, also it should be aware by the user, so adding it seems not meaingful.

Yeah, agree with you. The last change wants to treat "local" file without adding to fileServer and correct its scheme to "file:", but maybe add a local file, the behavior itself is a no-op? So we just forbidden user pass a file with "local" scheme in addFile?

SparkQA · 2018-06-27T14:16:59Z

Test build #92379 has finished for PR 21533 at commit ac12568.

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

jerryshao · 2018-06-28T00:59:04Z

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

+      case "local" =>
+        logWarning("We do not support add a local file here because file with local scheme is " +
+          "already existed on every node, there is no need to call addFile to add it again. " +
+          "(See more discussion about this in SPARK-24195.)")


Can we please rephrase to "File with 'local' scheme is not supported to add to file server, since it is already available on every node."?

Got it, rephrase done.

jerryshao · 2018-06-28T01:15:01Z

I think maybe we could:

either ignore the files with "local" scheme, and let user to decide how to fetch the files, like what current fix.
or copy the 'local' scheme files to the SparkFiles#getRootDirectory both in driver and executor. The change would be in Utils#fetchFile.

@jiangxb1987 @vanzin what's your option?

SparkQA · 2018-06-28T07:05:02Z

Test build #92410 has finished for PR 21533 at commit eb46ccf.

This patch fails due to an unknown error code, -9.
This patch merges cleanly.
This patch adds no public classes.

xuanyuanking · 2018-06-28T07:10:03Z

retest this please.

SparkQA · 2018-06-28T11:31:43Z

Test build #92415 has finished for PR 21533 at commit eb46ccf.

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

SparkQA · 2018-07-18T19:00:01Z

Test build #4219 has finished for PR 21533 at commit eb46ccf.

This patch fails Spark unit tests.
This patch does not merge cleanly.
This patch adds no public classes.

SparkQA · 2018-07-19T00:55:00Z

Test build #4220 has finished for PR 21533 at commit eb46ccf.

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

HyukjinKwon · 2018-07-19T02:09:07Z

retest this please.

jiangxb1987 · 2018-07-19T02:55:21Z

Please also update the title and PR description because we changed the proposed solution in the middle.

SparkQA · 2018-07-19T06:48:16Z

Test build #93259 has finished for PR 21533 at commit eb46ccf.

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

xuanyuanking · 2018-07-19T06:56:17Z

@jiangxb1987 Thanks for reminding, rephrase done.

jerryshao

LGTM, @jiangxb1987 WDYT?

jiangxb1987 · 2018-07-20T03:09:25Z

lgtm too

HyukjinKwon · 2018-07-20T03:10:50Z

SGTM too

jerryshao · 2018-07-20T03:24:25Z

Merging to master branch. Thanks all!

xuanyuanking · 2018-07-20T05:04:01Z

Thanks everyone for your help!

bug fix for local:/ path in sc.addFile

f922fd8

HyukjinKwon reviewed Jun 12, 2018

View reviewed changes

felixcheung reviewed Jun 13, 2018

View reviewed changes

address comments

797cefe

local file should not be added into fileServer

5daf804

jerryshao reviewed Jun 21, 2018

View reviewed changes

addFile don not support add file in local scheme

ac12568

jerryshao reviewed Jun 28, 2018

View reviewed changes

Rephrase log

eb46ccf

srowen approved these changes Jul 18, 2018

View reviewed changes

xuanyuanking changed the title ~~[SPARK-24195][Core] Bug fix for local:/ path in SparkContext.addFile~~ [SPARK-24195][Core] Ignore the files with "local" scheme in SparkContext.addFile Jul 19, 2018

jerryshao approved these changes Jul 20, 2018

View reviewed changes

asfgit closed this in 7db81ac Jul 20, 2018

xuanyuanking deleted the SPARK-24195 branch July 20, 2018 05:04

[SPARK-24195][Core] Ignore the files with "local" scheme in SparkContext.addFile #21533

[SPARK-24195][Core] Ignore the files with "local" scheme in SparkContext.addFile #21533

Uh oh!

Conversation

xuanyuanking commented Jun 12, 2018 • 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!

xuanyuanking commented Jun 12, 2018

Uh oh!

SparkQA commented Jun 12, 2018

Uh oh!

gatorsmile commented Jun 12, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

felixcheung Jun 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

HyukjinKwon Jun 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

HyukjinKwon commented Jun 12, 2018

Uh oh!

felixcheung left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

felixcheung Jun 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Jun 13, 2018

Uh oh!

jiangxb1987 commented Jun 14, 2018

Uh oh!

jerryshao commented Jun 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

HyukjinKwon commented Jun 15, 2018

Uh oh!

jerryshao commented Jun 15, 2018

Uh oh!

HyukjinKwon commented Jun 15, 2018

Uh oh!

xuanyuanking commented Jun 17, 2018

Uh oh!

SparkQA commented Jun 17, 2018

Uh oh!

xuanyuanking commented Jun 18, 2018

Uh oh!

SparkQA commented Jun 18, 2018

Uh oh!

Choose a reason for hiding this comment

xuanyuanking commented Jun 12, 2018 •

edited

Loading

felixcheung Jun 13, 2018 •

edited

Loading

HyukjinKwon Jun 13, 2018 •

edited

Loading

felixcheung Jun 13, 2018 •

edited

Loading

jerryshao commented Jun 15, 2018 •

edited

Loading

xuanyuanking Jun 23, 2018 •

edited

Loading