This is an automated email from the ASF dual-hosted git repository. gurwls223 pushed a commit to branch branch-3.0 in repository https://gitbox.apache.org/repos/asf/spark.git
The following commit(s) were added to refs/heads/branch-3.0 by this push: new b6bb158 [SPARK-32121][SHUFFLE] Support Windows OS in ExecutorDiskUtils b6bb158 is described below commit b6bb158d3c1191aac6f4d27d84d7c791c5063b24 Author: pancheng <379377...@qq.com> AuthorDate: Thu Jul 2 19:21:11 2020 +0900 [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 <379377...@qq.com> Co-authored-by: chengpan <cheng....@idiaoyan.com> Signed-off-by: HyukjinKwon <gurwls...@apache.org> (cherry picked from commit 7fda184f0fc39613fb68e912c189c54b93c638e6) Signed-off-by: HyukjinKwon <gurwls...@apache.org> --- .../spark/network/shuffle/ExecutorDiskUtils.java | 18 +++++++++++++++--- .../shuffle/ExternalShuffleBlockResolverSuite.java | 22 +++++++++++++++------- 2 files changed, 30 insertions(+), 10 deletions(-) diff --git a/common/network-shuffle/src/main/java/org/apache/spark/network/shuffle/ExecutorDiskUtils.java b/common/network-shuffle/src/main/java/org/apache/spark/network/shuffle/ExecutorDiskUtils.java index 13f6046..6549cac 100644 --- a/common/network-shuffle/src/main/java/org/apache/spark/network/shuffle/ExecutorDiskUtils.java +++ b/common/network-shuffle/src/main/java/org/apache/spark/network/shuffle/ExecutorDiskUtils.java @@ -23,11 +23,19 @@ import java.util.regex.Pattern; import com.google.common.annotations.VisibleForTesting; +import org.apache.commons.lang3.SystemUtils; import org.apache.spark.network.util.JavaUtils; public class ExecutorDiskUtils { - private static final Pattern MULTIPLE_SEPARATORS = Pattern.compile(File.separator + "{2,}"); + private static final Pattern MULTIPLE_SEPARATORS; + static { + if (SystemUtils.IS_OS_WINDOWS) { + MULTIPLE_SEPARATORS = Pattern.compile("[/\\\\]+"); + } else { + MULTIPLE_SEPARATORS = Pattern.compile("/{2,}"); + } + } /** * Hashes a filename into the corresponding local directory, in a manner consistent with @@ -50,14 +58,18 @@ public class ExecutorDiskUtils { * 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. + * + * On Windows, separator "\" is used instead of "/". + * + * "\\" is a legal character in path name on Unix-like OS, but illegal on Windows. */ @VisibleForTesting static String createNormalizedInternedPathname(String dir1, String dir2, String fname) { String pathname = dir1 + File.separator + dir2 + File.separator + fname; Matcher m = MULTIPLE_SEPARATORS.matcher(pathname); - pathname = m.replaceAll("/"); + pathname = m.replaceAll(Matcher.quoteReplacement(File.separator)); // A single trailing slash needs to be taken care of separately - if (pathname.length() > 1 && pathname.endsWith("/")) { + if (pathname.length() > 1 && pathname.charAt(pathname.length() - 1) == File.separatorChar) { pathname = pathname.substring(0, pathname.length() - 1); } return pathname.intern(); diff --git a/common/network-shuffle/src/test/java/org/apache/spark/network/shuffle/ExternalShuffleBlockResolverSuite.java b/common/network-shuffle/src/test/java/org/apache/spark/network/shuffle/ExternalShuffleBlockResolverSuite.java index 09b3143..6515b6c 100644 --- a/common/network-shuffle/src/test/java/org/apache/spark/network/shuffle/ExternalShuffleBlockResolverSuite.java +++ b/common/network-shuffle/src/test/java/org/apache/spark/network/shuffle/ExternalShuffleBlockResolverSuite.java @@ -25,6 +25,7 @@ import java.nio.charset.StandardCharsets; import com.fasterxml.jackson.databind.ObjectMapper; import com.google.common.io.CharStreams; +import org.apache.commons.lang3.SystemUtils; import org.apache.spark.network.shuffle.protocol.ExecutorShuffleInfo; import org.apache.spark.network.util.MapConfigProvider; import org.apache.spark.network.util.TransportConf; @@ -146,12 +147,19 @@ public class ExternalShuffleBlockResolverSuite { @Test public void testNormalizeAndInternPathname() { - assertPathsMatch("/foo", "bar", "baz", "/foo/bar/baz"); - assertPathsMatch("//foo/", "bar/", "//baz", "/foo/bar/baz"); - assertPathsMatch("foo", "bar", "baz///", "foo/bar/baz"); - assertPathsMatch("/foo/", "/bar//", "/baz", "/foo/bar/baz"); - assertPathsMatch("/", "", "", "/"); - assertPathsMatch("/", "/", "/", "/"); + String sep = File.separator; + String expectedPathname = sep + "foo" + sep + "bar" + sep + "baz"; + assertPathsMatch("/foo", "bar", "baz", expectedPathname); + assertPathsMatch("//foo/", "bar/", "//baz", expectedPathname); + assertPathsMatch("/foo/", "/bar//", "/baz", expectedPathname); + assertPathsMatch("foo", "bar", "baz///", "foo" + sep + "bar" + sep + "baz"); + assertPathsMatch("/", "", "", sep); + assertPathsMatch("/", "/", "/", sep); + if (SystemUtils.IS_OS_WINDOWS) { + assertPathsMatch("/foo\\/", "bar", "baz", expectedPathname); + } else { + assertPathsMatch("/foo\\/", "bar", "baz", sep + "foo\\" + sep + "bar" + sep + "baz"); + } } private void assertPathsMatch(String p1, String p2, String p3, String expectedPathname) { @@ -160,6 +168,6 @@ public class ExternalShuffleBlockResolverSuite { assertEquals(expectedPathname, normPathname); File file = new File(normPathname); String returnedPath = file.getPath(); - assertTrue(normPathname == returnedPath); + assertEquals(normPathname, returnedPath); } } --------------------------------------------------------------------- To unsubscribe, e-mail: commits-unsubscr...@spark.apache.org For additional commands, e-mail: commits-h...@spark.apache.org