[SPARK-32121][SHUFFLE] Support Windows OS in ExecutorDiskUtils by pan3793 · Pull Request #28940 · apache/spark

pan3793 · 2020-06-28T19:37:07Z

What changes were proposed in this pull request?

Correct file seprate use in ExecutorDiskUtils.createNormalizedInternedPathname on Windows

Why are the changes needed?

ExternalShuffleBlockResolverSuite failed on Windows, see detail at:
https://issues.apache.org/jira/browse/SPARK-32121

Does this PR introduce any user-facing change?

No

How was this patch tested?

The existed test suite.

…iled on Windows

HyukjinKwon · 2020-06-29T02:24:25Z

ok to test

HyukjinKwon · 2020-06-29T02:28:22Z

Looks fine.

SparkQA · 2020-06-29T02:37:24Z

Test build #124618 has finished for PR 28940 at commit 187e813.

This patch fails build dependency tests.
This patch merges cleanly.
This patch adds no public classes.

…dows and Unix-like OS

SparkQA · 2020-06-29T07:36:22Z

Test build #124627 has finished for PR 28940 at commit 8cd8bd1.

This patch fails build dependency tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2020-06-29T17:42:00Z

Test build #124643 has finished for PR 28940 at commit 06024f4.

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

HyukjinKwon · 2020-06-30T04:06:24Z

Build started: [CORE] org.apache.spark.network.shuffle.ExternalShuffleBlockResolverSuite

attilapiros

Kinda unrelated to your change but i am keep wondering how often could the createNormalizedInternedPathname give back a different String than the FileSystem#normalize if that is the case then the String#intern does not save us from storing multiple copies of the result string within the File class as a path.

attilapiros · 2020-06-30T12:01:44Z

What about the following?

As createNormalizedInternedPathname is only used here in production:

spark/common/network-shuffle/src/main/java/org/apache/spark/network/shuffle/ExecutorDiskUtils.java

Lines 48 to 49 in 50a742d

returnnewFile(createNormalizedInternedPathname( 
localDir, String.format("%02x", subDirId), filename)); 

What about creating the file with the constructor:
https://github.com/openjdk/jdk/blob/9a9add8825a040565051a09010b29b099c2e7d49/jdk/src/share/classes/java/io/File.java#L275-L281:

publicFile(Stringpathname) {
if (pathname == null) {
thrownewNullPointerException();
}
this.path = fs.normalize(pathname);
this.prefixLength = fs.prefixLength(this.path);
}

Then read the path via File#getPath back and build the File again with the the path where the String#intern called. As we already doing path transformations twice this new solution is not worse than the current implementation but at least it will be a perfect match regarding the normalized path. The constructed first File will be garbage collected (this might be a disadvantage and the prefixLength calculation I am checking how complex is that). Moreover we can get rid of the unnecessary tests.

@Ngone51 @HyukjinKwon what's your opinion?

HyukjinKwon · 2020-06-30T12:28:33Z

I was assuming we can't reuse io.File per:

the internal code in java.io.File would normalize it later, creating a new "foo/bar"
String copy. Unfortunately, we cannot just reuse the normalization code that java.io.File
uses, since it is in the package-private class java.io.FileSystem.

If it's clear that it's going to be more performant and simple, let's do it. Otherwise, let's just let get this in for now, and investigate that separately.

attilapiros · 2020-06-30T12:55:57Z

We have to measure it for sure.

Regarding simplicity of the solution I described it is basically sth like:
attilapiros@926ebbd

And I checked the prefixLength calculation they are O(1):

Of course we can do it in separate PR too.

attilapiros · 2020-06-30T13:14:22Z

- assertPathsMatch("/", "", "", File.separator);
- assertPathsMatch("/", "/", "/", File.separator);
+String sep = File.separator;
+String expectedPathname1 = sep + "foo" + sep + "bar" + sep + "baz";


I meant this value only. The others are used only once.

I prefer to write in single style if there is no strict rule that "value used only once must be inlined"

Extracting a variable which only used once just increases the indirection.
Especially here where the role of the value is trivial. If it would be something complex then I might understand to use a describing name but here not.

OK, reverted.

HyukjinKwon · 2020-07-01T01:58:28Z

retest this please

SparkQA · 2020-07-01T05:54:05Z

Test build #124722 has finished for PR 28940 at commit aac01ca.

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

SparkQA · 2020-07-01T08:20:24Z

Test build #124744 has finished for PR 28940 at commit a80bd6c.

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

HyukjinKwon · 2020-07-01T09:36:54Z

retest this please

attilapiros · 2020-07-01T10:18:05Z

Regarding the changes below I will create a PR and open a new Jira ticket (so it will be taken care of).

