Skip to content
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

Switch from Java 11 LTS to Java 21 LTS #760

Merged
merged 20 commits into from
Oct 27, 2023
Merged

Switch from Java 11 LTS to Java 21 LTS #760

merged 20 commits into from
Oct 27, 2023

Conversation

abyrd
Copy link
Member

@abyrd abyrd commented Nov 16, 2021

Update

The branch is still called jdk17 for continuity, but the title has been updated to reference the most recent JDK release. Corresponding issue is #852.

Original Description

accept java 17 source code and produce java 17 bytecode.
this will require running on machines with JDK17 installed.
use gradle cache functionality built into setup-java Github action.

accept java 17 source code and produce java 17 bytecode.
this will require running on machines with JDK17 installed.
use gradle cache functionality built into setup-java Github action.
@abyrd
Copy link
Member Author

abyrd commented Nov 16, 2021

Seven tests fail, all apparently due to java.lang.reflect.InaccessibleObjectException when we try to set a private field public.

Related

I think we'd have to either write hundreds or thousands of handcrafted serializers, or do some complicated Java configuration to "open" a bunch of packages to Kryo for reflection. I suspect this is going to make migration to Java 17 impractical for the time being.

I have seen the broken KryoNetworkSerializerTest pass by adding JVM command line options --add-opens=java.base/java.io=ALL-UNNAMED --add-opens=java.base/java.time=ALL-UNNAMED --add-opens=java.base/java.time.zone=ALL-UNNAMED --add-opens=java.base/java.lang=ALL-UNNAMED. Similar opens directives can be given in a module-info.java but those can only open our own classes - they can't open java.base internals to Kryo for example. That can only be done with command line switches. Last time I checked modularization was looking like a giant mess until every one of our dependencies was also modularized (we might be getting close, a few years have passed) and every dependency has to be listed in both gradle build config (to pull in the dependency files) and in module-info.java (to tell the compiler/runtime which modules can see one another). It's pretty ugly, but it's the future.

@abyrd
Copy link
Member Author

abyrd commented Nov 16, 2021

With the tests passing, we now run into problems with shadowJar. The best solution here is probably to simply stop using shadow jars and switch to ZIP distributions containing one jar per dependency. These shadow jars are only used in automated testing, not in our backend and worker instance startup scripts (which now use ZIP distributions).

@abyrd abyrd added p2 Priority level 2: medium priority, ideally next release t1 Time level 1: think days labels Nov 16, 2021
@abyrd abyrd mentioned this pull request Feb 25, 2022
@abyrd abyrd changed the title Switch from Java 11 LTS to Java 17 LTS Switch from Java 11 LTS to Java 19 LTS Jan 13, 2023
@abyrd abyrd changed the title Switch from Java 11 LTS to Java 19 LTS Switch from Java 11 LTS to Java 19 (prep for Java 21 LTS) Jan 13, 2023
shadow jars defy intended Java design and are essentially obsolete
instead set up Gradle to run application with the JARs on the classpath
will require changing end-to-end/UI testing to use 'gradle runBackend'
@abyrd abyrd force-pushed the jdk17 branch 2 times, most recently from 6df6e6a to 00ef573 Compare January 13, 2023 15:27
Update Github actions versions. Use Java 19.
Set max heap to 7G using dedicated parameter.
Use UI commit where 'yarn start-backend' performs 'gradle runBackend'.
abyrd and others added 2 commits January 17, 2023 18:25
match dependencies to kryo-tools 1.5
network file version bumped to nv3
update r5 automatic module name
eliminate use of Integer constructor in response to deprecation warning
@abyrd
Copy link
Member Author

abyrd commented Jan 18, 2023

Via a new release of kryo-tools (https://github.com/conveyal/kryo-tools/releases/tag/v1.5.0) this branch is now using the newest Kryo 5.4.0. Recently released at the end of December 2022, this cooperates much better with the Java module system that is now strictly enforced in more recent JVMs. It includes new serializers for internal Java classes so can avoid performing any reflection, so we no longer need any JVM command line switches to open JDK internal modules for reflection.

This led me back down the rabbit hole of Java modules - at first the idea of adding our own module-info was somewhat entangled with that of opening modules for reflection and eliminating the JVM switches. However these are two rather separate things. My conclusion is that it's best to stick with the same approach we've used since the module system appeared: declare an automatic-module-name for r5, but don't add a module-info.java which would change behavior with respect to dependency JARs that don't declare their own automatic-module-name or module-info.java. A large number of our dependencies are in this category, and we'd end up maintaining three declarations for every dependency, all of which need to be kept in sync: the dependency declaration for the Gradle build, Gradle plugin config to inject a module name into each of these dependencies, and the entries in our r5 module-info to say r5 requires each of them, plus all the other dependencies that do declare a module name. It's a lot to maintain for no particular benefit, other than pleasing our build tools and avoiding the occasional cryptic JVM command line flag. It's just that unfortunately Gradle refuses to expose "legacy" dependencies that don't declare module names inside projects that do, despite the fact that many (the majority?) of libraries have never adopted this module system.

It seems like there are two differing schools of thought on this - some think that Java modules are now official policy and it's virtuous to push people to adopt them. Others, including some developers of the module system (from whom I picked up on this alternate position) consider the module system something primarily intended for internal JRE usage, to permit making smaller JREs for embedded devices and distributing desktop apps, and for improving security within the JRE itself. This system could have been kept entirely internal to the JRE, indeed some of its developers wanted to, but was made public in case anyone else would get any use out of it, and they say that it's perfectly acceptable if people don't want to use it and just dump all their packages into automatic modules or the unnamed module.

So considering that I now have ways to get Kryo, our reflection-based ObjectDiffer, and incubator modules working on Java 19, I think we should continue with the automatic module approach.

@abyrd
Copy link
Member Author

abyrd commented Jan 18, 2023

Kryo 5 produces binary output that is incompatible with older versions of Kryo, so the Network version has been bumped to nv3. I added some lines in the Javadoc to track the successive network versions and when they were changed.

This PR is pretty much complete. One remaining detail is that it produces only ZIP distributions and not shaded JARs. Not using shaded JARs is a desirable move overall, but requires some changes any place we kick off integration tests to grab the backend JARs via those ZIPs, or via a shallow checkout and a Gradle build without tests. I'll prepare a PR to make those changes before marking this PR ready for review.

  • Use gradle startBackend instead of shadow JAR in testing
  • Update any AMI generation scripts to install Java 19 instead of 11

@abyrd abyrd marked this pull request as ready for review January 19, 2023 11:03
@abyrd
Copy link
Member Author

abyrd commented Aug 4, 2023

Next steps after JDK21 release (September 19, 2023):

  • release new conveyal/kryo-tools with latest version of Kryo
  • update r5 to use this new kryo-tools
  • update gradle script to consume Java 21 source and produce Java 21 bytecode
  • update testing to run Java 21
  • update AMI generation scripts to install Java 21

@abyrd
Copy link
Member Author

abyrd commented Aug 17, 2023

This is now updated to use Java 20, anticipating an R5 release before Java 21 is released and incorporated into our CI/deployment environment. Build is successful / all checks are passing and it's ready for review, but we should probably wait to merge it together with the "regular polling" PR, right before we begin testing, so we can more easily back those changes out in the event of unexpected incompatibilities.

@abyrd abyrd changed the title Switch from Java 11 LTS to Java 19 (prep for Java 21 LTS) Switch from Java 11 LTS to Java 20 (prep for Java 21 LTS) Aug 17, 2023
Align transitive dependency versions.
Kryo 5.5 is tested with Java 20.
New major version of Guava has security fixes but no breaking changes.
also updated copyright line
@abyrd abyrd changed the title Switch from Java 11 LTS to Java 20 (prep for Java 21 LTS) Switch from Java 11 LTS to Java 21 LTS Oct 19, 2023
@abyrd
Copy link
Member Author

abyrd commented Oct 19, 2023

Updated to use Java 21 LTS.

Corresponding UI changes: https://github.com/conveyal/ui/pull/1941
Corresponsing cluster changes: https://github.com/conveyal/cluster/pull/109

Copy link
Member

@trevorgerhardt trevorgerhardt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ran well locally. Tests are passing. Corresponding PRs in cluster and ui are ready. Lets merge 👍

@abyrd abyrd merged commit 25dfd1c into dev Oct 27, 2023
3 checks passed
@abyrd abyrd deleted the jdk17 branch October 27, 2023 12:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
p2 Priority level 2: medium priority, ideally next release t1 Time level 1: think days
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants