-
Notifications
You must be signed in to change notification settings - Fork 13.2k
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
[FLINK-25002][build] Add java 17 add-opens/add-exports JVM arguments #22794
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally, it looks fine. I think it's hard to review that one without the build failures. In this sense, I trust you that you did proper checking here. I just added a few comments to verify to make sure that I understand it correctly.
oh boy CI is broken pretty badly. I think this is the first time I'm running all this stuff on Java 8 👀 |
Fixed. Bit odd that surefire just hangs if the JVM crashes immediately 🤔 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I did a few test runs (but didn't check all the configuration changes). The change looks reasonable from my side. 👍
The CI failure seems to be unrelated to the change. Multiple Yarn tests are failing in the end. It looks like the first test failure didn't clean the Yarn cluster properly (I created FLINK-32366 to cover it).
hmm the yarn tests seems to reliably get stuck now; investigating... |
Yarn failure should be fixed; we were missing an |
yikes, good that you checked 👍 I'm gonna close FLINK-32366 again |
This PR sets of all required add-opens/add-exports statements for Maven, surefire, test-internal JVMs and flink-dist.
If a module requires such a statement for surefire/testing then it must define a list of statements in the
surefire.module.config
property of the module's pom. This property is set on the test JVMs via surefire, but is also exposed as a system property to the test itself such that utils like theTestJvmProcess
can forward them.I documented a hint for each statement as to why it is required.
flink-dist is configured separately via
flink-conf.yaml
.The required statements were purely determined by running tests and checking what fails.
"Completeness" was validated on my personal CI; we may not be covering all tests since some only run on the apache branches. We'll just amend missing ones in the future if necessary.