Skip to content

Conversation

@ekazakas
Copy link
Contributor

What is the purpose of the change

There is an issue with Flink Kubernetes operator where JVM reports unrecognized options. These options are:

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

The above are only valid in JDK environments. When running in JRE environments, where most of the Flink applications will be running, the VM reports the following warnings:

WARNING: Unknown module: jdk.compiler specified to --add-exports
WARNING: Unknown module: jdk.compiler specified to --add-exports
WARNING: Unknown module: jdk.compiler specified to --add-exports
<...>

Brief change log

  • Removed JDK only JVM options from operator Helm chart

Verifying this change

This change is a trivial rework / code cleanup without any test coverage.

Does this pull request potentially affect one of the following parts:

  • Dependencies (does it add or upgrade a dependency): no
  • The public API, i.e., is any changes to the CustomResourceDescriptors: no
  • Core observer or reconciler logic that is regularly executed: no

Documentation

  • Does this pull request introduce a new feature? no
  • If yes, how is the feature documented? not applicable

@gyfora
Copy link
Contributor

gyfora commented Apr 29, 2025

These were added to improve java 17 compatibility for Flink apps, will this affect that? If so having the warnings doesn't seem like a big deal

@ekazakas
Copy link
Contributor Author

ekazakas commented Apr 29, 2025

These were added to improve java 17 compatibility for Flink apps, will this affect that? If so having the warnings doesn't seem like a big deal

It should not affect it, especially if the apps are running in JRE environment. When running in JRE, these options are not taken into account at all, they simply do not exist in those environments.

P.S. The operator image itself is running in JRE environment

@gyfora
Copy link
Contributor

gyfora commented Apr 29, 2025

These were added to improve java 17 compatibility for Flink apps, will this affect that? If so having the warnings doesn't seem like a big deal

It should not affect it, especially if the apps are running in JRE environment. When running in JRE, these options are not taken into account at all, they simply do not exist in those environments.

P.S. The operator image itself is running in JRE environment

This is about the Flink jobs, and the question is whether these params make Java 17 compatibility better in JDK env. If they do we should keep that and accept the warning as that doesn't hurt

@ekazakas
Copy link
Contributor Author

It doesn't make the compatibility better, since Flink jobs do not require compiler modules. In case of very specific applications that for some reason would need to access the javac compiler and it's modules, users could specify that in env.java.opts.all. But it's up to Flink maintainers to decide whether they would agree with this change. It would avoid the unnecessarry log pollution for 99% of Flink jobs

@gyfora
Copy link
Contributor

gyfora commented Apr 29, 2025

Makes sense @ekazakas thanks for the explanation :) Let's try this

# These parameters are required for Java 17 support.
# These should be kept in-sync with the flink dist env.java.opts.all defaults (for the given flink version) in: flink-dist/src/main/resources/config.yaml
# Flink 1.18 uses env.java.opts.all, if a user supplies their own version of these opts in their FlinkDeployment the options below will be overridden.
# env.java.default-opts.all is used for 1.19 onwards so users can supply their own opts.all in their Job deployments and have these appended.

Choose a reason for hiding this comment

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

I see in the comment above that the below content should be kept in sync with flink-dist/src/main/resources/config.yaml.

I suggest that we amend the comment - as it will no longer be true.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have removed this comment

@ferenc-csaky
Copy link
Contributor

@gyfora we good to merge this?

@gyfora
Copy link
Contributor

gyfora commented May 15, 2025

Thanks for the reminder @ferenc-csaky , merging

@gyfora gyfora merged commit b7c429a into apache:main May 15, 2025
130 checks passed
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.

5 participants