Skip to content

[SPARK-32121][SHUFFLE] Support Windows OS in ExecutorDiskUtils #28940

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
pan3793 wants to merge 10 commits into from
Closed

[SPARK-32121][SHUFFLE] Support Windows OS in ExecutorDiskUtils #28940

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
e8ea16a
[SPARK-32121][SHUFFLE][TEST] Fix ExternalShuffleBlockResolverSuite fa…
pan3793 Jun 28, 2020
187e813
[FOLLOWUP][SPARK-32121][SHUFFLE][TEST] fix checkstyle
pan3793 Jun 28, 2020
9b24a14
[SPARK-32121][SHUFFLE][TEST] use different MULTIPLE_SEPARATORS in Win…
pan3793 Jun 29, 2020
8cd8bd1
[FOLLOWUP][SPARK-32121][SHUFFLE][TEST] fix checkstyle
pan3793 Jun 29, 2020
06024f4
[FOLLOWUP][SPARK-32121][SHUFFLE][TEST] fix checkstyle
pan3793 Jun 29, 2020
6a201a5
[FOLLOWUP][SPARK-32121][SHUFFLE][TEST] simplify code by commons-lang3
pan3793 Jun 29, 2020
50a742d
[FOLLOWUP][SPARK-32121][SHUFFLE][TEST] improve comments
pan3793 Jun 29, 2020
56f3be5
[FOLLOWUP][SPARK-32121][SHUFFLE][TEST] minor refactor
pan3793 Jun 30, 2020
aac01ca
[FOLLOWUP][SPARK-32121][SHUFFLE][TEST] code style
pan3793 Jun 30, 2020
a80bd6c
[SPARK-32121][SHUFFLE] improve comments
pan3793 Jul 1, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion common/network-shuffle/src/main/java/org/apache/spark/network/shuffle/ExecutorDiskUtils.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@ static String createNormalizedInternedPathname(String dir1, String dir2, String
Matcher m = MULTIPLE_SEPARATORS.matcher(pathname);
pathname = m.replaceAll(Matcher.quoteReplacement(File.separator));
// A single trailing slash needs to be taken care of separately
if (pathname.length() > 1 && pathname.endsWith(File.separator)) {
if (pathname.length() > 1 && pathname.charAt(pathname.length() - 1) == File.separatorChar) {
pathname = pathname.substring(0, pathname.length() - 1);
}
return pathname.intern();
Expand Down
26 changes: 12 additions & 14 deletions ...fle/src/test/java/org/apache/spark/network/shuffle/ExternalShuffleBlockResolverSuite.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -147,22 +147,20 @@ public void jsonSerializationOfExecutorRegistration() throws IOException {

@Test
public void testNormalizeAndInternPathname() {
assertPathsMatch("/foo", "bar", "baz",
File.separator + "foo" + File.separator + "bar" + File.separator + "baz");
assertPathsMatch("//foo/", "bar/", "//baz",
File.separator + "foo" + File.separator + "bar" + File.separator + "baz");
assertPathsMatch("foo", "bar", "baz///",
"foo" + File.separator + "bar" + File.separator + "baz");
assertPathsMatch("/foo/", "/bar//", "/baz",
File.separator + "foo" + File.separator + "bar" + File.separator + "baz");
assertPathsMatch("/", "", "", File.separator);
assertPathsMatch("/", "/", "/", File.separator);
String sep = File.separator;
String expectedPathname1 = sep + "foo" + sep + "bar" + sep + "baz";
Copy link
Contributor

@attilapiros attilapiros Jun 30, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

@pan3793 pan3793 Jun 30, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@attilapiros attilapiros Jun 30, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

@pan3793 pan3793 Jun 30, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, reverted.

String expectedPathname2 = "foo" + sep + "bar" + sep + "baz";
String expectedPathname3 = sep + "foo\\" + sep + "bar" + sep + "baz";
assertPathsMatch("/foo", "bar", "baz", expectedPathname1);
assertPathsMatch("//foo/", "bar/", "//baz", expectedPathname1);
assertPathsMatch("foo", "bar", "baz///", expectedPathname2);
assertPathsMatch("/foo/", "/bar//", "/baz", expectedPathname1);
assertPathsMatch("/", "", "", sep);
assertPathsMatch("/", "/", "/", sep);
if (SystemUtils.IS_OS_WINDOWS) {
assertPathsMatch("/foo\\/", "bar", "baz",
File.separator + "foo" + File.separator + "bar" + File.separator + "baz");
assertPathsMatch("/foo\\/", "bar", "baz", expectedPathname1);
} else {
assertPathsMatch("/foo\\/", "bar", "baz",
File.separator + "foo\\" + File.separator + "bar" + File.separator + "baz");
assertPathsMatch("/foo\\/", "bar", "baz", expectedPathname3);
}
}

Expand Down