New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Disable socket factory for pre JDK11 #2633
Disable socket factory for pre JDK11 #2633
Conversation
should this change (disabling |
@rohangarg Good catch. Thanks! The same problem happens for presto-jdbc too. |
27c981f
to
0ec30fe
Compare
Checkstyle error happening in CI seems to be unrelated to the change.
|
Could you rebase and push again? It should be fixed in da29dad. |
0ec30fe
to
56adbcc
Compare
There's a build failure:
The change looks good. Can you squash the commits? I'll merge it once you do that and the build passes. |
9f0054f
to
4d74f15
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let’s simplify the JDK 11 detection to be more robust.
|
||
// TODO: remove this when we upgrade to Java 9 (replace with java.lang.Runtime.getVersion()) | ||
// It's copied from io.prestosql.server module. | ||
public class JavaVersion |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I’m concerned about having this complex code (which has had bugs in the past) as part of the JDBC driver, which runs in various environments. We also plan to keep the driver on Java 8 for the foreseeable future, so this code won’t be removed anytime soon.
Some alternatives:
-
Just split on a period and take the first item, since that works for all versions.
-
Use reflection to check for and call the
Runtime.Version
API. We can check for and use thefeature
method which is in JDK 10+. -
Use reflection to look for the existence of a “marker” class like
java.net.http.HttpClient
which was added in JDK 11.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the code using Runtime.version()
reflectively:
@SuppressWarnings("JavaReflectionMemberAccess")
private static boolean isAtLeastJava11()
{
try {
Method versionMethod = Runtime.class.getMethod("version");
Method featureMethod = versionMethod.getReturnType().getMethod("feature");
Object version = versionMethod.invoke(null);
int feature = (int) featureMethod.invoke(version);
return feature >= 11;
}
catch (ReflectiveOperationException e) {
return false;
}
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code to safely parse the version:
private static boolean isAtLeastJava11()
{
String feature = Splitter.on(".").split(StandardSystemProperty.JAVA_VERSION.value()).next();
try {
return Integer.parseInt(feature) >= 11;
}
catch (NumberFormatException e) {
return false;
}
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dain agrees we should go with option 1, since that's simple and reliable
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please squash the commits when updating
1d8e60f
to
9188182
Compare
9188182
to
9927836
Compare
Thanks! |
Purpose
As discussed in #1169, the socket channel is the cause of the slowdown with JDK 11. As JDK-8131133 is fixed from JDK 9, the workaround is not necessary anymore. We can safely disable the socket factory usage if the runtime is JDK 11 or later. Confirmed the communication between server and client works well even with HTTP 2.
Overview
JavaVersion
class from presto-server module to parse Java version string. Although we expected to useRuntime.getVersion
, it does not exist in pre-JDK 9.