Skip to content

Commit 7fda184

Browse files
pan3793HyukjinKwon
authored andcommitted
[SPARK-32121][SHUFFLE] Support Windows OS in ExecutorDiskUtils
### 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 <[email protected]> Co-authored-by: chengpan <[email protected]> Signed-off-by: HyukjinKwon <[email protected]>
1 parent 3f7780d commit 7fda184

File tree

2 files changed

+30
-10
lines changed

2 files changed

+30
-10
lines changed

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

Lines changed: 15 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -23,11 +23,19 @@
2323

2424
import com.google.common.annotations.VisibleForTesting;
2525

26+
import org.apache.commons.lang3.SystemUtils;
2627
import org.apache.spark.network.util.JavaUtils;
2728

2829
public class ExecutorDiskUtils {
2930

30-
private static final Pattern MULTIPLE_SEPARATORS = Pattern.compile(File.separator + "{2,}");
31+
private static final Pattern MULTIPLE_SEPARATORS;
32+
static {
33+
if (SystemUtils.IS_OS_WINDOWS) {
34+
MULTIPLE_SEPARATORS = Pattern.compile("[/\\\\]+");
35+
} else {
36+
MULTIPLE_SEPARATORS = Pattern.compile("/{2,}");
37+
}
38+
}
3139

3240
/**
3341
* Hashes a filename into the corresponding local directory, in a manner consistent with
@@ -50,14 +58,18 @@ public static File getFile(String[] localDirs, int subDirsPerLocalDir, String fi
5058
* the internal code in java.io.File would normalize it later, creating a new "foo/bar"
5159
* String copy. Unfortunately, we cannot just reuse the normalization code that java.io.File
5260
* uses, since it is in the package-private class java.io.FileSystem.
61+
*
62+
* On Windows, separator "\" is used instead of "/".
63+
*
64+
* "\\" is a legal character in path name on Unix-like OS, but illegal on Windows.
5365
*/
5466
@VisibleForTesting
5567
static String createNormalizedInternedPathname(String dir1, String dir2, String fname) {
5668
String pathname = dir1 + File.separator + dir2 + File.separator + fname;
5769
Matcher m = MULTIPLE_SEPARATORS.matcher(pathname);
58-
pathname = m.replaceAll("/");
70+
pathname = m.replaceAll(Matcher.quoteReplacement(File.separator));
5971
// A single trailing slash needs to be taken care of separately
60-
if (pathname.length() > 1 && pathname.endsWith("/")) {
72+
if (pathname.length() > 1 && pathname.charAt(pathname.length() - 1) == File.separatorChar) {
6173
pathname = pathname.substring(0, pathname.length() - 1);
6274
}
6375
return pathname.intern();

common/network-shuffle/src/test/java/org/apache/spark/network/shuffle/ExternalShuffleBlockResolverSuite.java

Lines changed: 15 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,7 @@
2525

2626
import com.fasterxml.jackson.databind.ObjectMapper;
2727
import com.google.common.io.CharStreams;
28+
import org.apache.commons.lang3.SystemUtils;
2829
import org.apache.spark.network.shuffle.protocol.ExecutorShuffleInfo;
2930
import org.apache.spark.network.util.MapConfigProvider;
3031
import org.apache.spark.network.util.TransportConf;
@@ -146,12 +147,19 @@ public void jsonSerializationOfExecutorRegistration() throws IOException {
146147

147148
@Test
148149
public void testNormalizeAndInternPathname() {
149-
assertPathsMatch("/foo", "bar", "baz", "/foo/bar/baz");
150-
assertPathsMatch("//foo/", "bar/", "//baz", "/foo/bar/baz");
151-
assertPathsMatch("foo", "bar", "baz///", "foo/bar/baz");
152-
assertPathsMatch("/foo/", "/bar//", "/baz", "/foo/bar/baz");
153-
assertPathsMatch("/", "", "", "/");
154-
assertPathsMatch("/", "/", "/", "/");
150+
String sep = File.separator;
151+
String expectedPathname = sep + "foo" + sep + "bar" + sep + "baz";
152+
assertPathsMatch("/foo", "bar", "baz", expectedPathname);
153+
assertPathsMatch("//foo/", "bar/", "//baz", expectedPathname);
154+
assertPathsMatch("/foo/", "/bar//", "/baz", expectedPathname);
155+
assertPathsMatch("foo", "bar", "baz///", "foo" + sep + "bar" + sep + "baz");
156+
assertPathsMatch("/", "", "", sep);
157+
assertPathsMatch("/", "/", "/", sep);
158+
if (SystemUtils.IS_OS_WINDOWS) {
159+
assertPathsMatch("/foo\\/", "bar", "baz", expectedPathname);
160+
} else {
161+
assertPathsMatch("/foo\\/", "bar", "baz", sep + "foo\\" + sep + "bar" + sep + "baz");
162+
}
155163
}
156164

157165
private void assertPathsMatch(String p1, String p2, String p3, String expectedPathname) {
@@ -160,6 +168,6 @@ private void assertPathsMatch(String p1, String p2, String p3, String expectedPa
160168
assertEquals(expectedPathname, normPathname);
161169
File file = new File(normPathname);
162170
String returnedPath = file.getPath();
163-
assertTrue(normPathname == returnedPath);
171+
assertEquals(normPathname, returnedPath);
164172
}
165173
}

0 commit comments

Comments
 (0)