MINOR: Add JDK 20 CI build and remove some branch builds by ijuma · Pull Request #12948 · apache/kafka

ijuma · 2022-12-03T22:07:07Z

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.

Committer Checklist (excluded from commit message)

Verify design and implementation
Verify test coverage and CI build status
Verify documentation (including upgrade notes)

ijuma · 2022-12-04T01:41:37Z

#12675 needs to be merged for this to work.

ijuma · 2022-12-05T05:05:15Z

Rebased to include #12675.

ijuma · 2022-12-05T18:26:44Z

Interesting failure with Java 19:

09:03:40 Gradle Test Run :core:integrationTest > Gradle Test Executor 166 > DynamicBrokerReconfigurationTest > testTrustStoreAlter(String) > kafka.server.DynamicBrokerReconfigurationTest.testTrustStoreAlter(String)[2] FAILED
09:03:40 org.apache.kafka.common.KafkaException: Failed to close kafka consumer
09:03:40 at app//org.apache.kafka.clients.consumer.KafkaConsumer.close(KafkaConsumer.java:2430)
09:03:40 at app//org.apache.kafka.clients.consumer.KafkaConsumer.close(KafkaConsumer.java:2378)
09:03:40 at app//kafka.server.DynamicBrokerReconfigurationTest.$anonfun$tearDown$6(DynamicBrokerReconfigurationTest.scala:172)
09:03:40 at app//kafka.server.DynamicBrokerReconfigurationTest.$anonfun$tearDown$6$adapted(DynamicBrokerReconfigurationTest.scala:172)
09:03:40 at app//scala.collection.IterableOnceOps.foreach(IterableOnce.scala:575)
09:03:40 at app//scala.collection.IterableOnceOps.foreach$(IterableOnce.scala:573)
09:03:40 at app//scala.collection.AbstractIterable.foreach(Iterable.scala:933)
09:03:40 at app//kafka.server.DynamicBrokerReconfigurationTest.tearDown(DynamicBrokerReconfigurationTest.scala:172)
09:03:40
09:03:40 Caused by:
09:03:40 org.apache.kafka.common.errors.SslAuthenticationException: Failed to process post-handshake messages
09:03:40
09:03:40 Caused by:
09:03:40 javax.net.ssl.SSLHandshakeException: Received fatal alert: certificate_unknown

Needs investigation.

ijuma · 2023-06-21T14:00:34Z

@divijvaidya Looks like one of your changes introduced this issue where we are adding a long to an int in a couple of places in this file. Can you please take a look and suggest what's the right fix (I just did the simplest thing to make the compiler not complain - it's not safe as it stands)?

This is the main blocker remaining before this PR can be non draft.

@ijuma casting to int here is safe. Can you please add the following comment here:

// cast to int is fine here since the value of bytesSkipped is upper bound by value of avail // which is an int

It is safe because as per the lines above it long bytesSkipped = (avail < remaining) ? avail : remaining;, bytesSkipped is minimum of avail or remaining which means it's max value is available which we know is an int.

Same for the other place.

Hmm, in that case we can perhaps change bytesSkipped to be an int?

I am afraid that won't be possible.

This is because there are two branches where bytesSkipped is updated. In one branch when we call bytesSkipped = getInIfOpen().skip(remaining), bytesSkipped holds a long value since getInIfOpen().skip(remaining) returns a long. In the other branch, when we call bytesSkipped = (avail < remaining) ? avail : remaining, bytesSkipped holds an int since this min statement bounds it to max of avail, which is an int.

I can attempt to do a bunch of refactoring if that will make things clearer but that shouldn't block this PR.

I tweaked this a bit to make it more obvious why it's safe to cast. Please take a look.

It's a good idea for us to track the latest JDK version supported by Gradle.

…Java 20

ijuma · 2023-06-23T16:11:16Z

@divijvaidya Do you have cycles to review this PR? It is ready for review.

ijuma · 2023-06-23T16:14:12Z

      new ListenerName(""), SecurityProtocol.PLAINTEXT, mock(classOf[ClientInformation]), false)
    val request = new RequestChannel.Request(0, context, time.nanoseconds(),
-      mock(classOf[MemoryPool]), mock(classOf[ByteBuffer]), metrics)
+      mock(classOf[MemoryPool]), ByteBuffer.allocate(0), metrics)


It's not possible to mock this class with Java 20, but there's no reason to mock it.

divijvaidya · 2023-06-26T11:55:37Z

        // Skip bytes stored in intermediate buffer first
        int avail = count - pos;
-        long bytesSkipped = (avail < remaining) ? avail : remaining;
+        int bytesSkipped = Math.min(avail, (int) remaining);


I am not sure that this is a good idea to downcast and then perform comparison. Comparison amongst different types is performed by upgrading them to the higher type and then performing comparison. In current approach, we downcasting remaining to int is unsafe.

May I suggest an alternative where we upgrade avail to long before performing comparison and then converting result to int: int bytesSkipped = (int) Math.min((long) avail, remaining);

Can you please share an example where these two variants would have a different result given that avail is a positive int?

I am concerned about cases where remaining is outside the range of representable int values, let's say remaining = Integer.MIN_VALUE + 1000. When down casting in cases of overflow (i.e. in Math.min(avail, (int) remaining)), the behaviour is implementation defined and numeric value for (int)remaining may end up leading to a negative value numeric int. Calculating min for this negative value with avail will lead to bytesSkipped as negative.

In the implementation I suggested, the result of Math.min((long) avail, remaining) is guaranteed to fit in int because it's upper bound by avail.

behaviour is implementation defined

Can you share a reference for this? My understanding is that the Java specifies the behavior in these cases. Perhaps you're thinking of a C or something like that?

let's say remaining = Integer.MIN_VALUE + 1000

I think you meant to say that if we have a long of value (Integer.MAX_VALUE + 1000) may result in sign extension during truncation resulting in a negative int. That's a fair point - my bad. I'll fix this. It does show a gap in the tests for this class, we should ideally have tests that cover these boundary cases.

It does show a gap in the tests for this class, we should ideally have tests that cover these boundary cases.

Fair point. Added a JIRA https://issues.apache.org/jira/browse/KAFKA-15123

divijvaidya · 2023-06-26T12:08:28Z

-        // are pushed to trunk and/or release branches. We achieve this via
-        // the `when` clause.
-
-        stage('JDK 8 and Scala 2.13') {


Adding this note here for future visitors to this PR (and to ensure that we are on the same page here about the reason to remove these stages from CI)

Removing this combination from tests in fine because scala 2.13 is fully compatible with JDK 8 [1]. If we can guarantee that code is compiling correctly with 2.13 and JDK 8 separately, then that is sufficient and we do not necessarily have to test a unique combination of both.

[1] https://docs.scala-lang.org/overviews/jdk-compatibility/overview.html

That statement isn't strictly true since there could be bugs in the implementation - that's why we had these tests in the first place. But the truth is that they're low value since (1) the probability of a bug affecting JDK 8 and Scala 2.13 but not JDK 8 and Scala 2.12 is low (2) having too many variants makes it even less likely for people to pay attention to the flaky failures that exist (3) it's expensive to run so many variants.

divijvaidya

The changes look good to me. Feel free to merge after the CI passes or has unrelated flaky tests.

ijuma · 2023-06-30T08:10:27Z

Build looks good - the failures in the last build are all flakes.

ijuma · 2023-06-30T08:12:27Z

I pasted the wrong name on the reviewers field - sorry @divijvaidya.

divijvaidya · 2023-06-30T08:27:05Z

I pasted the wrong name on the reviewers field - sorry @divijvaidya.

No worries! I am glad to see this merged in.

ijuma force-pushed the java-19-build branch from f23292f to 524f8c2 Compare December 5, 2022 05:04

ijuma changed the title ~~MINOR: Add JDK 19 & Scala 2.13 CI build~~ MINOR: Add JDK 20 CI build Dec 24, 2022

ijuma force-pushed the java-19-build branch from c1de696 to 0f535ac Compare April 14, 2023 14:38

divijvaidya added the dependencies Pull requests that update a dependency file label Jun 19, 2023

ijuma force-pushed the java-19-build branch from 444009a to f255337 Compare June 21, 2023 13:59

ijuma commented Jun 21, 2023

View reviewed changes

ijuma marked this pull request as draft June 21, 2023 14:01

ijuma changed the title ~~MINOR: Add JDK 20 CI build~~ MINOR: Add JDK 20 CI build and remove some branch builds Jun 21, 2023

ijuma force-pushed the java-19-build branch 2 times, most recently from 4804537 to ad2b5d0 Compare June 22, 2023 18:57

ijuma added 10 commits June 23, 2023 09:05

MINOR: Add JDK 19 & Scala 2.13 CI build

cbb6b97

It's a good idea for us to track the latest JDK version supported by Gradle.

Suppress deprecation warning affecting --source/--target 8

5a6f9b9

Fix lossy conversion warnings.

f9984db

Possibly incorrect lossy conversion warning fix

321c6ff

Upgrade mockito to 4.11 with Scala 2.13 for Java 20 support

c7e1019

Clean up ChunkedBytesStream fixes

4d2e222

Mockito cannot mock ByteBuffer in Java 20

66f916a

Upgrade easymock to 5.1.0

e6a3951

Fix stray println

8070dfd

Disable SslTransportLayerTest.testValidEndpointIdentificationCN with …

a7e1e94

…Java 20

ijuma force-pushed the java-19-build branch from 4ccdbb4 to a7e1e94 Compare June 23, 2023 16:05

ijuma marked this pull request as ready for review June 23, 2023 16:10

ijuma commented Jun 23, 2023

View reviewed changes

divijvaidya reviewed Jun 26, 2023

View reviewed changes

Fix unsafe cast

3c974a8

divijvaidya approved these changes Jun 28, 2023

View reviewed changes

Only use easymock 5.1 with Java 16 or newer

345cc38

ijuma merged commit 1f4cbc5 into apache:trunk Jun 30, 2023

ijuma deleted the java-19-build branch June 30, 2023 08:12

Conversation

ijuma commented Dec 3, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Committer Checklist (excluded from commit message)

Uh oh!

ijuma commented Dec 4, 2022

Uh oh!

ijuma commented Dec 5, 2022

Uh oh!

ijuma commented Dec 5, 2022

Uh oh!

ijuma Jun 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ijuma commented Jun 23, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ijuma Jun 26, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

divijvaidya left a comment

Choose a reason for hiding this comment

Uh oh!

ijuma commented Jun 30, 2023

Uh oh!

ijuma commented Jun 30, 2023

Uh oh!

divijvaidya commented Jun 30, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

ijuma commented Dec 3, 2022 •

edited

Loading

ijuma Jun 21, 2023 •

edited

Loading

ijuma Jun 26, 2023 •

edited

Loading