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

Improved Java 17 support and Java runtime docs. #12839

Merged
merged 13 commits into from
Aug 4, 2022

Conversation

gianm
Copy link
Contributor

@gianm gianm commented Jul 30, 2022

Some work towards #12838.

  1. Add a "Java runtime" doc page with information about supported
    Java versions, garbage collection, and strong encapsulation.

  2. Update asm and equalsverifier to versions that support Java 17.

  3. Add additional "--add-opens" lines to surefire configuration, so
    tests can pass successfully under Java 17.

  4. Switch openjdk15 tests to openjdk17.

  5. Update FrameFile to specifically mention Java runtime incompatibility
    as the cause of not being able to use Memory.map.

  6. Update SegmentLoadDropHandler to log an error for Errors too, not
    just Exceptions. This is important because an IllegalAccessError is
    encountered when the correct "--add-opens" line is not provided,
    which would otherwise be silently ignored.

  7. Update example configs to use druid.indexer.runner.javaOptsArray
    instead of druid.indexer.runner.javaOpts. (The latter is deprecated.)

  8. Added a run-java command that sets up the proper "--add-opens"
    and "--add-exports" parameters for Java 17, and update out-of-box
    configs to include druid.indexer.runner.javaCommand=bin/run-java
    so this gets used.

1) Add a "Java runtime" doc page with information about supported
   Java versions, garbage collection, and strong encapsulation..

2) Update asm and equalsverifier to versions that support Java 17.

3) Add additional "--add-opens" lines to surefire configuration, so
   tests can pass successfully under Java 17.

4) Switch openjdk15 tests to openjdk17.

5) Update FrameFile to specifically mention Java runtime incompatibility
   as the cause of not being able to use Memory.map.

6) Update SegmentLoadDropHandler to log an error for Errors too, not
   just Exceptions. This is important because an IllegalAccessError is
   encountered when the correct "--add-opens" line is not provided,
   which would otherwise be silently ignored.

7) Update example configs to use druid.indexer.runner.javaOptsArray
   instead of druid.indexer.runner.javaOpts. (The latter is deprecated.)
@FrankChen021
Copy link
Member

I used to submitted a PR #12333 to reflect our support of JDK 11.
If JDK 17 is supported now, I think we also need to update the doc updated in that PR.

@gianm
Copy link
Contributor Author

gianm commented Jul 31, 2022

Ah, thanks for pointing that out. It's not quite supported yet, this PR is just a step. #12838 describes some more items we will need. We'll at least need to update DataSketches Memory.

@gianm
Copy link
Contributor Author

gianm commented Jul 31, 2022

When building with openjdk17 In CI, I ran into a problem where pull-deps was run without the necessary exports and opens. So in the latest commit I added a run-java command that sets this stuff up, and am using it in three places:

  1. At build time when pull-deps is run
  2. At run time for run-druid and run-zk
  3. In the default server configs, for druid.indexer.runner.javaCommand, so it gets used for tasks too.

Copy link
Contributor

@paul-rogers paul-rogers left a comment

Choose a reason for hiding this comment

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

The changes appear to be the result of actually trying Java 17 out with Druid and, as such, the changes are likely valid given that the result works. Reviewed the docs and other bits, all of which look great. LGTM (non-binding)

compatible with Java 17.

There are many free and actively-supported Java runtime environments available, such as
[Corretto](https://aws.amazon.com/corretto/) and [Zulu](https://www.azul.com/downloads/?package=jdk#download-openjdk).
Copy link
Member

Choose a reason for hiding this comment

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

should we also mention Adoptium Temurin https://adoptium.net/temurin/releases/ here as one of the non-vendor-specific alternatives?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea. I wasn't aware of that one. Added it.

Copy link
Member

@FrankChen021 FrankChen021 left a comment

Choose a reason for hiding this comment

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

Wow, all CI have passed. I have taken a loot at most of changes and it LGTM.

One thing I'm not sure is if pull-deps works under JRE 17.

@gianm
Copy link
Contributor Author

gianm commented Aug 2, 2022

One thing I'm not sure is if pull-deps works under JRE 17.

I did notice that it is run as part of the openjdk17 CI, because there was a problem with it in an earlier version of this patch. So it works at least well enough for that usage (if the strong-encapsulation-related command line parameters are included). I didn't try further usages.

examples/bin/run-java Outdated Show resolved Hide resolved
Co-authored-by: Xavier Léauté <xl+github@xvrl.net>
@gianm gianm merged commit ef6811e into apache:master Aug 4, 2022
@gianm gianm deleted the towards-java17 branch August 4, 2022 06:16
@gianm gianm mentioned this pull request Aug 10, 2022
@abhishekagarwal87 abhishekagarwal87 added this to the 24.0.0 milestone Aug 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants