[SPARK-14914] Skip some test cases on Windows due to limitation of Windows #12696
Conversation
| @@ -947,7 +945,9 @@ private[spark] object Utils extends Logging { | |||
| */ | |||
| def isSymlink(file: File): Boolean = { | |||
| if (file == null) throw new NullPointerException("File must not be null") | |||
| if (isWindows) return false | |||
| if (isWindows) { | |||
| return Files.isSymbolicLink(Paths.get(file.toURI)) | |||
There was a problem hiding this comment.
Let's just invoke this method as the body of this method, now that we can use Java 7, for Windows and all platforms.
| val sparkSubmit = if (Utils.isWindows) { | ||
| Seq(new File("..\\bin\\spark-submit.cmd").getAbsolutePath) ++ args | ||
| } else { | ||
| Seq("./bin/spark-submit") ++ args |
There was a problem hiding this comment.
Everything else but the path is the same right? the rest of these blocks don't need to be duplicated, even though it's very minor.
There was a problem hiding this comment.
OK this PR looks fine to me. My last suggested change would be to simply this changed bit of code to something like ...
val sparkSubmitPath = if (Utils.isWindows) "..." else "..."
...
val process = Utils.executeCommand(
sparkSubmitPath.getAbsolutePath +: args,
...
I don't feel strongly about it
There was a problem hiding this comment.
(Is getCanonicalPath a little safer to go ahead and resolve the ".."? not sure)
There was a problem hiding this comment.
@srowen I found out that if we do new File("../bin/spark-submit").getCanonicalPath, the path returned from Windows and Linux would be the same. Would you mind if I make them consistent?
There was a problem hiding this comment.
Sure, consistent is good. (Could be very slightly simpler with +: etc but this is really trivial)
|
Jenkins test this please |
|
Test build #57763 has finished for PR 12696 at commit
|
|
@taoli91 Do you mind if I ask rebase this? I would like to support to verify this PR. |
|
@taoli91 Would you be interested in https://issues.apache.org/jira/browse/SPARK-17591? |
|
@HyukjinKwon if you want to move this forward and there's no reply, I think you could fork this and continue it. |
|
@srowen Sure, I will wait for a few days and then try to proceed this. |
What changes were proposed in this pull request?
This pull request fix some general issue on windows:
How was this patch tested?
Unit tests (Not fully passed on windows)