-
Notifications
You must be signed in to change notification settings - Fork 3.4k
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
ARROW-6509: [Java][CI] Upgrade maven-surefire-plugin to version 3.0.0-M3, disable Gandiva JNI unit tests temporarily #5370
Conversation
Here's what the surefire docs says:
(not actually sure if this warning is related...) Are any of the Gandiva unit tests writing to stdout from native code? @pprudhvi @praveenbingo |
This SO thread suggests that surefire 3.0.0-M3 may fix the issue |
Turning off the JVM forking exposed the errors in the logs
|
These memory allocations look weird cc also @tianchen92 @liyafan82 |
@wesm I think the failure you were seeing might be because: |
added to systemPropertyValues so non-forking case can be handled.
add fallback paths for integration files.
@ursabot build |
@wesm I pushed some commits that I think will work around the brittleness in java tests when forking=0, did the upgrade of the plugin by itself also fail? I can't reproduce locally on my mac. I might try resurrecting a linux VM to see if I can reproduce the error there. |
java/pom.xml
Outdated
@@ -39,7 +39,7 @@ | |||
<dep.fbs.version>1.9.0</dep.fbs.version> | |||
<dep.flatc.version>1.9.0</dep.flatc.version> | |||
<arrow.vector.classifier></arrow.vector.classifier> | |||
<forkCount>2</forkCount> | |||
<forkCount>0</forkCount> |
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 fixing this problem.
IMO, reducing the value of forkCount seems to be the correct direction of solving the problem.
However, it also reduces the parallelism of running tests.
The original value (2) is already small enough. I am not sure if we should further reduce it.
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.
The bug still hasn't been fixed -- reducing the forkCount to 0 simply is allowing us to see the segfault / core dump that's happening...
OK, now we are getting somewhere
|
I restored the forkCount=2 and turned off the Gandiva JNI tests so we can get passing CI builds again |
Codecov Report
@@ Coverage Diff @@
## master #5370 +/- ##
=========================================
Coverage ? 88.61%
=========================================
Files ? 948
Lines ? 125732
Branches ? 1437
=========================================
Hits ? 111419
Misses ? 13951
Partials ? 362 Continue to review full report at Codecov.
|
If you filed a bug report at http://bugreport.java.com/bugreport/crash.jsp or anywhere else, could you drop a link to that here and/or on jira? |
I don't think it's a bug in the JVM, it seems likely it's caused by one of the last few Gandiva-related patches https://github.com/apache/arrow/commits/master/cpp/src/gandiva |
…-M3, disable Gandiva JNI unit tests temporarily To see if the CI issues go away. Closes apache#5370 from wesm/ARROW-6509 and squashes the following commits: dcd6980 <Wes McKinney> Disable Gandiva Java tests 92b1bee <emkornfield> use canonical file 1e3ae38 <emkornfield> use canonical file in read normalized. 9c63f96 <emkornfield> set user timezone. d805d2a <emkornfield> fix indentation. 997cea3 <emkornfield> Update TestIntegration.java 3a83fd5 <emkornfield> add path fallback for json 09a7bf9 <emkornfield> Add vector.max_allocation_bytes to config 7431585 <Wes McKinney> Do not fork JVM fb2b647 <Wes McKinney> try 3.0.0-M3 4cbeaad <Wes McKinney> Try upgrading surefire to 2.22.1 Lead-authored-by: Wes McKinney <wesm+git@apache.org> Co-authored-by: emkornfield <emkornfield@gmail.com> Signed-off-by: Wes McKinney <wesm+git@apache.org>
To see if the CI issues go away.