You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by GitBox <gi...@apache.org> on 2020/06/28 19:41:07 UTC
[GitHub] [spark] pan3793 opened a new pull request #28940: Fix ExternalShuffleBlockResolverSuite failed on Windows
pan3793 opened a new pull request #28940:
URL: https://github.com/apache/spark/pull/28940
### 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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] attilapiros commented on a change in pull request #28940: [SPARK-32121][SHUFFLE][TEST] Fix ExternalShuffleBlockResolverSuite failed on Windows
Posted by GitBox <gi...@apache.org>.
attilapiros commented on a change in pull request #28940:
URL: https://github.com/apache/spark/pull/28940#discussion_r446882734
##########
File path: common/network-shuffle/src/main/java/org/apache/spark/network/shuffle/ExecutorDiskUtils.java
##########
@@ -27,7 +27,7 @@
public class ExecutorDiskUtils {
- private static final Pattern MULTIPLE_SEPARATORS = Pattern.compile(File.separator + "{2,}");
+ private static final Pattern MULTIPLE_SEPARATORS = Pattern.compile("[/\\\\]+");
Review comment:
That RegExp was coming from here: https://github.com/apache/spark/blob/master/common/network-shuffle/src/main/java/org/apache/spark/network/shuffle/ExternalShuffleBlockResolver.java#L74
(Actually as I see no one is referencing `ExternalShuffleBlockResolver#MULTIPLE_SEPARATORS` so it can be removed.)
So now I am wondering if the original regexp is is so old and even backported to 2.2: https://github.com/apache/spark/pull/21456 whether this change is really necessary.
I plan to boot a windows and so some checks there.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] HyukjinKwon commented on pull request #28940: [SPARK-32121][SHUFFLE] Support Windows OS in ExecutorDiskUtils
Posted by GitBox <gi...@apache.org>.
HyukjinKwon commented on pull request #28940:
URL: https://github.com/apache/spark/pull/28940#issuecomment-652454533
retest this please
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on pull request #28940: [SPARK-32121][SHUFFLE] Support Windows OS in ExecutorDiskUtils
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #28940:
URL: https://github.com/apache/spark/pull/28940#issuecomment-652311263
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on pull request #28940: [SPARK-32121][SHUFFLE] Support Windows OS in ExecutorDiskUtils
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #28940:
URL: https://github.com/apache/spark/pull/28940#issuecomment-652403216
Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/124765/
Test FAILed.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] HyukjinKwon commented on a change in pull request #28940: [SPARK-32121][SHUFFLE][TEST] Fix ExternalShuffleBlockResolverSuite failed on Windows
Posted by GitBox <gi...@apache.org>.
HyukjinKwon commented on a change in pull request #28940:
URL: https://github.com/apache/spark/pull/28940#discussion_r446891403
##########
File path: common/network-shuffle/src/main/java/org/apache/spark/network/shuffle/ExecutorDiskUtils.java
##########
@@ -27,7 +27,7 @@
public class ExecutorDiskUtils {
- private static final Pattern MULTIPLE_SEPARATORS = Pattern.compile(File.separator + "{2,}");
+ private static final Pattern MULTIPLE_SEPARATORS = Pattern.compile("[/\\\\]+");
Review comment:
I have a sort of half automated script to run the tests on Windows via CI AppVeyor which I used at that time to audit the tests on Windows. Let me try to run and see if that works:
Build started: [CORE] `org.apache.spark.network.shuffle.ExternalShuffleBlockResolverSuite` [](https://ci.appveyor.com/project/HyukjinKwon/spark/branch/66D51DFB-3C81-4172-9E99-3A56B786E809):
```
[info] Test run started
[info] Test org.apache.spark.network.shuffle.ExternalShuffleBlockResolverSuite.testNormalizeAndInternPathname started
[info] Test org.apache.spark.network.shuffle.ExternalShuffleBlockResolverSuite.testSortShuffleBlocks started
[info] Test org.apache.spark.network.shuffle.ExternalShuffleBlockResolverSuite.testBadRequests started
[info] Test org.apache.spark.network.shuffle.ExternalShuffleBlockResolverSuite.jsonSerializationOfExecutorRegistration started
[info] Test run finished: 0 failed, 0 ignored, 4 total, 5.828s
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] HyukjinKwon commented on a change in pull request #28940: [SPARK-32121][SHUFFLE][TEST] Fix ExternalShuffleBlockResolverSuite failed on Windows
Posted by GitBox <gi...@apache.org>.
HyukjinKwon commented on a change in pull request #28940:
URL: https://github.com/apache/spark/pull/28940#discussion_r446745167
##########
File path: common/network-shuffle/src/main/java/org/apache/spark/network/shuffle/ExecutorDiskUtils.java
##########
@@ -27,7 +27,7 @@
public class ExecutorDiskUtils {
- private static final Pattern MULTIPLE_SEPARATORS = Pattern.compile(File.separator + "{2,}");
+ private static final Pattern MULTIPLE_SEPARATORS = Pattern.compile("[/\\\\]+");
Review comment:
Ah, okay. I read it as `{2}` and I just noticed it's actually `{2,}`. Can we compile the pattern differently after checking the OS? Looks like it's going to replace `\\` in the file name on other Linux base OSs.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA removed a comment on pull request #28940: [SPARK-32121][SHUFFLE] Support Windows OS in ExecutorDiskUtils
Posted by GitBox <gi...@apache.org>.
SparkQA removed a comment on pull request #28940:
URL: https://github.com/apache/spark/pull/28940#issuecomment-652767692
**[Test build #124859 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/124859/testReport)** for PR 28940 at commit [`a80bd6c`](https://github.com/apache/spark/commit/a80bd6c8a5d93187cf06f941c2a9d296a7b6ca61).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins commented on pull request #28940: [SPARK-32121][SHUFFLE] Support Windows OS in ExecutorDiskUtils
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #28940:
URL: https://github.com/apache/spark/pull/28940#issuecomment-652270931
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] pan3793 commented on a change in pull request #28940: [SPARK-32121][SHUFFLE][TEST] Fix ExternalShuffleBlockResolverSuite failed on Windows
Posted by GitBox <gi...@apache.org>.
pan3793 commented on a change in pull request #28940:
URL: https://github.com/apache/spark/pull/28940#discussion_r447727579
##########
File path: common/network-shuffle/src/test/java/org/apache/spark/network/shuffle/ExternalShuffleBlockResolverSuite.java
##########
@@ -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";
Review comment:
OK, reverted.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on pull request #28940: [SPARK-32121][SHUFFLE] Support Windows OS in ExecutorDiskUtils
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #28940:
URL: https://github.com/apache/spark/pull/28940#issuecomment-652270943
Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/124744/
Test FAILed.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] HyukjinKwon commented on pull request #28940: [SPARK-32121][SHUFFLE] Support Windows OS in ExecutorDiskUtils
Posted by GitBox <gi...@apache.org>.
HyukjinKwon commented on pull request #28940:
URL: https://github.com/apache/spark/pull/28940#issuecomment-652767399
retest this please
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins commented on pull request #28940: [SPARK-32121][SHUFFLE][TEST] Fix ExternalShuffleBlockResolverSuite failed on Windows
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #28940:
URL: https://github.com/apache/spark/pull/28940#issuecomment-650812434
Can one of the admins verify this patch?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on pull request #28940: [SPARK-32121][SHUFFLE] Support Windows OS in ExecutorDiskUtils
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #28940:
URL: https://github.com/apache/spark/pull/28940#issuecomment-652142021
**[Test build #124722 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/124722/testReport)** for PR 28940 at commit [`aac01ca`](https://github.com/apache/spark/commit/aac01ca11c3c024e8a75753e43a217cadb1d8c46).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on pull request #28940: [SPARK-32121][SHUFFLE][TEST] Fix ExternalShuffleBlockResolverSuite failed on Windows
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #28940:
URL: https://github.com/apache/spark/pull/28940#issuecomment-651264431
**[Test build #124643 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/124643/testReport)** for PR 28940 at commit [`06024f4`](https://github.com/apache/spark/commit/06024f4018e77549d6147d63c5fc75b276cc33e8).
* This patch passes all tests.
* This patch merges cleanly.
* This patch adds no public classes.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] pan3793 commented on a change in pull request #28940: [SPARK-32121][SHUFFLE][TEST] Fix ExternalShuffleBlockResolverSuite failed on Windows
Posted by GitBox <gi...@apache.org>.
pan3793 commented on a change in pull request #28940:
URL: https://github.com/apache/spark/pull/28940#discussion_r446740732
##########
File path: common/network-shuffle/src/main/java/org/apache/spark/network/shuffle/ExecutorDiskUtils.java
##########
@@ -27,7 +27,7 @@
public class ExecutorDiskUtils {
- private static final Pattern MULTIPLE_SEPARATORS = Pattern.compile(File.separator + "{2,}");
+ private static final Pattern MULTIPLE_SEPARATORS = Pattern.compile("[/\\\\]+");
Review comment:
I think the case already covered by https://github.com/apache/spark/pull/28940/files#diff-e48223108a52336484e4bf803d62221bR159-R160
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on pull request #28940: [SPARK-32121][SHUFFLE][TEST] Fix ExternalShuffleBlockResolverSuite failed on Windows
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #28940:
URL: https://github.com/apache/spark/pull/28940#issuecomment-650981021
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on pull request #28940: [SPARK-32121][SHUFFLE][TEST] Fix ExternalShuffleBlockResolverSuite failed on Windows
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #28940:
URL: https://github.com/apache/spark/pull/28940#issuecomment-651294806
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/124643/
Test PASSed.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] pan3793 commented on a change in pull request #28940: [SPARK-32121][SHUFFLE][TEST] Fix ExternalShuffleBlockResolverSuite failed on Windows
Posted by GitBox <gi...@apache.org>.
pan3793 commented on a change in pull request #28940:
URL: https://github.com/apache/spark/pull/28940#discussion_r446941699
##########
File path: common/network-shuffle/src/main/java/org/apache/spark/network/shuffle/ExecutorDiskUtils.java
##########
@@ -41,6 +48,13 @@ public static File getFile(String[] localDirs, int subDirsPerLocalDir, String fi
localDir, String.format("%02x", subDirId), filename));
}
+ /** Returns whether the OS is Windows. */
+ @VisibleForTesting
+ static boolean isWindows() {
+ String os = System.getProperty("os.name");
+ return os.startsWith("Windows");
+ }
Review comment:
The pre-existing method is in `core` module, `network-shuffle` doesn't depend on it.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on pull request #28940: [SPARK-32121][SHUFFLE] Support Windows OS in ExecutorDiskUtils
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #28940:
URL: https://github.com/apache/spark/pull/28940#issuecomment-652694372
Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/124827/
Test FAILed.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] attilapiros commented on a change in pull request #28940: [SPARK-32121][SHUFFLE][TEST] Fix ExternalShuffleBlockResolverSuite failed on Windows
Posted by GitBox <gi...@apache.org>.
attilapiros commented on a change in pull request #28940:
URL: https://github.com/apache/spark/pull/28940#discussion_r447671636
##########
File path: common/network-shuffle/src/test/java/org/apache/spark/network/shuffle/ExternalShuffleBlockResolverSuite.java
##########
@@ -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";
Review comment:
I meant this value only. The others are used only once.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on pull request #28940: [SPARK-32121][SHUFFLE] Support Windows OS in ExecutorDiskUtils
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #28940:
URL: https://github.com/apache/spark/pull/28940#issuecomment-652408680
**[Test build #124783 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/124783/testReport)** for PR 28940 at commit [`a80bd6c`](https://github.com/apache/spark/commit/a80bd6c8a5d93187cf06f941c2a9d296a7b6ca61).
* This patch **fails build dependency tests**.
* This patch merges cleanly.
* This patch adds no public classes.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] pan3793 commented on a change in pull request #28940: [SPARK-32121][SHUFFLE][TEST] Fix ExternalShuffleBlockResolverSuite failed on Windows
Posted by GitBox <gi...@apache.org>.
pan3793 commented on a change in pull request #28940:
URL: https://github.com/apache/spark/pull/28940#discussion_r446931029
##########
File path: common/network-shuffle/src/test/java/org/apache/spark/network/shuffle/ExternalShuffleBlockResolverSuite.java
##########
@@ -146,12 +146,25 @@ public void jsonSerializationOfExecutorRegistration() throws IOException {
@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("/", "/", "/", "/");
+ assertPathsMatch("/foo", "bar", "baz",
Review comment:
`/` as sep is acceptable on Windows. `\a\b\c` and `/a/b/c` both are valid and same path on Windows.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on pull request #28940: [SPARK-32121][SHUFFLE] Support Windows OS in ExecutorDiskUtils
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #28940:
URL: https://github.com/apache/spark/pull/28940#issuecomment-652459570
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins commented on pull request #28940: [SPARK-32121][SHUFFLE] Support Windows OS in ExecutorDiskUtils
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #28940:
URL: https://github.com/apache/spark/pull/28940#issuecomment-652825929
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] HyukjinKwon commented on a change in pull request #28940: [SPARK-32121][SHUFFLE][TEST] Fix ExternalShuffleBlockResolverSuite failed on Windows
Posted by GitBox <gi...@apache.org>.
HyukjinKwon commented on a change in pull request #28940:
URL: https://github.com/apache/spark/pull/28940#discussion_r446925122
##########
File path: common/network-shuffle/src/main/java/org/apache/spark/network/shuffle/ExecutorDiskUtils.java
##########
@@ -27,7 +27,7 @@
public class ExecutorDiskUtils {
- private static final Pattern MULTIPLE_SEPARATORS = Pattern.compile(File.separator + "{2,}");
+ private static final Pattern MULTIPLE_SEPARATORS = Pattern.compile("[/\\\\]+");
Review comment:
Ok .. at least it seems started to build.. :-). I will run it again tomorrow against the latest commit here.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] HyukjinKwon commented on pull request #28940: [SPARK-32121][SHUFFLE] Support Windows OS in ExecutorDiskUtils
Posted by GitBox <gi...@apache.org>.
HyukjinKwon commented on pull request #28940:
URL: https://github.com/apache/spark/pull/28940#issuecomment-652371340
Sure, @attilapiros. Thanks you for following up and investigating deep. I will merge this one as soon as the tests pass.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins commented on pull request #28940: [SPARK-32121][SHUFFLE][TEST] Fix ExternalShuffleBlockResolverSuite failed on Windows
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #28940:
URL: https://github.com/apache/spark/pull/28940#issuecomment-650987308
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] attilapiros commented on a change in pull request #28940: [SPARK-32121][SHUFFLE][TEST] Fix ExternalShuffleBlockResolverSuite failed on Windows
Posted by GitBox <gi...@apache.org>.
attilapiros commented on a change in pull request #28940:
URL: https://github.com/apache/spark/pull/28940#discussion_r446913662
##########
File path: common/network-shuffle/src/test/java/org/apache/spark/network/shuffle/ExternalShuffleBlockResolverSuite.java
##########
@@ -146,12 +146,25 @@ public void jsonSerializationOfExecutorRegistration() throws IOException {
@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("/", "/", "/", "/");
+ 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);
+ if (ExecutorDiskUtils.isWindows()) {
+ assertPathsMatch("/foo\\/", "bar", "baz",
+ File.separator + "foo" + File.separator + "bar" + File.separator + "baz");
+ } else {
+ assertPathsMatch("/foo\\/", "bar", "baz",
+ File.separator + "foo\\" + File.separator + "bar" + File.separator + "baz");
Review comment:
Nit: too much indent here too
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on pull request #28940: [SPARK-32121][SHUFFLE] Support Windows OS in ExecutorDiskUtils
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #28940:
URL: https://github.com/apache/spark/pull/28940#issuecomment-652747923
**[Test build #124850 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/124850/testReport)** for PR 28940 at commit [`a80bd6c`](https://github.com/apache/spark/commit/a80bd6c8a5d93187cf06f941c2a9d296a7b6ca61).
* This patch **fails build dependency tests**.
* This patch merges cleanly.
* This patch adds no public classes.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] dongjoon-hyun commented on a change in pull request #28940: [SPARK-32121][SHUFFLE] Support Windows OS in ExecutorDiskUtils
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun commented on a change in pull request #28940:
URL: https://github.com/apache/spark/pull/28940#discussion_r448109490
##########
File path: common/network-shuffle/src/main/java/org/apache/spark/network/shuffle/ExecutorDiskUtils.java
##########
@@ -50,14 +58,18 @@ public static File getFile(String[] localDirs, int subDirsPerLocalDir, String fi
* 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 legal character in path name on Unix like OS, but illegal on Windows.
Review comment:
Maybe,
- `is legal character` -> `is a legal character`.
- `Unix like` -> `Unix-like`.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on pull request #28940: [SPARK-32121][SHUFFLE] Support Windows OS in ExecutorDiskUtils
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #28940:
URL: https://github.com/apache/spark/pull/28940#issuecomment-652407778
**[Test build #124783 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/124783/testReport)** for PR 28940 at commit [`a80bd6c`](https://github.com/apache/spark/commit/a80bd6c8a5d93187cf06f941c2a9d296a7b6ca61).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA removed a comment on pull request #28940: [SPARK-32121][SHUFFLE] Support Windows OS in ExecutorDiskUtils
Posted by GitBox <gi...@apache.org>.
SparkQA removed a comment on pull request #28940:
URL: https://github.com/apache/spark/pull/28940#issuecomment-652188033
**[Test build #124744 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/124744/testReport)** for PR 28940 at commit [`a80bd6c`](https://github.com/apache/spark/commit/a80bd6c8a5d93187cf06f941c2a9d296a7b6ca61).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on pull request #28940: [SPARK-32121][SHUFFLE][TEST] Fix ExternalShuffleBlockResolverSuite failed on Windows
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #28940:
URL: https://github.com/apache/spark/pull/28940#issuecomment-650987296
**[Test build #124627 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/124627/testReport)** for PR 28940 at commit [`8cd8bd1`](https://github.com/apache/spark/commit/8cd8bd1708fd492db7583bfe3f0fcfe05fb8bb75).
* This patch **fails build dependency tests**.
* This patch merges cleanly.
* This patch adds no public classes.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] attilapiros commented on a change in pull request #28940: [SPARK-32121][SHUFFLE][TEST] Fix ExternalShuffleBlockResolverSuite failed on Windows
Posted by GitBox <gi...@apache.org>.
attilapiros commented on a change in pull request #28940:
URL: https://github.com/apache/spark/pull/28940#discussion_r447682473
##########
File path: common/network-shuffle/src/test/java/org/apache/spark/network/shuffle/ExternalShuffleBlockResolverSuite.java
##########
@@ -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";
Review comment:
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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on pull request #28940: [SPARK-32121][SHUFFLE] Support Windows OS in ExecutorDiskUtils
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #28940:
URL: https://github.com/apache/spark/pull/28940#issuecomment-652825075
**[Test build #124859 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/124859/testReport)** for PR 28940 at commit [`a80bd6c`](https://github.com/apache/spark/commit/a80bd6c8a5d93187cf06f941c2a9d296a7b6ca61).
* This patch **fails due to an unknown error code, -9**.
* This patch merges cleanly.
* This patch adds no public classes.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on pull request #28940: [SPARK-32121][SHUFFLE] Support Windows OS in ExecutorDiskUtils
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #28940:
URL: https://github.com/apache/spark/pull/28940#issuecomment-652569383
Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/124790/
Test FAILed.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on pull request #28940: [SPARK-32121][SHUFFLE] Support Windows OS in ExecutorDiskUtils
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #28940:
URL: https://github.com/apache/spark/pull/28940#issuecomment-652310681
**[Test build #124765 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/124765/testReport)** for PR 28940 at commit [`a80bd6c`](https://github.com/apache/spark/commit/a80bd6c8a5d93187cf06f941c2a9d296a7b6ca61).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on pull request #28940: [SPARK-32121][SHUFFLE] Support Windows OS in ExecutorDiskUtils
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #28940:
URL: https://github.com/apache/spark/pull/28940#issuecomment-652767970
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins commented on pull request #28940: [SPARK-32121][SHUFFLE][TEST] Fix ExternalShuffleBlockResolverSuite failed on Windows
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #28940:
URL: https://github.com/apache/spark/pull/28940#issuecomment-650812330
Can one of the admins verify this patch?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] attilapiros commented on a change in pull request #28940: [SPARK-32121][SHUFFLE][TEST] Fix ExternalShuffleBlockResolverSuite failed on Windows
Posted by GitBox <gi...@apache.org>.
attilapiros commented on a change in pull request #28940:
URL: https://github.com/apache/spark/pull/28940#discussion_r446916363
##########
File path: common/network-shuffle/src/test/java/org/apache/spark/network/shuffle/ExternalShuffleBlockResolverSuite.java
##########
@@ -146,12 +146,25 @@ public void jsonSerializationOfExecutorRegistration() throws IOException {
@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("/", "/", "/", "/");
+ assertPathsMatch("/foo", "bar", "baz",
Review comment:
Here is your comment: https://github.com/apache/spark/blob/8cd8bd1708fd492db7583bfe3f0fcfe05fb8bb75/common/network-shuffle/src/main/java/org/apache/spark/network/shuffle/ExecutorDiskUtils.java#L68
Should not we use the correct path separator for testing too? What I mean is starting a path with `/` on windows
(like `/foo` ) is invalid in the first place.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins commented on pull request #28940: [SPARK-32121][SHUFFLE] Support Windows OS in ExecutorDiskUtils
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #28940:
URL: https://github.com/apache/spark/pull/28940#issuecomment-652040882
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on pull request #28940: [SPARK-32121][SHUFFLE] Support Windows OS in ExecutorDiskUtils
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #28940:
URL: https://github.com/apache/spark/pull/28940#issuecomment-652827291
**[Test build #124873 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/124873/testReport)** for PR 28940 at commit [`a80bd6c`](https://github.com/apache/spark/commit/a80bd6c8a5d93187cf06f941c2a9d296a7b6ca61).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins commented on pull request #28940: [SPARK-32121][SHUFFLE] Support Windows OS in ExecutorDiskUtils
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #28940:
URL: https://github.com/apache/spark/pull/28940#issuecomment-652767970
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on pull request #28940: [SPARK-32121][SHUFFLE] Support Windows OS in ExecutorDiskUtils
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #28940:
URL: https://github.com/apache/spark/pull/28940#issuecomment-652408620
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on pull request #28940: [SPARK-32121][SHUFFLE][TEST] Fix ExternalShuffleBlockResolverSuite failed on Windows
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #28940:
URL: https://github.com/apache/spark/pull/28940#issuecomment-650812434
Can one of the admins verify this patch?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on pull request #28940: [SPARK-32121][SHUFFLE][TEST] Fix ExternalShuffleBlockResolverSuite failed on Windows
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #28940:
URL: https://github.com/apache/spark/pull/28940#issuecomment-650869741
**[Test build #124618 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/124618/testReport)** for PR 28940 at commit [`187e813`](https://github.com/apache/spark/commit/187e8133522ee15a608a6473aa089086e224a88f).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] dongjoon-hyun commented on pull request #28940: [SPARK-32121][SHUFFLE] Support Windows OS in ExecutorDiskUtils
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun commented on pull request #28940:
URL: https://github.com/apache/spark/pull/28940#issuecomment-652692293
Retest this please.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on pull request #28940: [SPARK-32121][SHUFFLE][TEST] Fix ExternalShuffleBlockResolverSuite failed on Windows
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #28940:
URL: https://github.com/apache/spark/pull/28940#issuecomment-650986578
**[Test build #124627 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/124627/testReport)** for PR 28940 at commit [`8cd8bd1`](https://github.com/apache/spark/commit/8cd8bd1708fd492db7583bfe3f0fcfe05fb8bb75).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] attilapiros commented on pull request #28940: [SPARK-32121][SHUFFLE] Support Windows OS in ExecutorDiskUtils
Posted by GitBox <gi...@apache.org>.
attilapiros commented on pull request #28940:
URL: https://github.com/apache/spark/pull/28940#issuecomment-652331458
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](https://github.com/attilapiros/spark/commit/926ebbd417f49fe2897bf29dfaa3b17178bbbdaf)
>
> 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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] HyukjinKwon commented on pull request #28940: [SPARK-32121][SHUFFLE] Support Windows OS in ExecutorDiskUtils
Posted by GitBox <gi...@apache.org>.
HyukjinKwon commented on pull request #28940:
URL: https://github.com/apache/spark/pull/28940#issuecomment-652731403
retest this please
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] attilapiros edited a comment on pull request #28940: [SPARK-32121][SHUFFLE][TEST] Fix ExternalShuffleBlockResolverSuite failed on Windows
Posted by GitBox <gi...@apache.org>.
attilapiros edited a comment on pull request #28940:
URL: https://github.com/apache/spark/pull/28940#issuecomment-651747960
What about the following?
As `createNormalizedInternedPathname` is only used here in production:
https://github.com/apache/spark/blob/50a742da1c48bcfb6f69b319c9a0142801c959c6/common/network-shuffle/src/main/java/org/apache/spark/network/shuffle/ExecutorDiskUtils.java#L48-L49
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:
```java
public File(String pathname) {
if (pathname == null) {
throw new NullPointerException();
}
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?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on pull request #28940: [SPARK-32121][SHUFFLE] Support Windows OS in ExecutorDiskUtils
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #28940:
URL: https://github.com/apache/spark/pull/28940#issuecomment-652747926
Merged build finished. Test FAILed.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on pull request #28940: [SPARK-32121][SHUFFLE] Support Windows OS in ExecutorDiskUtils
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #28940:
URL: https://github.com/apache/spark/pull/28940#issuecomment-652920690
**[Test build #124873 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/124873/testReport)** for PR 28940 at commit [`a80bd6c`](https://github.com/apache/spark/commit/a80bd6c8a5d93187cf06f941c2a9d296a7b6ca61).
* This patch passes all tests.
* This patch merges cleanly.
* This patch adds no public classes.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA removed a comment on pull request #28940: [SPARK-32121][SHUFFLE][TEST] Fix ExternalShuffleBlockResolverSuite failed on Windows
Posted by GitBox <gi...@apache.org>.
SparkQA removed a comment on pull request #28940:
URL: https://github.com/apache/spark/pull/28940#issuecomment-651179260
**[Test build #124643 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/124643/testReport)** for PR 28940 at commit [`06024f4`](https://github.com/apache/spark/commit/06024f4018e77549d6147d63c5fc75b276cc33e8).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] pan3793 commented on a change in pull request #28940: [SPARK-32121][SHUFFLE][TEST] Fix ExternalShuffleBlockResolverSuite failed on Windows
Posted by GitBox <gi...@apache.org>.
pan3793 commented on a change in pull request #28940:
URL: https://github.com/apache/spark/pull/28940#discussion_r447728376
##########
File path: common/network-shuffle/src/test/java/org/apache/spark/network/shuffle/ExternalShuffleBlockResolverSuite.java
##########
@@ -146,12 +147,23 @@ public void jsonSerializationOfExecutorRegistration() throws IOException {
@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("/", "/", "/", "/");
+ assertPathsMatch("/foo", "bar", "baz",
+ File.separator + "foo" + File.separator + "bar" + File.separator + "baz");
Review comment:
Done.
##########
File path: common/network-shuffle/src/main/java/org/apache/spark/network/shuffle/ExecutorDiskUtils.java
##########
@@ -50,14 +58,18 @@ public static File getFile(String[] localDirs, int subDirsPerLocalDir, String fi
* 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 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.endsWith(File.separator)) {
Review comment:
Done.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] pan3793 commented on a change in pull request #28940: [SPARK-32121][SHUFFLE][TEST] Fix ExternalShuffleBlockResolverSuite failed on Windows
Posted by GitBox <gi...@apache.org>.
pan3793 commented on a change in pull request #28940:
URL: https://github.com/apache/spark/pull/28940#discussion_r446737765
##########
File path: common/network-shuffle/src/test/java/org/apache/spark/network/shuffle/ExternalShuffleBlockResolverSuite.java
##########
@@ -146,12 +146,18 @@ public void jsonSerializationOfExecutorRegistration() throws IOException {
@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("/", "/", "/", "/");
+ assertPathsMatch("/foo", "bar", "baz",
Review comment:
Do you means `s"${var}content"`? It's Java code.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on pull request #28940: [SPARK-32121][SHUFFLE] Support Windows OS in ExecutorDiskUtils
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #28940:
URL: https://github.com/apache/spark/pull/28940#issuecomment-652209718
Merged build finished. Test FAILed.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on pull request #28940: [SPARK-32121][SHUFFLE] Support Windows OS in ExecutorDiskUtils
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #28940:
URL: https://github.com/apache/spark/pull/28940#issuecomment-652142426
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] HyukjinKwon commented on a change in pull request #28940: [SPARK-32121][SHUFFLE][TEST] Fix ExternalShuffleBlockResolverSuite failed on Windows
Posted by GitBox <gi...@apache.org>.
HyukjinKwon commented on a change in pull request #28940:
URL: https://github.com/apache/spark/pull/28940#discussion_r446891403
##########
File path: common/network-shuffle/src/main/java/org/apache/spark/network/shuffle/ExecutorDiskUtils.java
##########
@@ -27,7 +27,7 @@
public class ExecutorDiskUtils {
- private static final Pattern MULTIPLE_SEPARATORS = Pattern.compile(File.separator + "{2,}");
+ private static final Pattern MULTIPLE_SEPARATORS = Pattern.compile("[/\\\\]+");
Review comment:
I have a sort of half automated script to run the tests on Windows via CI AppVeyor which I used at that time to audit the tests on Windows. Let me try to run and see if that works:
Build started: [CORE] `org.apache.spark.network.shuffle.ExternalShuffleBlockResolverSuite` [](https://ci.appveyor.com/project/HyukjinKwon/spark/branch/66D51DFB-3C81-4172-9E99-3A56B786E809)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA removed a comment on pull request #28940: [SPARK-32121][SHUFFLE] Support Windows OS in ExecutorDiskUtils
Posted by GitBox <gi...@apache.org>.
SparkQA removed a comment on pull request #28940:
URL: https://github.com/apache/spark/pull/28940#issuecomment-652694032
**[Test build #124827 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/124827/testReport)** for PR 28940 at commit [`a80bd6c`](https://github.com/apache/spark/commit/a80bd6c8a5d93187cf06f941c2a9d296a7b6ca61).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on pull request #28940: [SPARK-32121][SHUFFLE] Support Windows OS in ExecutorDiskUtils
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #28940:
URL: https://github.com/apache/spark/pull/28940#issuecomment-652732196
**[Test build #124838 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/124838/testReport)** for PR 28940 at commit [`a80bd6c`](https://github.com/apache/spark/commit/a80bd6c8a5d93187cf06f941c2a9d296a7b6ca61).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on pull request #28940: [SPARK-32121][SHUFFLE] Support Windows OS in ExecutorDiskUtils
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #28940:
URL: https://github.com/apache/spark/pull/28940#issuecomment-652734629
**[Test build #124840 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/124840/testReport)** for PR 28940 at commit [`a80bd6c`](https://github.com/apache/spark/commit/a80bd6c8a5d93187cf06f941c2a9d296a7b6ca61).
* This patch **fails build dependency tests**.
* This patch merges cleanly.
* This patch adds no public classes.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] HyukjinKwon commented on pull request #28940: [SPARK-32121][SHUFFLE] Support Windows OS in ExecutorDiskUtils
Posted by GitBox <gi...@apache.org>.
HyukjinKwon commented on pull request #28940:
URL: https://github.com/apache/spark/pull/28940#issuecomment-652405146
retest this please
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins commented on pull request #28940: [SPARK-32121][SHUFFLE] Support Windows OS in ExecutorDiskUtils
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #28940:
URL: https://github.com/apache/spark/pull/28940#issuecomment-652734641
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on pull request #28940: [SPARK-32121][SHUFFLE] Support Windows OS in ExecutorDiskUtils
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #28940:
URL: https://github.com/apache/spark/pull/28940#issuecomment-652732493
Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/124838/
Test FAILed.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] HyukjinKwon commented on pull request #28940: [SPARK-32121][SHUFFLE][TEST] Fix ExternalShuffleBlockResolverSuite failed on Windows
Posted by GitBox <gi...@apache.org>.
HyukjinKwon commented on pull request #28940:
URL: https://github.com/apache/spark/pull/28940#issuecomment-650868096
Looks fine.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on pull request #28940: [SPARK-32121][SHUFFLE] Support Windows OS in ExecutorDiskUtils
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #28940:
URL: https://github.com/apache/spark/pull/28940#issuecomment-652040882
Merged build finished. Test FAILed.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on pull request #28940: [SPARK-32121][SHUFFLE][TEST] Fix ExternalShuffleBlockResolverSuite failed on Windows
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #28940:
URL: https://github.com/apache/spark/pull/28940#issuecomment-650870141
Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/124618/
Test FAILed.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins commented on pull request #28940: [SPARK-32121][SHUFFLE] Support Windows OS in ExecutorDiskUtils
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #28940:
URL: https://github.com/apache/spark/pull/28940#issuecomment-652569376
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on pull request #28940: [SPARK-32121][SHUFFLE] Support Windows OS in ExecutorDiskUtils
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #28940:
URL: https://github.com/apache/spark/pull/28940#issuecomment-652734641
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA removed a comment on pull request #28940: [SPARK-32121][SHUFFLE] Support Windows OS in ExecutorDiskUtils
Posted by GitBox <gi...@apache.org>.
SparkQA removed a comment on pull request #28940:
URL: https://github.com/apache/spark/pull/28940#issuecomment-651899676
**[Test build #124656 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/124656/testReport)** for PR 28940 at commit [`aac01ca`](https://github.com/apache/spark/commit/aac01ca11c3c024e8a75753e43a217cadb1d8c46).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on pull request #28940: [SPARK-32121][SHUFFLE] Support Windows OS in ExecutorDiskUtils
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #28940:
URL: https://github.com/apache/spark/pull/28940#issuecomment-652188385
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] pan3793 commented on a change in pull request #28940: [SPARK-32121][SHUFFLE][TEST] Fix ExternalShuffleBlockResolverSuite failed on Windows
Posted by GitBox <gi...@apache.org>.
pan3793 commented on a change in pull request #28940:
URL: https://github.com/apache/spark/pull/28940#discussion_r446766066
##########
File path: common/network-shuffle/src/main/java/org/apache/spark/network/shuffle/ExecutorDiskUtils.java
##########
@@ -27,7 +27,7 @@
public class ExecutorDiskUtils {
- private static final Pattern MULTIPLE_SEPARATORS = Pattern.compile(File.separator + "{2,}");
+ private static final Pattern MULTIPLE_SEPARATORS = Pattern.compile("[/\\\\]+");
Review comment:
You are right, `\\` is legal char in filename on Unix-like OS, but not on Windows, use different pattern is a better choice.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] HyukjinKwon commented on pull request #28940: [SPARK-32121][SHUFFLE][TEST] Fix ExternalShuffleBlockResolverSuite failed on Windows
Posted by GitBox <gi...@apache.org>.
HyukjinKwon commented on pull request #28940:
URL: https://github.com/apache/spark/pull/28940#issuecomment-651517591
Build started: [CORE] `org.apache.spark.network.shuffle.ExternalShuffleBlockResolverSuite` [](https://ci.appveyor.com/project/HyukjinKwon/spark/branch/92C9A950-6909-4EB4-93E1-18523B43DF46)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins commented on pull request #28940: [SPARK-32121][SHUFFLE] Support Windows OS in ExecutorDiskUtils
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #28940:
URL: https://github.com/apache/spark/pull/28940#issuecomment-652734809
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins commented on pull request #28940: [SPARK-32121][SHUFFLE] Support Windows OS in ExecutorDiskUtils
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #28940:
URL: https://github.com/apache/spark/pull/28940#issuecomment-652747776
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] attilapiros commented on a change in pull request #28940: [SPARK-32121][SHUFFLE][TEST] Fix ExternalShuffleBlockResolverSuite failed on Windows
Posted by GitBox <gi...@apache.org>.
attilapiros commented on a change in pull request #28940:
URL: https://github.com/apache/spark/pull/28940#discussion_r446937653
##########
File path: common/network-shuffle/src/test/java/org/apache/spark/network/shuffle/ExternalShuffleBlockResolverSuite.java
##########
@@ -146,12 +146,25 @@ public void jsonSerializationOfExecutorRegistration() throws IOException {
@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("/", "/", "/", "/");
+ 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);
+ if (ExecutorDiskUtils.isWindows()) {
+ assertPathsMatch("/foo\\/", "bar", "baz",
+ File.separator + "foo" + File.separator + "bar" + File.separator + "baz");
Review comment:
I agree it would better to have it as rule but from the start as now there are many violations. If you would correct all of them with 1 commit than git blame would be a bit harder to use a blame would show your commit first on a such a line where we definitely would be more interested about the feature/bug which touched it before.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] HyukjinKwon commented on a change in pull request #28940: [SPARK-32121][SHUFFLE][TEST] Fix ExternalShuffleBlockResolverSuite failed on Windows
Posted by GitBox <gi...@apache.org>.
HyukjinKwon commented on a change in pull request #28940:
URL: https://github.com/apache/spark/pull/28940#discussion_r446738787
##########
File path: common/network-shuffle/src/test/java/org/apache/spark/network/shuffle/ExternalShuffleBlockResolverSuite.java
##########
@@ -146,12 +146,18 @@ public void jsonSerializationOfExecutorRegistration() throws IOException {
@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("/", "/", "/", "/");
+ assertPathsMatch("/foo", "bar", "baz",
Review comment:
oops
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] HyukjinKwon commented on pull request #28940: [SPARK-32121][SHUFFLE][TEST] Fix ExternalShuffleBlockResolverSuite failed on Windows
Posted by GitBox <gi...@apache.org>.
HyukjinKwon commented on pull request #28940:
URL: https://github.com/apache/spark/pull/28940#issuecomment-650867232
ok to test
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on pull request #28940: [SPARK-32121][SHUFFLE] Support Windows OS in ExecutorDiskUtils
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #28940:
URL: https://github.com/apache/spark/pull/28940#issuecomment-652747932
Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/124850/
Test FAILed.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins commented on pull request #28940: [SPARK-32121][SHUFFLE][TEST] Fix ExternalShuffleBlockResolverSuite failed on Windows
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #28940:
URL: https://github.com/apache/spark/pull/28940#issuecomment-650871137
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins commented on pull request #28940: [SPARK-32121][SHUFFLE][TEST] Fix ExternalShuffleBlockResolverSuite failed on Windows
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #28940:
URL: https://github.com/apache/spark/pull/28940#issuecomment-651286206
Merged build finished. Test PASSed.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA removed a comment on pull request #28940: [SPARK-32121][SHUFFLE] Support Windows OS in ExecutorDiskUtils
Posted by GitBox <gi...@apache.org>.
SparkQA removed a comment on pull request #28940:
URL: https://github.com/apache/spark/pull/28940#issuecomment-652734306
**[Test build #124840 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/124840/testReport)** for PR 28940 at commit [`a80bd6c`](https://github.com/apache/spark/commit/a80bd6c8a5d93187cf06f941c2a9d296a7b6ca61).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] attilapiros commented on a change in pull request #28940: [SPARK-32121][SHUFFLE][TEST] Fix ExternalShuffleBlockResolverSuite failed on Windows
Posted by GitBox <gi...@apache.org>.
attilapiros commented on a change in pull request #28940:
URL: https://github.com/apache/spark/pull/28940#discussion_r446913440
##########
File path: common/network-shuffle/src/test/java/org/apache/spark/network/shuffle/ExternalShuffleBlockResolverSuite.java
##########
@@ -146,12 +146,25 @@ public void jsonSerializationOfExecutorRegistration() throws IOException {
@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("/", "/", "/", "/");
+ 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);
+ if (ExecutorDiskUtils.isWindows()) {
+ assertPathsMatch("/foo\\/", "bar", "baz",
+ File.separator + "foo" + File.separator + "bar" + File.separator + "baz");
Review comment:
Nit: too much indent
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] attilapiros edited a comment on pull request #28940: [SPARK-32121][SHUFFLE][TEST] Fix ExternalShuffleBlockResolverSuite failed on Windows
Posted by GitBox <gi...@apache.org>.
attilapiros edited a comment on pull request #28940:
URL: https://github.com/apache/spark/pull/28940#issuecomment-651772632
We have to measure it for sure.
Regarding simplicity of the solution I described it is basically sth like:
https://github.com/attilapiros/spark/commit/926ebbd417f49fe2897bf29dfaa3b17178bbbdaf
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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] attilapiros commented on a change in pull request #28940: [SPARK-32121][SHUFFLE][TEST] Fix ExternalShuffleBlockResolverSuite failed on Windows
Posted by GitBox <gi...@apache.org>.
attilapiros commented on a change in pull request #28940:
URL: https://github.com/apache/spark/pull/28940#discussion_r446932602
##########
File path: common/network-shuffle/src/test/java/org/apache/spark/network/shuffle/ExternalShuffleBlockResolverSuite.java
##########
@@ -146,12 +146,25 @@ public void jsonSerializationOfExecutorRegistration() throws IOException {
@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("/", "/", "/", "/");
+ assertPathsMatch("/foo", "bar", "baz",
Review comment:
Then your comment is invalid, right?
This:
https://github.com/apache/spark/blob/8cd8bd1708fd492db7583bfe3f0fcfe05fb8bb75/common/network-shuffle/src/main/java/org/apache/spark/network/shuffle/ExecutorDiskUtils.java#L68
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA removed a comment on pull request #28940: [SPARK-32121][SHUFFLE] Support Windows OS in ExecutorDiskUtils
Posted by GitBox <gi...@apache.org>.
SparkQA removed a comment on pull request #28940:
URL: https://github.com/apache/spark/pull/28940#issuecomment-652310681
**[Test build #124765 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/124765/testReport)** for PR 28940 at commit [`a80bd6c`](https://github.com/apache/spark/commit/a80bd6c8a5d93187cf06f941c2a9d296a7b6ca61).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on pull request #28940: [SPARK-32121][SHUFFLE] Support Windows OS in ExecutorDiskUtils
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #28940:
URL: https://github.com/apache/spark/pull/28940#issuecomment-652270931
Merged build finished. Test FAILed.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on pull request #28940: [SPARK-32121][SHUFFLE] Support Windows OS in ExecutorDiskUtils
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #28940:
URL: https://github.com/apache/spark/pull/28940#issuecomment-652207766
**[Test build #124722 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/124722/testReport)** for PR 28940 at commit [`aac01ca`](https://github.com/apache/spark/commit/aac01ca11c3c024e8a75753e43a217cadb1d8c46).
* This patch **fails Spark unit tests**.
* This patch merges cleanly.
* This patch adds no public classes.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins commented on pull request #28940: [SPARK-32121][SHUFFLE] Support Windows OS in ExecutorDiskUtils
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #28940:
URL: https://github.com/apache/spark/pull/28940#issuecomment-652142426
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] attilapiros commented on a change in pull request #28940: [SPARK-32121][SHUFFLE][TEST] Fix ExternalShuffleBlockResolverSuite failed on Windows
Posted by GitBox <gi...@apache.org>.
attilapiros commented on a change in pull request #28940:
URL: https://github.com/apache/spark/pull/28940#discussion_r446882734
##########
File path: common/network-shuffle/src/main/java/org/apache/spark/network/shuffle/ExecutorDiskUtils.java
##########
@@ -27,7 +27,7 @@
public class ExecutorDiskUtils {
- private static final Pattern MULTIPLE_SEPARATORS = Pattern.compile(File.separator + "{2,}");
+ private static final Pattern MULTIPLE_SEPARATORS = Pattern.compile("[/\\\\]+");
Review comment:
That RegExp was coming from here: https://github.com/apache/spark/blob/master/common/network-shuffle/src/main/java/org/apache/spark/network/shuffle/ExternalShuffleBlockResolver.java#L74
(Actually as I see no one is referencing `ExternalShuffleBlockResolver#MULTIPLE_SEPARATORS` so it can be removed.)
So now I am wondering if the original regexp is is so old and even backported to 2.2: https://github.com/apache/spark/pull/21456 whether this change is really necessary.
I plan to boot a windows and do some checks there.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on pull request #28940: [SPARK-32121][SHUFFLE] Support Windows OS in ExecutorDiskUtils
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #28940:
URL: https://github.com/apache/spark/pull/28940#issuecomment-652921860
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA removed a comment on pull request #28940: [SPARK-32121][SHUFFLE][TEST] Fix ExternalShuffleBlockResolverSuite failed on Windows
Posted by GitBox <gi...@apache.org>.
SparkQA removed a comment on pull request #28940:
URL: https://github.com/apache/spark/pull/28940#issuecomment-650986578
**[Test build #124627 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/124627/testReport)** for PR 28940 at commit [`8cd8bd1`](https://github.com/apache/spark/commit/8cd8bd1708fd492db7583bfe3f0fcfe05fb8bb75).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on pull request #28940: [SPARK-32121][SHUFFLE][TEST] Fix ExternalShuffleBlockResolverSuite failed on Windows
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #28940:
URL: https://github.com/apache/spark/pull/28940#issuecomment-651286206
Merged build finished. Test PASSed.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] pan3793 commented on a change in pull request #28940: [SPARK-32121][SHUFFLE][TEST] Fix ExternalShuffleBlockResolverSuite failed on Windows
Posted by GitBox <gi...@apache.org>.
pan3793 commented on a change in pull request #28940:
URL: https://github.com/apache/spark/pull/28940#discussion_r446924627
##########
File path: common/network-shuffle/src/test/java/org/apache/spark/network/shuffle/ExternalShuffleBlockResolverSuite.java
##########
@@ -146,12 +146,25 @@ public void jsonSerializationOfExecutorRegistration() throws IOException {
@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("/", "/", "/", "/");
+ assertPathsMatch("/foo", "bar", "baz",
Review comment:
Not exactly.
`\Program Files\Custom Utilities\StringFinder.exe` means an absolute path from the root of the current drive.
Ref: https://docs.microsoft.com/en-us/dotnet/standard/io/file-path-formats
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] pan3793 commented on a change in pull request #28940: [SPARK-32121][SHUFFLE][TEST] Fix ExternalShuffleBlockResolverSuite failed on Windows
Posted by GitBox <gi...@apache.org>.
pan3793 commented on a change in pull request #28940:
URL: https://github.com/apache/spark/pull/28940#discussion_r447677818
##########
File path: common/network-shuffle/src/test/java/org/apache/spark/network/shuffle/ExternalShuffleBlockResolverSuite.java
##########
@@ -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";
Review comment:
I prefer to write in single style if there is no strict rule that "value used only once must be inlined"
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] pan3793 commented on a change in pull request #28940: [SPARK-32121][SHUFFLE][TEST] Fix ExternalShuffleBlockResolverSuite failed on Windows
Posted by GitBox <gi...@apache.org>.
pan3793 commented on a change in pull request #28940:
URL: https://github.com/apache/spark/pull/28940#discussion_r446949548
##########
File path: common/network-shuffle/src/test/java/org/apache/spark/network/shuffle/ExternalShuffleBlockResolverSuite.java
##########
@@ -146,12 +146,25 @@ public void jsonSerializationOfExecutorRegistration() throws IOException {
@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("/", "/", "/", "/");
+ assertPathsMatch("/foo", "bar", "baz",
Review comment:
I think the right expression should be: On Windows, separator "\\" is used instead of "/".
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins commented on pull request #28940: [SPARK-32121][SHUFFLE] Support Windows OS in ExecutorDiskUtils
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #28940:
URL: https://github.com/apache/spark/pull/28940#issuecomment-652732489
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] HyukjinKwon commented on pull request #28940: [SPARK-32121][SHUFFLE] Support Windows OS in ExecutorDiskUtils
Posted by GitBox <gi...@apache.org>.
HyukjinKwon commented on pull request #28940:
URL: https://github.com/apache/spark/pull/28940#issuecomment-652825600
retest this please
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] HyukjinKwon commented on pull request #28940: [SPARK-32121][SHUFFLE] Support Windows OS in ExecutorDiskUtils
Posted by GitBox <gi...@apache.org>.
HyukjinKwon commented on pull request #28940:
URL: https://github.com/apache/spark/pull/28940#issuecomment-652922348
Merged to master and branch-3.0.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] pan3793 commented on a change in pull request #28940: [SPARK-32121][SHUFFLE] Support Windows OS in ExecutorDiskUtils
Posted by GitBox <gi...@apache.org>.
pan3793 commented on a change in pull request #28940:
URL: https://github.com/apache/spark/pull/28940#discussion_r448112801
##########
File path: common/network-shuffle/src/main/java/org/apache/spark/network/shuffle/ExecutorDiskUtils.java
##########
@@ -50,14 +58,18 @@ public static File getFile(String[] localDirs, int subDirsPerLocalDir, String fi
* 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 legal character in path name on Unix like OS, but illegal on Windows.
Review comment:
Changed.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins commented on pull request #28940: [SPARK-32121][SHUFFLE] Support Windows OS in ExecutorDiskUtils
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #28940:
URL: https://github.com/apache/spark/pull/28940#issuecomment-652459570
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] Ngone51 commented on a change in pull request #28940: [SPARK-32121][SHUFFLE][TEST] Fix ExternalShuffleBlockResolverSuite failed on Windows
Posted by GitBox <gi...@apache.org>.
Ngone51 commented on a change in pull request #28940:
URL: https://github.com/apache/spark/pull/28940#discussion_r446943605
##########
File path: common/network-shuffle/src/main/java/org/apache/spark/network/shuffle/ExecutorDiskUtils.java
##########
@@ -41,6 +48,13 @@ public static File getFile(String[] localDirs, int subDirsPerLocalDir, String fi
localDir, String.format("%02x", subDirId), filename));
}
+ /** Returns whether the OS is Windows. */
+ @VisibleForTesting
+ static boolean isWindows() {
+ String os = System.getProperty("os.name");
+ return os.startsWith("Windows");
+ }
Review comment:
I see. Then, could we also use `org.apache.commons.lang3.SystemUtils.IS_OS_WINDOWS` for simplicity?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on pull request #28940: [SPARK-32121][SHUFFLE] Support Windows OS in ExecutorDiskUtils
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #28940:
URL: https://github.com/apache/spark/pull/28940#issuecomment-652569376
Merged build finished. Test FAILed.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] pan3793 commented on a change in pull request #28940: [SPARK-32121][SHUFFLE][TEST] Fix ExternalShuffleBlockResolverSuite failed on Windows
Posted by GitBox <gi...@apache.org>.
pan3793 commented on a change in pull request #28940:
URL: https://github.com/apache/spark/pull/28940#discussion_r446927754
##########
File path: common/network-shuffle/src/test/java/org/apache/spark/network/shuffle/ExternalShuffleBlockResolverSuite.java
##########
@@ -146,12 +146,25 @@ public void jsonSerializationOfExecutorRegistration() throws IOException {
@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("/", "/", "/", "/");
+ 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);
+ if (ExecutorDiskUtils.isWindows()) {
+ assertPathsMatch("/foo\\/", "bar", "baz",
+ File.separator + "foo" + File.separator + "bar" + File.separator + "baz");
Review comment:
reduced to 2 space.
BTW, consider add this rule to check-style? I also see some 4 space indent.
##########
File path: common/network-shuffle/src/test/java/org/apache/spark/network/shuffle/ExternalShuffleBlockResolverSuite.java
##########
@@ -146,12 +146,25 @@ public void jsonSerializationOfExecutorRegistration() throws IOException {
@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("/", "/", "/", "/");
+ 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);
+ if (ExecutorDiskUtils.isWindows()) {
+ assertPathsMatch("/foo\\/", "bar", "baz",
+ File.separator + "foo" + File.separator + "bar" + File.separator + "baz");
+ } else {
+ assertPathsMatch("/foo\\/", "bar", "baz",
+ File.separator + "foo\\" + File.separator + "bar" + File.separator + "baz");
Review comment:
reduced to 2 space.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] attilapiros commented on a change in pull request #28940: [SPARK-32121][SHUFFLE][TEST] Fix ExternalShuffleBlockResolverSuite failed on Windows
Posted by GitBox <gi...@apache.org>.
attilapiros commented on a change in pull request #28940:
URL: https://github.com/apache/spark/pull/28940#discussion_r446944331
##########
File path: common/network-shuffle/src/test/java/org/apache/spark/network/shuffle/ExternalShuffleBlockResolverSuite.java
##########
@@ -146,12 +146,25 @@ public void jsonSerializationOfExecutorRegistration() throws IOException {
@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("/", "/", "/", "/");
+ assertPathsMatch("/foo", "bar", "baz",
Review comment:
Sorry in this case I cannot see value in the comment. If it is not a requirement for the client of this method but a description of what the method does then I suggest to change it to:
```
* On Windows, separator "/" is used instead of "\".
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins commented on pull request #28940: [SPARK-32121][SHUFFLE] Support Windows OS in ExecutorDiskUtils
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #28940:
URL: https://github.com/apache/spark/pull/28940#issuecomment-652747926
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on pull request #28940: [SPARK-32121][SHUFFLE] Support Windows OS in ExecutorDiskUtils
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #28940:
URL: https://github.com/apache/spark/pull/28940#issuecomment-652694032
**[Test build #124827 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/124827/testReport)** for PR 28940 at commit [`a80bd6c`](https://github.com/apache/spark/commit/a80bd6c8a5d93187cf06f941c2a9d296a7b6ca61).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on pull request #28940: [SPARK-32121][SHUFFLE] Support Windows OS in ExecutorDiskUtils
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #28940:
URL: https://github.com/apache/spark/pull/28940#issuecomment-652270020
**[Test build #124744 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/124744/testReport)** for PR 28940 at commit [`a80bd6c`](https://github.com/apache/spark/commit/a80bd6c8a5d93187cf06f941c2a9d296a7b6ca61).
* This patch **fails Spark unit tests**.
* This patch merges cleanly.
* This patch adds no public classes.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] attilapiros commented on a change in pull request #28940: [SPARK-32121][SHUFFLE][TEST] Fix ExternalShuffleBlockResolverSuite failed on Windows
Posted by GitBox <gi...@apache.org>.
attilapiros commented on a change in pull request #28940:
URL: https://github.com/apache/spark/pull/28940#discussion_r446926688
##########
File path: common/network-shuffle/src/test/java/org/apache/spark/network/shuffle/ExternalShuffleBlockResolverSuite.java
##########
@@ -146,12 +146,25 @@ public void jsonSerializationOfExecutorRegistration() throws IOException {
@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("/", "/", "/", "/");
+ assertPathsMatch("/foo", "bar", "baz",
Review comment:
`/` != `\`
I talked about `/` and your example uses `\`.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA removed a comment on pull request #28940: [SPARK-32121][SHUFFLE] Support Windows OS in ExecutorDiskUtils
Posted by GitBox <gi...@apache.org>.
SparkQA removed a comment on pull request #28940:
URL: https://github.com/apache/spark/pull/28940#issuecomment-652407778
**[Test build #124783 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/124783/testReport)** for PR 28940 at commit [`a80bd6c`](https://github.com/apache/spark/commit/a80bd6c8a5d93187cf06f941c2a9d296a7b6ca61).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins commented on pull request #28940: [SPARK-32121][SHUFFLE] Support Windows OS in ExecutorDiskUtils
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #28940:
URL: https://github.com/apache/spark/pull/28940#issuecomment-652694366
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] HyukjinKwon commented on pull request #28940: [SPARK-32121][SHUFFLE][TEST] Fix ExternalShuffleBlockResolverSuite failed on Windows
Posted by GitBox <gi...@apache.org>.
HyukjinKwon commented on pull request #28940:
URL: https://github.com/apache/spark/pull/28940#issuecomment-651844863
Build started: [CORE] `org.apache.spark.network.shuffle.ExternalShuffleBlockResolverSuite` [](https://ci.appveyor.com/project/HyukjinKwon/spark/branch/FEC639EC-AEE8-467B-9CA9-A522A73C9121)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on pull request #28940: [SPARK-32121][SHUFFLE] Support Windows OS in ExecutorDiskUtils
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #28940:
URL: https://github.com/apache/spark/pull/28940#issuecomment-652828058
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on pull request #28940: [SPARK-32121][SHUFFLE] Support Windows OS in ExecutorDiskUtils
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #28940:
URL: https://github.com/apache/spark/pull/28940#issuecomment-652732479
**[Test build #124838 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/124838/testReport)** for PR 28940 at commit [`a80bd6c`](https://github.com/apache/spark/commit/a80bd6c8a5d93187cf06f941c2a9d296a7b6ca61).
* This patch **fails build dependency tests**.
* This patch merges cleanly.
* This patch adds no public classes.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on pull request #28940: [SPARK-32121][SHUFFLE][TEST] Fix ExternalShuffleBlockResolverSuite failed on Windows
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #28940:
URL: https://github.com/apache/spark/pull/28940#issuecomment-651179260
**[Test build #124643 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/124643/testReport)** for PR 28940 at commit [`06024f4`](https://github.com/apache/spark/commit/06024f4018e77549d6147d63c5fc75b276cc33e8).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on pull request #28940: [SPARK-32121][SHUFFLE] Support Windows OS in ExecutorDiskUtils
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #28940:
URL: https://github.com/apache/spark/pull/28940#issuecomment-652747776
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on pull request #28940: [SPARK-32121][SHUFFLE] Support Windows OS in ExecutorDiskUtils
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #28940:
URL: https://github.com/apache/spark/pull/28940#issuecomment-652825929
Merged build finished. Test FAILed.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] Ngone51 commented on a change in pull request #28940: [SPARK-32121][SHUFFLE][TEST] Fix ExternalShuffleBlockResolverSuite failed on Windows
Posted by GitBox <gi...@apache.org>.
Ngone51 commented on a change in pull request #28940:
URL: https://github.com/apache/spark/pull/28940#discussion_r446930264
##########
File path: common/network-shuffle/src/main/java/org/apache/spark/network/shuffle/ExecutorDiskUtils.java
##########
@@ -41,6 +48,13 @@ public static File getFile(String[] localDirs, int subDirsPerLocalDir, String fi
localDir, String.format("%02x", subDirId), filename));
}
+ /** Returns whether the OS is Windows. */
+ @VisibleForTesting
+ static boolean isWindows() {
+ String os = System.getProperty("os.name");
+ return os.startsWith("Windows");
+ }
Review comment:
Use existed `Utils.isWindows`?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] pan3793 commented on a change in pull request #28940: [SPARK-32121][SHUFFLE][TEST] Fix ExternalShuffleBlockResolverSuite failed on Windows
Posted by GitBox <gi...@apache.org>.
pan3793 commented on a change in pull request #28940:
URL: https://github.com/apache/spark/pull/28940#discussion_r446742099
##########
File path: common/network-shuffle/src/main/java/org/apache/spark/network/shuffle/ExecutorDiskUtils.java
##########
@@ -27,7 +27,7 @@
public class ExecutorDiskUtils {
- private static final Pattern MULTIPLE_SEPARATORS = Pattern.compile(File.separator + "{2,}");
+ private static final Pattern MULTIPLE_SEPARATORS = Pattern.compile("[/\\\\]+");
Review comment:
As before implement, assume all path sep is `/`, only over 2 `/` should be replaced by `/`. But now, consider the path `/a\b/\c` on Windows, the single `/` also should be replaced
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on pull request #28940: [SPARK-32121][SHUFFLE][TEST] Fix ExternalShuffleBlockResolverSuite failed on Windows
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #28940:
URL: https://github.com/apache/spark/pull/28940#issuecomment-650870138
Merged build finished. Test FAILed.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] HyukjinKwon commented on a change in pull request #28940: [SPARK-32121][SHUFFLE][TEST] Fix ExternalShuffleBlockResolverSuite failed on Windows
Posted by GitBox <gi...@apache.org>.
HyukjinKwon commented on a change in pull request #28940:
URL: https://github.com/apache/spark/pull/28940#discussion_r446891403
##########
File path: common/network-shuffle/src/main/java/org/apache/spark/network/shuffle/ExecutorDiskUtils.java
##########
@@ -27,7 +27,7 @@
public class ExecutorDiskUtils {
- private static final Pattern MULTIPLE_SEPARATORS = Pattern.compile(File.separator + "{2,}");
+ private static final Pattern MULTIPLE_SEPARATORS = Pattern.compile("[/\\\\]+");
Review comment:
I have a sort of half automated script to run the tests on Windows via CI via AppVeyor which I used at that time to audit the tests on Windows. Let me try to run and see if that works:
Build started: [CORE] `org.apache.spark.network.shuffle.ExternalShuffleBlockResolverSuite` [](https://ci.appveyor.com/project/HyukjinKwon/spark/branch/66D51DFB-3C81-4172-9E99-3A56B786E809)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on pull request #28940: [SPARK-32121][SHUFFLE] Support Windows OS in ExecutorDiskUtils
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #28940:
URL: https://github.com/apache/spark/pull/28940#issuecomment-652188033
**[Test build #124744 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/124744/testReport)** for PR 28940 at commit [`a80bd6c`](https://github.com/apache/spark/commit/a80bd6c8a5d93187cf06f941c2a9d296a7b6ca61).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on pull request #28940: [SPARK-32121][SHUFFLE] Support Windows OS in ExecutorDiskUtils
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #28940:
URL: https://github.com/apache/spark/pull/28940#issuecomment-652209724
Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/124722/
Test FAILed.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] attilapiros commented on a change in pull request #28940: [SPARK-32121][SHUFFLE][TEST] Fix ExternalShuffleBlockResolverSuite failed on Windows
Posted by GitBox <gi...@apache.org>.
attilapiros commented on a change in pull request #28940:
URL: https://github.com/apache/spark/pull/28940#discussion_r446937653
##########
File path: common/network-shuffle/src/test/java/org/apache/spark/network/shuffle/ExternalShuffleBlockResolverSuite.java
##########
@@ -146,12 +146,25 @@ public void jsonSerializationOfExecutorRegistration() throws IOException {
@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("/", "/", "/", "/");
+ 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);
+ if (ExecutorDiskUtils.isWindows()) {
+ assertPathsMatch("/foo\\/", "bar", "baz",
+ File.separator + "foo" + File.separator + "bar" + File.separator + "baz");
Review comment:
I agree it would better to have it as rule but from the start as now there are many violations. If you would correct all of them with 1 commit than git blame would be a bit harder to use a blame would show your commit first on a line where we definitely would be more interested about the feature/bug which touched it before.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on pull request #28940: [SPARK-32121][SHUFFLE][TEST] Fix ExternalShuffleBlockResolverSuite failed on Windows
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #28940:
URL: https://github.com/apache/spark/pull/28940#issuecomment-650812330
Can one of the admins verify this patch?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on pull request #28940: [SPARK-32121][SHUFFLE] Support Windows OS in ExecutorDiskUtils
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #28940:
URL: https://github.com/apache/spark/pull/28940#issuecomment-652767692
**[Test build #124859 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/124859/testReport)** for PR 28940 at commit [`a80bd6c`](https://github.com/apache/spark/commit/a80bd6c8a5d93187cf06f941c2a9d296a7b6ca61).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] attilapiros commented on a change in pull request #28940: [SPARK-32121][SHUFFLE][TEST] Fix ExternalShuffleBlockResolverSuite failed on Windows
Posted by GitBox <gi...@apache.org>.
attilapiros commented on a change in pull request #28940:
URL: https://github.com/apache/spark/pull/28940#discussion_r447551067
##########
File path: common/network-shuffle/src/main/java/org/apache/spark/network/shuffle/ExecutorDiskUtils.java
##########
@@ -50,14 +58,18 @@ public static File getFile(String[] localDirs, int subDirsPerLocalDir, String fi
* 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 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.endsWith(File.separator)) {
Review comment:
Nit: (micro-optimization) `pathname(pathname.length() - 1) == File.separatorChar` is more efficient then `endsWith` which uses two strings.
##########
File path: common/network-shuffle/src/test/java/org/apache/spark/network/shuffle/ExternalShuffleBlockResolverSuite.java
##########
@@ -146,12 +147,23 @@ public void jsonSerializationOfExecutorRegistration() throws IOException {
@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("/", "/", "/", "/");
+ assertPathsMatch("/foo", "bar", "baz",
+ File.separator + "foo" + File.separator + "bar" + File.separator + "baz");
Review comment:
Nit: the `File.separator + "foo" + File.separator + "bar" + File.separator + "baz"` is repeating 4 times I suggest to extract it into a variable/constant for readability.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on pull request #28940: [SPARK-32121][SHUFFLE] Support Windows OS in ExecutorDiskUtils
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #28940:
URL: https://github.com/apache/spark/pull/28940#issuecomment-652747385
**[Test build #124850 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/124850/testReport)** for PR 28940 at commit [`a80bd6c`](https://github.com/apache/spark/commit/a80bd6c8a5d93187cf06f941c2a9d296a7b6ca61).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on pull request #28940: [SPARK-32121][SHUFFLE] Support Windows OS in ExecutorDiskUtils
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #28940:
URL: https://github.com/apache/spark/pull/28940#issuecomment-652825939
Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/124859/
Test FAILed.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] attilapiros commented on pull request #28940: [SPARK-32121][SHUFFLE][TEST] Fix ExternalShuffleBlockResolverSuite failed on Windows
Posted by GitBox <gi...@apache.org>.
attilapiros commented on pull request #28940:
URL: https://github.com/apache/spark/pull/28940#issuecomment-651747960
What about the following?
As `createNormalizedInternedPathname` is only used here in production:
https://github.com/apache/spark/blob/50a742da1c48bcfb6f69b319c9a0142801c959c6/common/network-shuffle/src/main/java/org/apache/spark/network/shuffle/ExecutorDiskUtils.java#L48-L49
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:
```java
public File(String pathname) {
if (pathname == null) {
throw new NullPointerException();
}
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). Moreover we can get rid of the unnecessary tests.
@Ngone51 @HyukjinKwon what's your opinion?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] HyukjinKwon commented on pull request #28940: [SPARK-32121][SHUFFLE] Support Windows OS in ExecutorDiskUtils
Posted by GitBox <gi...@apache.org>.
HyukjinKwon commented on pull request #28940:
URL: https://github.com/apache/spark/pull/28940#issuecomment-652310096
retest this please
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] HyukjinKwon commented on a change in pull request #28940: [SPARK-32121][SHUFFLE][TEST] Fix ExternalShuffleBlockResolverSuite failed on Windows
Posted by GitBox <gi...@apache.org>.
HyukjinKwon commented on a change in pull request #28940:
URL: https://github.com/apache/spark/pull/28940#discussion_r446739587
##########
File path: common/network-shuffle/src/main/java/org/apache/spark/network/shuffle/ExecutorDiskUtils.java
##########
@@ -27,7 +27,7 @@
public class ExecutorDiskUtils {
- private static final Pattern MULTIPLE_SEPARATORS = Pattern.compile(File.separator + "{2,}");
+ private static final Pattern MULTIPLE_SEPARATORS = Pattern.compile("[/\\\\]+");
Review comment:
Can we add another test case where there are three file separators?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on pull request #28940: [SPARK-32121][SHUFFLE][TEST] Fix ExternalShuffleBlockResolverSuite failed on Windows
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #28940:
URL: https://github.com/apache/spark/pull/28940#issuecomment-650870133
**[Test build #124618 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/124618/testReport)** for PR 28940 at commit [`187e813`](https://github.com/apache/spark/commit/187e8133522ee15a608a6473aa089086e224a88f).
* This patch **fails build dependency tests**.
* This patch merges cleanly.
* This patch adds no public classes.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on pull request #28940: [SPARK-32121][SHUFFLE] Support Windows OS in ExecutorDiskUtils
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #28940:
URL: https://github.com/apache/spark/pull/28940#issuecomment-652408737
Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/124783/
Test FAILed.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on pull request #28940: [SPARK-32121][SHUFFLE] Support Windows OS in ExecutorDiskUtils
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #28940:
URL: https://github.com/apache/spark/pull/28940#issuecomment-652401587
**[Test build #124765 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/124765/testReport)** for PR 28940 at commit [`a80bd6c`](https://github.com/apache/spark/commit/a80bd6c8a5d93187cf06f941c2a9d296a7b6ca61).
* This patch **fails Spark unit tests**.
* This patch merges cleanly.
* This patch adds no public classes.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins commented on pull request #28940: [SPARK-32121][SHUFFLE] Support Windows OS in ExecutorDiskUtils
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #28940:
URL: https://github.com/apache/spark/pull/28940#issuecomment-652188378
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on pull request #28940: [SPARK-32121][SHUFFLE] Support Windows OS in ExecutorDiskUtils
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #28940:
URL: https://github.com/apache/spark/pull/28940#issuecomment-652040889
Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/124656/
Test FAILed.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins commented on pull request #28940: [SPARK-32121][SHUFFLE] Support Windows OS in ExecutorDiskUtils
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #28940:
URL: https://github.com/apache/spark/pull/28940#issuecomment-652209724
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] HyukjinKwon commented on a change in pull request #28940: [SPARK-32121][SHUFFLE][TEST] Fix ExternalShuffleBlockResolverSuite failed on Windows
Posted by GitBox <gi...@apache.org>.
HyukjinKwon commented on a change in pull request #28940:
URL: https://github.com/apache/spark/pull/28940#discussion_r446887373
##########
File path: common/network-shuffle/src/main/java/org/apache/spark/network/shuffle/ExecutorDiskUtils.java
##########
@@ -27,7 +27,7 @@
public class ExecutorDiskUtils {
- private static final Pattern MULTIPLE_SEPARATORS = Pattern.compile(File.separator + "{2,}");
+ private static final Pattern MULTIPLE_SEPARATORS = Pattern.compile("[/\\\\]+");
Review comment:
Hm, that one would probably need to fix too. I sort of audited and managed to make all tests pass on Windows .. but seems like there are a bunch of new instances to fix again ..
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] HyukjinKwon commented on pull request #28940: [SPARK-32121][SHUFFLE] Support Windows OS in ExecutorDiskUtils
Posted by GitBox <gi...@apache.org>.
HyukjinKwon commented on pull request #28940:
URL: https://github.com/apache/spark/pull/28940#issuecomment-652746381
retest this please
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on pull request #28940: [SPARK-32121][SHUFFLE] Support Windows OS in ExecutorDiskUtils
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #28940:
URL: https://github.com/apache/spark/pull/28940#issuecomment-652694366
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] pan3793 commented on a change in pull request #28940: [SPARK-32121][SHUFFLE][TEST] Fix ExternalShuffleBlockResolverSuite failed on Windows
Posted by GitBox <gi...@apache.org>.
pan3793 commented on a change in pull request #28940:
URL: https://github.com/apache/spark/pull/28940#discussion_r446953127
##########
File path: common/network-shuffle/src/main/java/org/apache/spark/network/shuffle/ExecutorDiskUtils.java
##########
@@ -41,6 +48,13 @@ public static File getFile(String[] localDirs, int subDirsPerLocalDir, String fi
localDir, String.format("%02x", subDirId), filename));
}
+ /** Returns whether the OS is Windows. */
+ @VisibleForTesting
+ static boolean isWindows() {
+ String os = System.getProperty("os.name");
+ return os.startsWith("Windows");
+ }
Review comment:
Sounds good.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA removed a comment on pull request #28940: [SPARK-32121][SHUFFLE] Support Windows OS in ExecutorDiskUtils
Posted by GitBox <gi...@apache.org>.
SparkQA removed a comment on pull request #28940:
URL: https://github.com/apache/spark/pull/28940#issuecomment-652142021
**[Test build #124722 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/124722/testReport)** for PR 28940 at commit [`aac01ca`](https://github.com/apache/spark/commit/aac01ca11c3c024e8a75753e43a217cadb1d8c46).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA removed a comment on pull request #28940: [SPARK-32121][SHUFFLE] Support Windows OS in ExecutorDiskUtils
Posted by GitBox <gi...@apache.org>.
SparkQA removed a comment on pull request #28940:
URL: https://github.com/apache/spark/pull/28940#issuecomment-652827291
**[Test build #124873 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/124873/testReport)** for PR 28940 at commit [`a80bd6c`](https://github.com/apache/spark/commit/a80bd6c8a5d93187cf06f941c2a9d296a7b6ca61).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on pull request #28940: [SPARK-32121][SHUFFLE] Support Windows OS in ExecutorDiskUtils
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #28940:
URL: https://github.com/apache/spark/pull/28940#issuecomment-652732489
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins commented on pull request #28940: [SPARK-32121][SHUFFLE] Support Windows OS in ExecutorDiskUtils
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #28940:
URL: https://github.com/apache/spark/pull/28940#issuecomment-652694435
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on pull request #28940: [SPARK-32121][SHUFFLE] Support Windows OS in ExecutorDiskUtils
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #28940:
URL: https://github.com/apache/spark/pull/28940#issuecomment-652039812
**[Test build #124656 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/124656/testReport)** for PR 28940 at commit [`aac01ca`](https://github.com/apache/spark/commit/aac01ca11c3c024e8a75753e43a217cadb1d8c46).
* This patch **fails Spark unit tests**.
* This patch merges cleanly.
* This patch adds no public classes.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA removed a comment on pull request #28940: [SPARK-32121][SHUFFLE][TEST] Fix ExternalShuffleBlockResolverSuite failed on Windows
Posted by GitBox <gi...@apache.org>.
SparkQA removed a comment on pull request #28940:
URL: https://github.com/apache/spark/pull/28940#issuecomment-650869741
**[Test build #124618 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/124618/testReport)** for PR 28940 at commit [`187e813`](https://github.com/apache/spark/commit/187e8133522ee15a608a6473aa089086e224a88f).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins commented on pull request #28940: [SPARK-32121][SHUFFLE] Support Windows OS in ExecutorDiskUtils
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #28940:
URL: https://github.com/apache/spark/pull/28940#issuecomment-652921860
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on pull request #28940: [SPARK-32121][SHUFFLE] Support Windows OS in ExecutorDiskUtils
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #28940:
URL: https://github.com/apache/spark/pull/28940#issuecomment-652403204
Merged build finished. Test FAILed.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA removed a comment on pull request #28940: [SPARK-32121][SHUFFLE] Support Windows OS in ExecutorDiskUtils
Posted by GitBox <gi...@apache.org>.
SparkQA removed a comment on pull request #28940:
URL: https://github.com/apache/spark/pull/28940#issuecomment-652747385
**[Test build #124850 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/124850/testReport)** for PR 28940 at commit [`a80bd6c`](https://github.com/apache/spark/commit/a80bd6c8a5d93187cf06f941c2a9d296a7b6ca61).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on pull request #28940: [SPARK-32121][SHUFFLE] Support Windows OS in ExecutorDiskUtils
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #28940:
URL: https://github.com/apache/spark/pull/28940#issuecomment-652734649
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] HyukjinKwon commented on pull request #28940: [SPARK-32121][SHUFFLE] Support Windows OS in ExecutorDiskUtils
Posted by GitBox <gi...@apache.org>.
HyukjinKwon commented on pull request #28940:
URL: https://github.com/apache/spark/pull/28940#issuecomment-652734124
retest this please
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] HyukjinKwon commented on a change in pull request #28940: [SPARK-32121][SHUFFLE][TEST] Fix ExternalShuffleBlockResolverSuite failed on Windows
Posted by GitBox <gi...@apache.org>.
HyukjinKwon commented on a change in pull request #28940:
URL: https://github.com/apache/spark/pull/28940#discussion_r446736284
##########
File path: common/network-shuffle/src/test/java/org/apache/spark/network/shuffle/ExternalShuffleBlockResolverSuite.java
##########
@@ -146,12 +146,18 @@ public void jsonSerializationOfExecutorRegistration() throws IOException {
@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("/", "/", "/", "/");
+ assertPathsMatch("/foo", "bar", "baz",
Review comment:
Let's use a string interpolation here
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] attilapiros commented on pull request #28940: [SPARK-32121][SHUFFLE][TEST] Fix ExternalShuffleBlockResolverSuite failed on Windows
Posted by GitBox <gi...@apache.org>.
attilapiros commented on pull request #28940:
URL: https://github.com/apache/spark/pull/28940#issuecomment-651772632
We have to measure it for sure.
Regarding simplicity of the solution I described it is basically sth like:
https://github.com/attilapiros/spark/commit/926ebbd417f49fe2897bf29dfaa3b17178bbbdaf
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
For sure we can do it in separate PR too.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins commented on pull request #28940: [SPARK-32121][SHUFFLE] Support Windows OS in ExecutorDiskUtils
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #28940:
URL: https://github.com/apache/spark/pull/28940#issuecomment-652408620
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins commented on pull request #28940: [SPARK-32121][SHUFFLE][TEST] Fix ExternalShuffleBlockResolverSuite failed on Windows
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #28940:
URL: https://github.com/apache/spark/pull/28940#issuecomment-651294806
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/124643/
Test PASSed.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on pull request #28940: [SPARK-32121][SHUFFLE][TEST] Fix ExternalShuffleBlockResolverSuite failed on Windows
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #28940:
URL: https://github.com/apache/spark/pull/28940#issuecomment-650987314
Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/124627/
Test FAILed.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] attilapiros commented on a change in pull request #28940: [SPARK-32121][SHUFFLE][TEST] Fix ExternalShuffleBlockResolverSuite failed on Windows
Posted by GitBox <gi...@apache.org>.
attilapiros commented on a change in pull request #28940:
URL: https://github.com/apache/spark/pull/28940#discussion_r446926688
##########
File path: common/network-shuffle/src/test/java/org/apache/spark/network/shuffle/ExternalShuffleBlockResolverSuite.java
##########
@@ -146,12 +146,25 @@ public void jsonSerializationOfExecutorRegistration() throws IOException {
@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("/", "/", "/", "/");
+ assertPathsMatch("/foo", "bar", "baz",
Review comment:
`/` != `\`
I talked about `/` and you example is `\`.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on pull request #28940: [SPARK-32121][SHUFFLE][TEST] Fix ExternalShuffleBlockResolverSuite failed on Windows
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #28940:
URL: https://github.com/apache/spark/pull/28940#issuecomment-650871137
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] HyukjinKwon commented on pull request #28940: [SPARK-32121][SHUFFLE] Support Windows OS in ExecutorDiskUtils
Posted by GitBox <gi...@apache.org>.
HyukjinKwon commented on pull request #28940:
URL: https://github.com/apache/spark/pull/28940#issuecomment-652141341
retest this please
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] HyukjinKwon commented on a change in pull request #28940: [SPARK-32121][SHUFFLE][TEST] Fix ExternalShuffleBlockResolverSuite failed on Windows
Posted by GitBox <gi...@apache.org>.
HyukjinKwon commented on a change in pull request #28940:
URL: https://github.com/apache/spark/pull/28940#discussion_r446891403
##########
File path: common/network-shuffle/src/main/java/org/apache/spark/network/shuffle/ExecutorDiskUtils.java
##########
@@ -27,7 +27,7 @@
public class ExecutorDiskUtils {
- private static final Pattern MULTIPLE_SEPARATORS = Pattern.compile(File.separator + "{2,}");
+ private static final Pattern MULTIPLE_SEPARATORS = Pattern.compile("[/\\\\]+");
Review comment:
I have a sort of half automated script to run the tests on Windows via CI via AppVeyor which I used at that time to audit the tests on Windows. Let me try to run and see if that works:
Build started: [CORE] `org.apache.spark.network.shuffle.ExternalShuffleBlockResolverSuite` [](https://ci.appveyor.com/project/spark-test/spark/branch/6BE4E4C8-09CE-4166-85DD-53C91C022221)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] HyukjinKwon commented on a change in pull request #28940: [SPARK-32121][SHUFFLE][TEST] Fix ExternalShuffleBlockResolverSuite failed on Windows
Posted by GitBox <gi...@apache.org>.
HyukjinKwon commented on a change in pull request #28940:
URL: https://github.com/apache/spark/pull/28940#discussion_r446739907
##########
File path: common/network-shuffle/src/main/java/org/apache/spark/network/shuffle/ExecutorDiskUtils.java
##########
@@ -27,7 +27,7 @@
public class ExecutorDiskUtils {
- private static final Pattern MULTIPLE_SEPARATORS = Pattern.compile(File.separator + "{2,}");
+ private static final Pattern MULTIPLE_SEPARATORS = Pattern.compile("[/\\\\]+");
Review comment:
@attilapiros was this simply a mistake or intentionally used 2 to match? Seems the current change is correct but wanted make doubly sure.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] HyukjinKwon commented on a change in pull request #28940: [SPARK-32121][SHUFFLE][TEST] Fix ExternalShuffleBlockResolverSuite failed on Windows
Posted by GitBox <gi...@apache.org>.
HyukjinKwon commented on a change in pull request #28940:
URL: https://github.com/apache/spark/pull/28940#discussion_r446887373
##########
File path: common/network-shuffle/src/main/java/org/apache/spark/network/shuffle/ExecutorDiskUtils.java
##########
@@ -27,7 +27,7 @@
public class ExecutorDiskUtils {
- private static final Pattern MULTIPLE_SEPARATORS = Pattern.compile(File.separator + "{2,}");
+ private static final Pattern MULTIPLE_SEPARATORS = Pattern.compile("[/\\\\]+");
Review comment:
Hm, that one probably had to fix. I sort of audited and managed to make all tests pass on Windows .. but seems like there are a bunch of new instances to fix again ..
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] pan3793 commented on a change in pull request #28940: [SPARK-32121][SHUFFLE][TEST] Fix ExternalShuffleBlockResolverSuite failed on Windows
Posted by GitBox <gi...@apache.org>.
pan3793 commented on a change in pull request #28940:
URL: https://github.com/apache/spark/pull/28940#discussion_r446953127
##########
File path: common/network-shuffle/src/main/java/org/apache/spark/network/shuffle/ExecutorDiskUtils.java
##########
@@ -41,6 +48,13 @@ public static File getFile(String[] localDirs, int subDirsPerLocalDir, String fi
localDir, String.format("%02x", subDirId), filename));
}
+ /** Returns whether the OS is Windows. */
+ @VisibleForTesting
+ static boolean isWindows() {
+ String os = System.getProperty("os.name");
+ return os.startsWith("Windows");
+ }
Review comment:
Sound good.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] pan3793 commented on a change in pull request #28940: [SPARK-32121][SHUFFLE][TEST] Fix ExternalShuffleBlockResolverSuite failed on Windows
Posted by GitBox <gi...@apache.org>.
pan3793 commented on a change in pull request #28940:
URL: https://github.com/apache/spark/pull/28940#discussion_r446936981
##########
File path: common/network-shuffle/src/test/java/org/apache/spark/network/shuffle/ExternalShuffleBlockResolverSuite.java
##########
@@ -146,12 +146,25 @@ public void jsonSerializationOfExecutorRegistration() throws IOException {
@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("/", "/", "/", "/");
+ assertPathsMatch("/foo", "bar", "baz",
Review comment:
What I means is: the purpose of this method is to `normalize` the path, even `/` and `\` both are acceptable sep on Windows, the `normalized` sep should be `\`. Just like `/a//b` and `/a/b` are both valid and same path on Unix, but the `normalized` expression is `/a/b`
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] attilapiros commented on a change in pull request #28940: [SPARK-32121][SHUFFLE][TEST] Fix ExternalShuffleBlockResolverSuite failed on Windows
Posted by GitBox <gi...@apache.org>.
attilapiros commented on a change in pull request #28940:
URL: https://github.com/apache/spark/pull/28940#discussion_r446951724
##########
File path: common/network-shuffle/src/test/java/org/apache/spark/network/shuffle/ExternalShuffleBlockResolverSuite.java
##########
@@ -146,12 +146,25 @@ public void jsonSerializationOfExecutorRegistration() throws IOException {
@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("/", "/", "/", "/");
+ assertPathsMatch("/foo", "bar", "baz",
Review comment:
Sure :)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] attilapiros commented on a change in pull request #28940: [SPARK-32121][SHUFFLE][TEST] Fix ExternalShuffleBlockResolverSuite failed on Windows
Posted by GitBox <gi...@apache.org>.
attilapiros commented on a change in pull request #28940:
URL: https://github.com/apache/spark/pull/28940#discussion_r446932602
##########
File path: common/network-shuffle/src/test/java/org/apache/spark/network/shuffle/ExternalShuffleBlockResolverSuite.java
##########
@@ -146,12 +146,25 @@ public void jsonSerializationOfExecutorRegistration() throws IOException {
@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("/", "/", "/", "/");
+ assertPathsMatch("/foo", "bar", "baz",
Review comment:
Then your comment is invalid, right?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on pull request #28940: [SPARK-32121][SHUFFLE] Support Windows OS in ExecutorDiskUtils
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #28940:
URL: https://github.com/apache/spark/pull/28940#issuecomment-652734306
**[Test build #124840 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/124840/testReport)** for PR 28940 at commit [`a80bd6c`](https://github.com/apache/spark/commit/a80bd6c8a5d93187cf06f941c2a9d296a7b6ca61).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins commented on pull request #28940: [SPARK-32121][SHUFFLE] Support Windows OS in ExecutorDiskUtils
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #28940:
URL: https://github.com/apache/spark/pull/28940#issuecomment-652828058
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on pull request #28940: [SPARK-32121][SHUFFLE][TEST] Fix ExternalShuffleBlockResolverSuite failed on Windows
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #28940:
URL: https://github.com/apache/spark/pull/28940#issuecomment-650987308
Merged build finished. Test FAILed.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins commented on pull request #28940: [SPARK-32121][SHUFFLE] Support Windows OS in ExecutorDiskUtils
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #28940:
URL: https://github.com/apache/spark/pull/28940#issuecomment-652311263
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins commented on pull request #28940: [SPARK-32121][SHUFFLE][TEST] Fix ExternalShuffleBlockResolverSuite failed on Windows
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #28940:
URL: https://github.com/apache/spark/pull/28940#issuecomment-650870138
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA removed a comment on pull request #28940: [SPARK-32121][SHUFFLE] Support Windows OS in ExecutorDiskUtils
Posted by GitBox <gi...@apache.org>.
SparkQA removed a comment on pull request #28940:
URL: https://github.com/apache/spark/pull/28940#issuecomment-652458988
**[Test build #124790 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/124790/testReport)** for PR 28940 at commit [`a80bd6c`](https://github.com/apache/spark/commit/a80bd6c8a5d93187cf06f941c2a9d296a7b6ca61).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA removed a comment on pull request #28940: [SPARK-32121][SHUFFLE] Support Windows OS in ExecutorDiskUtils
Posted by GitBox <gi...@apache.org>.
SparkQA removed a comment on pull request #28940:
URL: https://github.com/apache/spark/pull/28940#issuecomment-652732196
**[Test build #124838 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/124838/testReport)** for PR 28940 at commit [`a80bd6c`](https://github.com/apache/spark/commit/a80bd6c8a5d93187cf06f941c2a9d296a7b6ca61).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on pull request #28940: [SPARK-32121][SHUFFLE] Support Windows OS in ExecutorDiskUtils
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #28940:
URL: https://github.com/apache/spark/pull/28940#issuecomment-652567994
**[Test build #124790 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/124790/testReport)** for PR 28940 at commit [`a80bd6c`](https://github.com/apache/spark/commit/a80bd6c8a5d93187cf06f941c2a9d296a7b6ca61).
* This patch **fails PySpark unit tests**.
* This patch merges cleanly.
* This patch adds no public classes.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] HyukjinKwon closed pull request #28940: [SPARK-32121][SHUFFLE] Support Windows OS in ExecutorDiskUtils
Posted by GitBox <gi...@apache.org>.
HyukjinKwon closed pull request #28940:
URL: https://github.com/apache/spark/pull/28940
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] HyukjinKwon commented on pull request #28940: [SPARK-32121][SHUFFLE][TEST] Fix ExternalShuffleBlockResolverSuite failed on Windows
Posted by GitBox <gi...@apache.org>.
HyukjinKwon commented on pull request #28940:
URL: https://github.com/apache/spark/pull/28940#issuecomment-651759667
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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on pull request #28940: [SPARK-32121][SHUFFLE] Support Windows OS in ExecutorDiskUtils
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #28940:
URL: https://github.com/apache/spark/pull/28940#issuecomment-652694354
**[Test build #124827 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/124827/testReport)** for PR 28940 at commit [`a80bd6c`](https://github.com/apache/spark/commit/a80bd6c8a5d93187cf06f941c2a9d296a7b6ca61).
* This patch **fails build dependency tests**.
* This patch merges cleanly.
* This patch adds no public classes.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins commented on pull request #28940: [SPARK-32121][SHUFFLE][TEST] Fix ExternalShuffleBlockResolverSuite failed on Windows
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #28940:
URL: https://github.com/apache/spark/pull/28940#issuecomment-650981021
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org