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

[Java17] Add --add-export settings to restore JDK17 compatibility #13825

Merged
merged 5 commits into from
Mar 24, 2022

Conversation

robbavey
Copy link
Member

@robbavey robbavey commented Mar 1, 2022

After #13700 updated google-java-format dependency, it is now required to add a number of --add-export flags in order to run on JDK17. This commit adds these flags to the jvm options for a running logstash, and to the tests running on gradle to enable tests to still work

Release notes

Note: If you use custom jvm.options, you will need to add the following settings to config.jvm.options to allow Logstash to start:

11-:--add-exports=jdk.compiler/com.sun.tools.javac.api=ALL-UNNAMED
11-:--add-exports=jdk.compiler/com.sun.tools.javac.file=ALL-UNNAMED
11-:--add-exports=jdk.compiler/com.sun.tools.javac.parser=ALL-UNNAMED
11-:--add-exports=jdk.compiler/com.sun.tools.javac.tree=ALL-UNNAMED
11-:--add-exports=jdk.compiler/com.sun.tools.javac.util=ALL-UNNAMED

Why is it important/What is the impact to the user?

Without these additional settings, logstash will not start, and tests will fail.

Checklist

  • My code follows the style guidelines of this project
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have made corresponding change to the default configuration files (and/or docker env variables)
  • I have added tests that prove my fix is effective or that my feature works

Author's Checklist

How to test this PR locally

  • Ensure that JDK17 is installed and set using LS_JAVA_HOME
  • Run bin/logstash with a simple pipeline including at least one filter
  • Also run unit and integration tests, ensuring that JDK17 is being used

Related issues

Closes #13819
Relates #13700
Relates logstash-plugins/logstash-filter-kv#98

@yaauie
Copy link
Member

yaauie commented Mar 1, 2022

  • ALL-UNNAMED is perhaps overly broad -- Is the generated code that is triggering this maybe too anonymous to allow a more refined export?
  • since these exports are required functionality to make Logstash work, should it live in a separate source file that is always added to the JVM options, instead of requiring upgrading users to change their inherited config?

@robbavey
Copy link
Member Author

robbavey commented Mar 1, 2022

@yaauie

ALL-UNNAMED is perhaps overly broad -- Is the generated code that is triggering this maybe too anonymous to allow a more refined export?

I don't believe we can - the error message we get is

java.lang.IllegalAccessError: class com.google.googlejavaformat.java.JavaInput (in unnamed module @0x42f93a98) cannot access class com.sun.tools.javac.parser.Tokens$TokenKind (in module jdk.compiler) because module jdk.compiler does not export com.sun.tools.javac.parser to unnamed module @0x42f93a98

referring to an unnamed module. The extra additions made here are directly from the release notes for the library that causes this issue.

since these exports are required functionality to make Logstash work, should it live in a separate source file that is always added to the JVM options, instead of requiring upgrading users to change their inherited config?

That's probably not a bad idea. Once I'm confident this fixes the issue, I'll take a look at maybe adding this to the JvmOptionsParser? (and maybe include regex.interruptible as well...)

@yaauie
Copy link
Member

yaauie commented Mar 1, 2022

It looks like in order to be more refined than ALL-UNNAMED, the googlejavaformat package needs to provide a module manifest (google/google-java-format#755).

@robbavey
Copy link
Member Author

robbavey commented Mar 4, 2022

Jenkins test this please

After #13700 updated google-java-format dependency, it is now required to add a number
of `--add-export` flags in order to run on JDK17. This commit adds these flags to the
jvm options for a running logstash, and to the tests running on gradle to enable tests
to still work
…rser

Certain values for the JVM are mandatory for Logstash to function correctly. Rather than
leave them in config/jvm.options where they can be updated, and can cause upgrade issues
for users when we add new mandatory options, move them into code where they cannot be
changed
@robbavey
Copy link
Member Author

@yaauie Apologies for the delay, ready for another round of reviews

Copy link
Member

@yaauie yaauie left a comment

Choose a reason for hiding this comment

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

LGTM 💫

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

JDK17 support is broken since update to google java format dependency
3 participants