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
DRILL-6349: Drill JDBC driver fails on Java 1.9+ with NoClassDefFoundError: sun/misc/VM #1446
Conversation
@oleg-zinovev can you verify for 1.9 as well? Why failed tests cannot be fixed? |
@@ -69,6 +69,8 @@ public String build(String jarName, String buildDirectory, String includeFiles, | |||
List<String> params = new LinkedList<>(); | |||
params.add("clean"); | |||
params.add("package"); | |||
params.add("-Dmaven.compiler.source=1.6"); |
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 use 1.8
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.
Ok, by default was a 1.5
} | ||
|
||
|
||
private static long maxDirectMemory0() { |
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.
Why method is named with 0 in the end?
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.
Just a copy-paste from netty PlatformDependent.
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 add javadoc for this class explaining its purpose.
- I still suggest to rename the method.
- Maybe add trace for the ignored exception, just in case we might need to find out what has happened.
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.
Why is copy/paste necessary (why not to use netty PlatformDependent
directly)?
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.
netty PlatformDependent do not try to access jdk.internal.misc.VM.
By "copy-paste" i mean only a method name
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.
If netty PlatformDependent
uses wrong max direct memory, will not netty memory allocation be affected that may cause OOM in Drill?
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.
Looks like it can cause OOM:
In Drill version of PooledByteBufAllocatorL
exists a reflective access to PooledByteBufAllocator.directArenas
field.
It's size depends on
DEFAULT_NUM_DIRECT_ARENA = Math.max(0, SystemPropertyUtil.getInt( "io.netty.allocator.numDirectArenas", (int) Math.min( defaultMinNumArena, PlatformDependent.maxDirectMemory() / defaultChunkSize / 2 / 3)));
Should i use a netty PlatformDependent
, or "override" it (as PooledByteBufAllocatorL
) ?
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.
Drill delegates to netty all large memory allocation and netty internally tracks how much direct memory is available. If netty incorrectly determines max direct memory size or Drill and netty max direct memory sizes are different, it is easy to get into a situation where netty will fail allocation when memory is available or it will try to allocate when memory is not available.
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.
netty PlatformDependent
and jdk.internal.misc.VM
returns same value for maxDirectMemory, so i remove my custom class
Can you verify for 1.9 as well? Why failed tests cannot be fixed? |
@oleg-zinovev did you verify for open-jdk? |
@oleg-zinovev did you verify for open-jdk? |
c1a50d9
to
fb758d1
Compare
At least Kafka and HBase tests can not be fixed under Java 9+ |
build works with Java 9, except hive and kafka tests (HBase tests surprisely works) |
@oleg-zinovev besides comment in the PR I suggest a couple more things:
|
@@ -215,6 +215,12 @@ SET JAVA_CMD=%JAVA_HOME%\bin\%JAVA_EXE% | |||
if "%JAVA_HOME%" == "" (set JAVA_CMD=%JAVA_EXE%) | |||
set ERROR_CODE=0 | |||
|
|||
for /f tokens^=2-5^ delims^=.-_^" %%j in ('"%JAVA_CMD%" -fullversion 2^>^&1') do set "version=%%j%%k" |
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.
LF/CR?
@@ -69,6 +69,8 @@ public String build(String jarName, String buildDirectory, String includeFiles, | |||
List<String> params = new LinkedList<>(); | |||
params.add("clean"); | |||
params.add("package"); | |||
params.add("-Dmaven.compiler.source=1.8"); |
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.
use pom.xml
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.
fixed
@@ -179,7 +179,7 @@ protected void createPartitionSublists() { | |||
|
|||
// build a list of DFSDirPartitionLocation. | |||
for (final List<String> dirs : dirToFileMap.keySet()) { | |||
locations.add( new DFSDirPartitionLocation((String [])dirs.toArray(), dirToFileMap.get(dirs))); | |||
locations.add( new DFSDirPartitionLocation(dirs.toArray(new String [0]), dirToFileMap.get(dirs))); |
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.
use dirs.size()
instead of 0.
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.
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.
Thanks for the link. There is a difference in behavior of ArrayList
and Array.ArrayList
that is used in this method. In any case, if you touch this method, you will need to fix it, as there is no reason to create ArrayList
, so I'd suggest to file a separate JIRA and fix it in a separate PR.
pom.xml
Outdated
-XX:+IgnoreUnrecognizedVMOptions | ||
"--add-modules java.se" | ||
-Djdk.attach.allowAttachSelf=true | ||
-Duser.language=en |
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.
Why is this necessary?
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.
-Djdk.attach.allowAttachSelf=true - for jmockit (more info: https://bugs.java.com/view_bug.do?bug_id=8180425)
-Duser.language=en - format tests fails on non-english system locale
-XX:+IgnoreUnrecognizedVMOptions "--add-modules java.se" - revalidate it tomorrow
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.
http://openjdk.java.net/jeps/261 - java.se module available by default for unnamed modules, so -XX:+IgnoreUnrecognizedVMOptions "--add-modules java.se"
can be removed
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.
@oleg-zinovev, since this is another bug, please log a Jira and remove changes connected with locale from this PR.
ecdf283
to
147e125
Compare
<!-- <scope>test</scope> --> | ||
</dependency> | ||
<dependency> | ||
<groupId>org.ow2.asm</groupId> | ||
<artifactId>asm-util</artifactId> |
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 don't see any new dependencies being introduced as part of the PR, why is this necessary? The same for all other newly introduced dependencies.
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.
New asm version supports jdk 10, but there asm-debug-all
jar was removed (see https://gitlab.ow2.org/asm/asm/commit/4c449bea1e3fb980238c61d60103fe1be76e77e5), so new dependencies, where classes used by Drill are available were introduced.
If I understand correctly, jaxb-api
and activation
dependencies were added due to http://openjdk.java.net/jeps/320.
@oleg-zinovev, Is it make sence to move these two dependencies to the root pom?
Also, please remove commented out lines introduced in previous commits and move version for asm
dependencies to the property in the root pom.
@@ -180,7 +180,7 @@ protected void createPartitionSublists() { | |||
|
|||
// build a list of DFSDirPartitionLocation. | |||
for (final List<String> dirs : dirToFileMap.keySet()) { | |||
locations.add( new DFSDirPartitionLocation((String [])dirs.toArray(), dirToFileMap.get(dirs))); | |||
locations.add( new DFSDirPartitionLocation(dirs.toArray(new String[dirs.size()]), dirToFileMap.get(dirs))); |
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.
Why is this change necessary? As I already mentioned, I don't see why is it necessary to go back and forth between an array and a list in this method, so I would prefer not to introduce an unrelated change into the PR and handle it in a separate PR/JIRA.
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.
https://bugs.java.com/view_bug.do?bug_id=6260652
Result type of Arrays.ArrayList::toArray() was changed from T[] to Object[] in JDK 9+.
So, FileSystemPartitionDescriptor::createPartitionSublists() doesn't work if Drill running on JDK9+.
@@ -375,6 +375,11 @@ else if (NullPointerException.class == cause.getClass() | |||
|
|||
public void testAllMethods() { | |||
for (Method method : jdbcIntf.getMethods()) { | |||
if (method.isDefault()) { |
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.
Consider moving to isOkayNonthrowingMethod()
. What new default methods were introduced in java 10 that require special handling?
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.
Some method was added in JDBC 4.3 API
Additional information:
https://jcp.org/aboutJava/communityprocess/maintenance/jsr221/JDBC4.3MR-January2017.pdf
6. java.sql.Connection changes
8. java.sql.DatabaseMetaData changes
12. java.sql.Statement changes
pom.xml
Outdated
@@ -734,6 +735,9 @@ | |||
-Djava.net.preferIPv4Stack=true | |||
-Djava.awt.headless=true | |||
-XX:+CMSClassUnloadingEnabled -ea | |||
-Djdk.attach.allowAttachSelf=true | |||
-Duser.language=en |
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 fixes unit tests but opens Drill for bugs on system where language or country are different from en/US.
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 tests fails on non-english locale. Main reason is a decimal and thousand separator char.
https://drill.apache.org/docs/compiling-drill-from-source/ - I don't see any requirements about locale in documentaion. Maybe it must be an another Bug+PR ?
TestDurationFormat.testCompactSecMillis:58->validateDurationFormat:43 expected:<4[,]545s> but was:<4[.]545s>
TestDurationFormat.testCompactTwoDigitMilliSec:48->validateDurationFormat:43 expected:<0[,]045s> but was:<0[.]045s>
TestFunctionsQuery.testToCharFunction:534 » After matching 0 records, did not...
TestSelectivity.testFilterSelectivityOptions » UserRemote PARSE ERROR: Encount...
TestVarDecimalFunctions.testCastDecimalDouble:680->BaseTestQuery.testRunAndReturn:341 » Rpc
TestVarDecimalFunctions.testCastDoubleDecimal:650->BaseTestQuery.testRunAndReturn:341 » Rpc
TestVarDecimalFunctions.testDecimalToChar:731 » at position 0 column 's1
' m...
TestTopNSchemaChanges.testMissingColumn:192 » org.apache.drill.common.excepti...
TestTopNSchemaChanges.testNumericTypes:82 » org.apache.drill.common.exception...
TestTopNSchemaChanges.testUnionTypes:162 » org.apache.drill.common.exceptions...
TestMergeJoinWithSchemaChanges.testNumericStringTypes:192->BaseTestQuery.testRunAndReturn:341 » Rpc
TestMergeJoinWithSchemaChanges.testNumericTypes:114->BaseTestQuery.testRunAndReturn:341 » Rpc
TestMergeJoinWithSchemaChanges.testOneSideSchemaChanges:348->BaseTestQuery.testRunAndReturn:341 » Rpc
TestExternalSort.testNumericTypesLegacy:49->testNumericTypes:113 » org.apache...
TestExternalSort.testNumericTypesManaged:44->testNumericTypes:113 » org.apach...
TestImageRecordReader.testAviImage:101->createAndQuery:50 » at position 0 col...
TestImageRecordReader.testBmpImage:56->createAndQuery:50 » at position 0 colu...
TestImageRecordReader.testEpsImage:121->createAndQuery:50 » at position 0 col...
TestImageRecordReader.testJpegImage:71->createAndQuery:50 » at position 0 col...
TestImageRecordReader.testMovImage:111->createAndQuery:50 » at position 0 col...
TestImageRecordReader.testPngImage:81->createAndQuery:50 » at position 0 colu...
TestImageRecordReader.testPsdImage:86->createAndQuery:50 » at position 0 colu...
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.
@oleg-zinovev, yes, let's fix it in another PR. But currently to avoid unit tests failures you may change the locale on your machine.
94c099e
to
8faa57b
Compare
I got the same issue with java 10. Please also make the jdbc-driver-all available to download without downloading the complete tar.gz. Would save much time. java.lang.NoClassDefFoundError: Could not initialize class oadd.org.apache.drill.common.config.DrillConfig |
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.
@oleg-zinovev, could you please rebase onto the master and resolve conflicts?
pom.xml
Outdated
@@ -734,6 +735,9 @@ | |||
-Djava.net.preferIPv4Stack=true | |||
-Djava.awt.headless=true | |||
-XX:+CMSClassUnloadingEnabled -ea | |||
-Djdk.attach.allowAttachSelf=true | |||
-Duser.language=en |
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.
@oleg-zinovev, yes, let's fix it in another PR. But currently to avoid unit tests failures you may change the locale on your machine.
<!-- <scope>test</scope> --> | ||
</dependency> | ||
<dependency> | ||
<groupId>org.ow2.asm</groupId> | ||
<artifactId>asm-util</artifactId> |
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.
New asm version supports jdk 10, but there asm-debug-all
jar was removed (see https://gitlab.ow2.org/asm/asm/commit/4c449bea1e3fb980238c61d60103fe1be76e77e5), so new dependencies, where classes used by Drill are available were introduced.
If I understand correctly, jaxb-api
and activation
dependencies were added due to http://openjdk.java.net/jeps/320.
@oleg-zinovev, Is it make sence to move these two dependencies to the root pom?
Also, please remove commented out lines introduced in previous commits and move version for asm
dependencies to the property in the root pom.
@dprutean, yes, it looks like the same issue, the stack trace looks similar, but I don't see
|
8faa57b
to
0783358
Compare
@vvysotskyi
P.S. Time limit error for last build... |
@vvysotskyi
|
819e867
to
4b9c8c7
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.
@oleg-zinovev, looks like the issue with winutils
is caused by the difference in hadoop
and hadoop-winutils
versions.
Also, could you please hide warnings which are displayed after starting Drill in embedded mode for JDK 9+ for both Linux and Windows?
WARNING: An illegal reflective access operation has occurred
WARNING: Illegal reflective access by org.apache.calcite.avatica.com.google.protobuf.UnsafeUtil (file:/tmp/drill/distribution/target/apache-drill-1.15.0-SNAPSHOT/apache-drill-1.15.0-SNAPSHOT/jars/3rdparty/avatica-1.12.0.jar) to field java.nio.Buffer.address
WARNING: Please consider reporting this to the maintainers of org.apache.calcite.avatica.com.google.protobuf.UnsafeUtil
WARNING: Use --illegal-access=warn to enable warnings of further illegal reflective access operations
WARNING: All illegal access operations will be denied in a future release
:end | ||
endlocal | ||
exit /B %ERROR_CODE% | ||
@REM |
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 revert changes here connected with replacing CRLF
by LF
.
|
||
:end | ||
endlocal | ||
exit /B %ERROR_CODE% |
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 leave a new line here.
@vvysotskyi,
According to http://mail.openjdk.java.net/pipermail/jigsaw-dev/2017-May/012673.html there is no official way to remove this warnings. |
67c055b
to
b0e996c
Compare
@vvysotskyi What about Visual C++ Runtime library? |
@oleg-zinovev, thanks for removing those warnings. I'm seeing bug similar to the HBASE-18867:
And as it was described in that Jira it is fixed after updating |
d3b54f5
to
d4fff1d
Compare
@vvysotskyi , done. |
@oleg-zinovev, thanks for making changes. Please resolve a problem with We will wait until |
@vvysotskyi According to https://github.com/apache/hadoop/tree/master/hadoop-common-project/hadoop-common/src/main/winutils, winutils in hadoop was last modified in 2015. P.S. I will try to reproduce it with fresh Windows 10 and Drill 1.14. |
https://github.com/steveloughran/winutils contains built binaries, and according to the comment from the docs, sources are taken from the git commit ID used for the ASF release. It will be built and pushed to the one of the repositories used by Drill. Could you please clarify, what do you mean under adding a Visual C++ runtime? |
It's strange that for JDK 8 |
Perhaps this issue woun't be fixed after winutils update. I have checked with 2.8.1, and it still fails. For JDK 8 So I agree with you that we should document that for JDK 11 should be installed |
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.
@oleg-zinovev, please resolve CR comments in sqlline.bat
and maven-failsafe-plugin
, and we will merge this PR if there wouldn't be any additional issues.
@@ -104,7 +104,7 @@ | |||
<!-- Because the JDBC tests are somewhat heavyweight, we only run them in the 'verify' phase --> | |||
<groupId>org.apache.maven.plugins</groupId> | |||
<artifactId>maven-failsafe-plugin</artifactId> | |||
<version>2.18.1</version> | |||
<version>2.22.0</version> | |||
<configuration> |
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.
Could you please also set <useSystemClassLoader>false</useSystemClassLoader>
for maven-failsafe-plugin
, since it also fails.
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.
@vvysotskyi , done
43e64cf
to
dc0cd86
Compare
…Error: sun/misc/VM
dc0cd86
to
5f827c6
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.
+1
…Error: sun/misc/VM closes apache#1446
…Error: sun/misc/VM closes apache#1446
…Error: sun/misc/VM closes apache#1446
PR allows both build and run with JDK8, JDK 10 (and, likely, JDK9).
All tests, except HBase, Hive, Kafka Storage Plugin tests, works on JDK10:
Changes:
P.S. I am sorry for possible mistakes because of my bad English