We have to measure it for sure.
Regarding simplicity of the solution I described it is basically sth like:
attilapiros@926ebbd
And I checked the prefixLength calculation they are O(1):
https://github.com/openjdk/jdk/blob/jdk8-b120/jdk/src/windows/classes/java/io/WinNTFileSystem.java#L206-L223
https://github.com/openjdk/jdk/blob/jdk8-b120/jdk/src/solaris/classes/java/io/UnixFileSystem.java#L96-L99
Of course we can do it in separate PR too.

HyukjinKwon · 2020-07-01T11:47:22Z

Sure, @attilapiros. Thanks you for following up and investigating deep. I will merge this one as soon as the tests pass.

SparkQA · 2020-07-01T12:56:08Z

Test build #124765 has finished for PR 28940 at commit a80bd6c.

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

HyukjinKwon · 2020-07-01T13:03:20Z

retest this please

SparkQA · 2020-07-01T13:09:55Z

Test build #124783 has finished for PR 28940 at commit a80bd6c.

This patch fails build dependency tests.
This patch merges cleanly.
This patch adds no public classes.

HyukjinKwon · 2020-07-01T14:32:15Z

retest this please

SparkQA · 2020-07-01T18:04:45Z

Test build #124790 has finished for PR 28940 at commit a80bd6c.

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

dongjoon-hyun · 2020-07-01T23:22:03Z

Retest this please.

SparkQA · 2020-07-01T23:29:22Z

Test build #124827 has finished for PR 28940 at commit a80bd6c.

This patch fails build dependency tests.
This patch merges cleanly.
This patch adds no public classes.

HyukjinKwon · 2020-07-02T01:47:54Z

retest this please

SparkQA · 2020-07-02T01:52:06Z

Test build #124838 has finished for PR 28940 at commit a80bd6c.

This patch fails build dependency tests.
This patch merges cleanly.
This patch adds no public classes.

HyukjinKwon · 2020-07-02T01:57:47Z

retest this please

SparkQA · 2020-07-02T01:59:39Z

Test build #124840 has finished for PR 28940 at commit a80bd6c.

This patch fails build dependency tests.
This patch merges cleanly.
This patch adds no public classes.

HyukjinKwon · 2020-07-02T02:40:48Z

retest this please

SparkQA · 2020-07-02T02:46:16Z

Test build #124850 has finished for PR 28940 at commit a80bd6c.

This patch fails build dependency tests.
This patch merges cleanly.
This patch adds no public classes.

HyukjinKwon · 2020-07-02T03:58:25Z

retest this please

SparkQA · 2020-07-02T07:05:02Z

Test build #124859 has finished for PR 28940 at commit a80bd6c.

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

HyukjinKwon · 2020-07-02T07:06:15Z

retest this please

SparkQA · 2020-07-02T10:17:41Z

Test build #124873 has finished for PR 28940 at commit a80bd6c.

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

HyukjinKwon · 2020-07-02T10:21:08Z

Merged to master and branch-3.0.

### What changes were proposed in this pull request? Correct file seprate use in `ExecutorDiskUtils.createNormalizedInternedPathname` on Windows ### Why are the changes needed? `ExternalShuffleBlockResolverSuite` failed on Windows, see detail at: https://issues.apache.org/jira/browse/SPARK-32121 ### Does this PR introduce _any_ user-facing change? No ### How was this patch tested? The existed test suite. Closes#28940 from pan3793/SPARK-32121. Lead-authored-by: pancheng <379377944@qq.com> Co-authored-by: chengpan <cheng.pan@idiaoyan.com> Signed-off-by: HyukjinKwon <gurwls223@apache.org> (cherry picked from commit 7fda184) Signed-off-by: HyukjinKwon <gurwls223@apache.org>

[SPARK-32121][SHUFFLE][TEST] Fix ExternalShuffleBlockResolverSuite fa…
e8ea16a
…iled on Windows

probot-autolabelerBot added the CORE label Jun 28, 2020

pan3793 changed the title ~~Fix ExternalShuffleBlockResolverSuite failed on Windows~~[SPARK-32121][SHUFFLE][TEST] Fix ExternalShuffleBlockResolverSuite failed on WindowsJun 28, 2020

[FOLLOWUP][SPARK-32121][SHUFFLE][TEST] fix checkstyle
187e813

HyukjinKwon reviewed Jun 29, 2020
View reviewed changes

Comment thread...huffle/src/test/java/org/apache/spark/network/shuffle/ExternalShuffleBlockResolverSuite.java Outdated

pan3793 requested a review from HyukjinKwon June 29, 2020 02:19

HyukjinKwon reviewed Jun 29, 2020
View reviewed changes

Comment threadcommon/network-shuffle/src/main/java/org/apache/spark/network/shuffle/ExecutorDiskUtils.java Outdated

pan3793 requested a review from HyukjinKwon June 29, 2020 02:33

