-
Notifications
You must be signed in to change notification settings - Fork 514
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
Update default Dataflow add-open opts for Java 17+ #5537
Conversation
|
||
sys.props("java.version") match { | ||
case javaVersionRe(version) | ||
if version.toInt >= 17 && dataflowPipelineOpts.getJdkAddOpenModules == null => |
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.
I suppose this is also a bugfix PR for Java 21 users 😬
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #5537 +/- ##
==========================================
+ Coverage 61.43% 61.44% +0.01%
==========================================
Files 312 312
Lines 11106 11105 -1
Branches 750 776 +26
==========================================
+ Hits 6823 6824 +1
+ Misses 4283 4281 -2 ☔ View full report in Codecov by Sentry. |
We should add it there too for the tests |
dataflowPipelineOpts.setJdkAddOpenModules( | ||
List("java.base/java.util=ALL-UNNAMED", "java.base/java.lang.invoke=ALL-UNNAMED").asJava | ||
) | ||
val javaVersionRe = "^(\\d+)\\..*".r |
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.
This regex would give 1
for java 8 (version is set as 1.8....
).
Let's use the same def as for the build
System.getProperty("java.version").stripPrefix("1.").takeWhile(_.isDigit).toInt
87fd804
to
2b14afd
Compare
Match all the add-opens required by Kryo
tested in Dataflow