GH-38051: [Java] Remove Java 8 support #43139
Conversation
laurentgo
requested review from
assignUser,
kou,
raulcd and
lidavidm
as code owners
|
|
github-actions
bot
added
Component: Java
Component: Documentation
awaiting review
|
It should be merged only after the 17.0.0 has been released. As for crossbow, I tried to do some local testing for it and it mostly works except maybe for some spark issues. |
java/flight/flight-core/pom.xml
Outdated
| <filters> | ||
| <filter> | ||
| <excludes> | ||
| <exclude>**/module-info.class</exclude> |
There was a problem hiding this comment.
Does this save us from "module hell"? aka publishing a package that has automatic module names auto-generated from filenames?
e.g.
[WARNING] * Required filename-based automodules detected: [flatbuffers-java-24.3.25.jar, jsr305-3.0.2.jar, protobuf-java-3.25.1.jar, protobuf-java-util-3.25.1.jar]. Please don't publish this project to a public artifact repository! *
There was a problem hiding this comment.
Not sure., maybe we can add Automatic-Module-Name attribute in the shaded jars to make the name not filename dependent.
What it helps with though is having shaded jars which exposes the wrong module information (like the jackson or gson ones which are copied over under META-INF/versions/9/module-info.class)
There was a problem hiding this comment.
FWIW I did request flatbuffers to add the Automatic-Module-Name attribute: google/flatbuffers#8348
There was a problem hiding this comment.
There's no built-in way to merge all module-info.class files into a single one, one need to analyze what needs to be exported and how to deal also with relocation.
Specifically for this module, there are 3 different shaded jars with different set of dependencies included and different set of relocations and I'm not sure which ones are supposed to be used internally and which ones are to be used externally. Knowing the intended usage may also guide us in deciding if those artifacts should be modularized or not
There was a problem hiding this comment.
I'm still willing to get rid of the shaded artifact here...It causes confusion and complicates the build
There was a problem hiding this comment.
Maybe I should open a separate issue/pull request? I tried to analyze how many shaded artifacts are generated and how many are used.
Here's the list of shaded artifacts:
- arrow-tools-{version}-jar-with-dependencies.jar
- flight-sql-jdbc-driver-{version}.jar
- flight-integration-tests-{version}-jar-with-dependencies.jar
- flight-core-{version}-shaded.jar
- flight-core-{version}-shaded-ext.jar
- flight-core-{version}-jar-with-dependencies.jar
- arrow-performance-{version}-benchmarks.jar
- arrow-vector-{version}-shade-format-flatbuffers
arrow-vector is used by other modules to not expose flatbuffers directly.
I can also see arrow-tools/flight-integration-tests used by some tests defined in dev/tasks.yml, and arrow-perfornance uber jar is common for people who wants to run jmh from commandline: those shaded jars are useful and I do not expect that we need modularization for those jars
The jdbc driver is shaded because people expect it to be self contained. It could benefit from having its own module-info.java which would only allow exposes the driver + public api, but all relocated classes would be not accessible.
Finally there are all the flight-core shaded jars and I haven't figured out if they are used or not
There was a problem hiding this comment.
I think a separate pull request for shaded artifacts is okay. We should also get rid of the warning I pasted initially before releasing v18. Someone on the original GH issue recommended using shading to prevent us from exposing those module names. Maybe that can be a separate issue, too.
github-actions
bot
added
awaiting committer review
|
@github-actions crossbow submit -g java |
|
Revision: 851a6c5 Submitted crossbow builds: ursacomputing/crossbow @ actions-29a2a39d61
|
danepitkin
changed the title
|
|
java/flight/flight-core/pom.xml
Outdated
| <filters> | ||
| <filter> | ||
| <excludes> | ||
| <exclude>**/module-info.class</exclude> |
There was a problem hiding this comment.
I'm still willing to get rid of the shaded artifact here...It causes confusion and complicates the build
github-actions
bot
added
awaiting changes
laurentgo
force-pushed
the
laurentgo/drop-java8
branch
from
851a6c5
to
4c64ea4
Compare
github-actions
bot
added
awaiting change review
|
@github-actions crossbow submit -g java |
|
Revision: 4c64ea4 Submitted crossbow builds: ursacomputing/crossbow @ actions-1b9da10481
|
|
Seems like most of the tests are now passing. The remaining issues seems to be linked to #43204 and apache/arrow-java#82 (didn't find a ticket for the go test issues but I did find several pull requests having the exact same failed tests) |
Already filed #43217 for 1) |
|
Thank you! I'll file the second one. Also, looks like there's a merge conflict. |
|
Issue for 2) apache/arrow-java#67 |
laurentgo
force-pushed
the
laurentgo/drop-java8
branch
2 times, most recently
from
d2f2d8a
to
4d96f14
Compare
|
I will merge this once Arrow v17 is officially released, which should happen in the next few days. |
|
LGTM. I feel like we should investigate if we can refactor some of the memory manager code to avoid using reflection/unsafe and use MethodHandles instead now that we're off JDK 8 but it probably should be separate from this work. |
I actually did some of the work to use |
laurentgo
force-pushed
the
laurentgo/drop-java8
branch
from
4d96f14
to
93f17aa
Compare
Remove build support for Java 8 and make Java 11 the minimum version to use to build Arrow in github actions and ci tasks
Channge minimum Java version to build Arrow to 11: * Change minimum java version and source/target/release compiler properties to 11 * Remove maven modules * Remove jdk11+ profiles and integrate their content into the main section * Let maven-compiler-plugin process `module-info.java` files and address several declaration issues * Exclude non modularized modules from javadoc aggregate tasks * Exclude module-info.class files from shaded jars as it is not representative of the whole content and may actually directly coming from a 3rd party dependency.
laurentgo
force-pushed
the
laurentgo/drop-java8
branch
from
93f17aa
to
5dd7042
Compare
|
@github-actions crossbow submit -g java |
|
Revision: 5dd7042 Submitted crossbow builds: ursacomputing/crossbow @ actions-91691f3234
|
|
@github-actions crossbow submit -g java |
|
The Dev / Source Release and Merge Script is failing, I thought it might have been caused by the remaining arrow maven plugin artifacts, but perhaps not. Edit: Probably because its building from cache - |
|
Revision: 5dd7042 Submitted crossbow builds: ursacomputing/crossbow @ actions-89524a3647
|
|
Thank you @laurentgo ! This PR was immensely helpful to the project. |
This is a common message I see all the time and I think it's a gradle develocity limitation. If the |
|
Whoops it wasn't a caching issue: #43313 |
|
After merging your PR, Conbench analyzed the 4 benchmarking runs that have been run so far on merge-commit 4161898. There were no benchmark performance regressions. 🎉 The full Conbench report has more details. It also includes information about 1 possible false positive for unstable benchmarks that are known to sometimes produce them. |
What changes are included in this PR?
mavenmodulesmodule-info.javafiles and address several declaration issuesAre these changes tested?
Through CI/CD.
Are there any user-facing changes?
Yes. Java 11 is now required to run any Arrow code
This PR includes breaking changes to public APIs.