pan3793 added 2 commits June 29, 2020 11:54

[SPARK-32121][SHUFFLE][TEST] use different MULTIPLE_SEPARATORS in Win…
9b24a14
…dows and Unix-like OS

[FOLLOWUP][SPARK-32121][SHUFFLE][TEST] fix checkstyle
8cd8bd1

attilapiros reviewed Jun 29, 2020
View reviewed changes

Comment thread...huffle/src/test/java/org/apache/spark/network/shuffle/ExternalShuffleBlockResolverSuite.java Outdated

attilapiros reviewed Jun 29, 2020
View reviewed changes

Comment thread...huffle/src/test/java/org/apache/spark/network/shuffle/ExternalShuffleBlockResolverSuite.java Outdated

attilapiros reviewed Jun 29, 2020
View reviewed changes

Comment thread...huffle/src/test/java/org/apache/spark/network/shuffle/ExternalShuffleBlockResolverSuite.java Outdated

[FOLLOWUP][SPARK-32121][SHUFFLE][TEST] fix checkstyle
06024f4

Ngone51 reviewed Jun 29, 2020
View reviewed changes

Comment threadcommon/network-shuffle/src/main/java/org/apache/spark/network/shuffle/ExecutorDiskUtils.java Outdated

pan3793 added 2 commits June 29, 2020 20:58

[FOLLOWUP][SPARK-32121][SHUFFLE][TEST] simplify code by commons-lang3
6a201a5

[FOLLOWUP][SPARK-32121][SHUFFLE][TEST] improve comments
50a742d

attilapiros reviewed Jun 30, 2020
View reviewed changes

Comment thread...huffle/src/test/java/org/apache/spark/network/shuffle/ExternalShuffleBlockResolverSuite.java Outdated
Comment threadcommon/network-shuffle/src/main/java/org/apache/spark/network/shuffle/ExecutorDiskUtils.java Outdated

[FOLLOWUP][SPARK-32121][SHUFFLE][TEST] minor refactor
56f3be5

attilapiros reviewed Jun 30, 2020
View reviewed changes

[FOLLOWUP][SPARK-32121][SHUFFLE][TEST] code style
aac01ca

HyukjinKwon approved these changes Jul 1, 2020
View reviewed changes

dongjoon-hyun reviewed Jul 1, 2020
View reviewed changes

Comment threadcommon/network-shuffle/src/main/java/org/apache/spark/network/shuffle/ExecutorDiskUtils.java Outdated

[SPARK-32121][SHUFFLE] improve comments
a80bd6c

HyukjinKwon closed this in 7fda184Jul 2, 2020

Uh oh!

Conversation

pan3793 commented Jun 28, 2020

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

Uh oh!

Uh oh!

HyukjinKwon commented Jun 29, 2020

Uh oh!

Uh oh!

HyukjinKwon commented Jun 29, 2020

Uh oh!

SparkQA commented Jun 29, 2020

Uh oh!

SparkQA commented Jun 29, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

SparkQA commented Jun 29, 2020

Uh oh!

HyukjinKwon commented Jun 30, 2020

Uh oh!

attilapiros left a comment • 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!

Uh oh!

Uh oh!

attilapiros commented Jun 30, 2020• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

HyukjinKwon commented Jun 30, 2020

Uh oh!

attilapiros commented Jun 30, 2020• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

attilapirosJun 30, 2020

Choose a reason for hiding this comment

Uh oh!

pan3793Jun 30, 2020

Choose a reason for hiding this comment

Uh oh!

attilapirosJun 30, 2020

Choose a reason for hiding this comment

Uh oh!

pan3793Jun 30, 2020

Choose a reason for hiding this comment

Uh oh!

HyukjinKwon commented Jul 1, 2020

Uh oh!

Uh oh!

SparkQA commented Jul 1, 2020

Uh oh!

SparkQA commented Jul 1, 2020

Uh oh!

HyukjinKwon commented Jul 1, 2020

Uh oh!

attilapiros commented Jul 1, 2020

Uh oh!

HyukjinKwon commented Jul 1, 2020

Uh oh!

SparkQA commented Jul 1, 2020

Uh oh!

HyukjinKwon commented Jul 1, 2020

Uh oh!

SparkQA commented Jul 1, 2020

Uh oh!

HyukjinKwon commented Jul 1, 2020

Uh oh!

SparkQA commented Jul 1, 2020

Uh oh!

dongjoon-hyun commented Jul 1, 2020

Uh oh!

SparkQA commented Jul 1, 2020

Uh oh!

HyukjinKwon commented Jul 2, 2020

attilapiros left a comment •
edited
Loading

attilapiros commented Jun 30, 2020•
edited
Loading

attilapiros commented Jun 30, 2020•
edited
Loading