MINOR: Fix lossy conversions flagged by Java 20#13582
Conversation
divijvaidya
left a comment
divijvaidya
left a comment
There was a problem hiding this comment.
Add a nit. Feel free to ignore.
Overall, looks good to me!
There was a problem hiding this comment.
Do we still need this cast to byte? Both COMPRESSION_CODEC_MASK and type.id are bytes.
There was a problem hiding this comment.
Unfortunately, the answer is yes: https://docs.oracle.com/javase/specs/jls/se8/html/jls-5.html#jls-5.6.2
There was a problem hiding this comment.
I expected type promotion to a common ancestor for cases where overflow is expected such as multiplication but didn't expect it for & bit operation. Nevertheless TIL!
There was a problem hiding this comment.
please add a comment that in the schema for RecordBatch, the type is represented by 2 bits. Hence, byte representation is sufficient.
ijuma
force-pushed
the
java-20-compilation-fixes
branch
from
97f4bee to
6be073f
Compare
|
@ijuma are we waiting to get additional pair of eyes on this one? I am comfortable with merging this PR in it's current form. |
|
@divijvaidya if you're comfortable, I think this is ready to be merged. |
|
Looks like there is a spotbugs issue, I'll investigate that. |
An example of the warning: > warning: [lossy-conversions] implicit cast from long to int in compound assignment is possibly lossy
ijuma
force-pushed
the
java-20-compilation-fixes
branch
from
21313c2 to
5e13b8e
Compare
| for (byte b : data) { | ||
| value <<= 8; | ||
| value |= b & 0xFF; | ||
| value |= (short) (b & 0xFF); |
There was a problem hiding this comment.
Fixed SpotBugs issue by changing (byte) to (short).
| // Starting JDK 12, this implementation could be replaced by InputStream#skipNBytes | ||
| while (bytesToSkip > 0) { | ||
| long ns = in.skip(bytesToSkip); | ||
| int ns = (int) in.skip(bytesToSkip); |
There was a problem hiding this comment.
New issue introduced in trunk after this PR was originally created. There are a few others, but I'll address them together with the JDK 20 CI build (which will prevent the issue altogether).
|
When you are creating the CI build @ijuma perhaps, consider creating a daily job instead on every PR? Current CI takes 3.5 hours and this new stage may increase it further. A daily run will help catch any regressions that we may have for JDK 20 until we want to officially support the next LTS release in Kakfa. |
|
Jobs run in parallel and hence don't add time. Daily jobs are basically ignored and are sort of useless. But we can discuss that in the relevant PR. |
|
After several test runs, it looks like the failures in the last run are flakes. JDK 8 build had a single failure and it passed locally when I ran it.
|
It's good for us to add support for Java 20 in preparation for Java 21 - the next LTS. Given that Scala 2.12 support has been deprecated, a Scala 2.12 variant is not included. Also remove some branch builds that add load to the CI, but have low value: JDK 8 & Scala 2.13 (JDK 8 support has been deprecated), JDK 11 & Scala 2.12 (Scala 2.12 support has been deprecated) and JDK 17 & Scala 2.12 (Scala 2.12 support has been deprecated). A newer version of Mockito (4.9.0 -> 4.11.0) is required for Java 20 support, but we only use it with Scala 2.13+ since it causes compilation errors with Scala 2.12. Similarly, we upgrade easymock when the Java version is 16 or newer as it's incompatible with powermock (which doesn't support Java 16 or newer). Filed KAFKA-15117 for a test that fails with Java 20 (SslTransportLayerTest.testValidEndpointIdentificationCN). Finally, fixed some lossy conversions that were added after #13582 was submitted. Reviewers: Ismael Juma <ismael@juma.me.uk>
2 participants
An example of the warning:
There should be no change in behavior as part of these changes - runtime logic ensured
we didn't run into issues due to the lossy conversions.
Committer Checklist (excluded from commit message)