[SPARK-13278][CORE] Launcher fails to start with JDK 9 EA #11160
Conversation
|
ok to test |
|
LGTM pending tests. |
| * Get the major version of the java.version string supplied. | ||
| */ | ||
| static int javaMajorVersion(String javaVersion) { | ||
| String[] version = javaVersion.split("[+.\\-]+"); |
There was a problem hiding this comment.
this is a super minor point, but I don't think we need to split on anything other than "." even with non-JEPS223 strings since we only want to extract the major version.
There was a problem hiding this comment.
But for early access version, e.g., 9-ea, the previous codes will throw an exception.
There was a problem hiding this comment.
But they won't since we only try and access the second element of the array if the first element is less than or equal to 1.
There was a problem hiding this comment.
For 9-ea the original split would give you an array whose first element is "9-ea", Integer.parseInt("9-ea") throws NumberFormatException. Feel free to try revert to the old regex and run the test cases I added to verify this.
There was a problem hiding this comment.
I meant Integer.parseInt("9-ea") will throw NumberFormatException.
There was a problem hiding this comment.
oh yah sorry not enough coffee for me today, I guess we need .- but not + (although if we did use java.runtime.version we would need the +) and one of the samples in the test "9+100" shouldn't show up in java.version but should in java.runtime.version
There was a problem hiding this comment.
Ah, yes, you're right that handling + is probably superfluous for java.version. I can prune that from the commit if you want, but doesn't seem to do any harm to leave it in for completeness.
There was a problem hiding this comment.
It probably isn't much harm; the only concern would be that it split string, the test and the comment together imply a different input format than the one specified. It might be slightly nicer to have it match the spec since its just a few characters difference. (but a super minor point)
There was a problem hiding this comment.
How about just splitting on non-numbers? It's all kind of a theoretical difference though. This looks OK.
|
Test build #51068 has finished for PR 11160 at commit
|
|
I think |
|
I tried building with JDK 9 EA, and there are more immediate issues such as -XX:MaxPermSize (which was deprecated/removed in 8 and fails to start in 9) being used in various places. The quick fix would be to add Runtime support is of a more immediate concern to us, though, as it allows the OpenJDK project to use Spark itself as a functional test/benchmark. Sadly JEP-223 has stopped that effort for now. |
|
While not a general rule, testing lexically that |
|
Test build #51221 has finished for PR 11160 at commit
|
| val Array(major, minor, _) = System.getProperty("java.version").split("\\.", 3) | ||
| if (major.toInt >= 1 && minor.toInt >= 8) Seq("-Xdoclint:all", "-Xdoclint:-missing") else Seq.empty | ||
| val version = System.getProperty("java.version") | ||
| if (version >= "1.8.0") Seq("-Xdoclint:all", "-Xdoclint:-missing") else Seq.empty |
There was a problem hiding this comment.
Hm, this only happens to work for 1.8 vs 9. This seems hacky compared to just emulating the simple logic you wrote above -- why not that?
|
I think I think the code and scripts attempt to not set MaxPermSize for Java 8 already. If it doesn't somewhere, that also needs to be patched up. But my impression is this should already be the case |
|
This should likely be refactored to a place where both sbt and test code can access it, but I hope this is good enough. |
|
Test build #51241 has finished for PR 11160 at commit
|
|
LGTM |
|
Merged to master |
See http://openjdk.java.net/jeps/223 for more information about the JDK 9 version string scheme.