KAFKA-14552: Assume a baseline of 3.0 for server protocol versions #18497

ijuma · 2025-01-12T21:13:23Z

Kafka 4.0 will remove support for zk mode and will require conversion to kraft before upgrading to 4.0. The minimum kraft version is 3.0 (aka 3.0-IV1).

This provides an opportunity to remove exclusively server side protocols versions that only exist to allow direct upgrades from versions older than 3.0 or that are used only by zk mode.

Since KRaft became production ready in 3.3, we should consider setting the baseline to 3.3. But that requires more discussion and it can be done via a separate change (KAFKA-18601).

Protocol changes:

Remove RequestHeader v0 (only used by ControlledShutdown v0)
Remove WriteTxnMarkers v0
Remove all versions of ControlledShutdown, LeaderAndIsr, StopReplica, UpdateMetadata

In order to remove all versions safely, extend generator to support setting "versions" to "none".
In this case, we no longer generate the *Data classes, but we still reserve the id for
the relevant protocol api (so it doesn't get accidentally used for something else). The protocol
documentation is correct after these changes.

We kept a simplified version of LeaderAndIsr{Request|Response} because it's used by many
tests that are still relevant in kraft mode. Once KAFKA-18486 is done, it may be possible to remove
it (I left a comment on the ticket). Similarly, KAFKA-18487 may make it possible to remove
the introduced StopReplicaPartitionState (left a comment on that ticket too).

There are a number of places that were adjusted to include an ApiKeys.hasValidVersion
check.

Committer Checklist (excluded from commit message)

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

ijuma · 2025-01-19T23:13:02Z

core/src/test/scala/unit/kafka/server/ReplicaManagerTest.scala

-  }
-
-  @Test
-  def testUpdateStrayLogs(): Unit = {


ReplicaManager.updateStrayLogs was deleted and hence this method is no longer needed. @cmccabe @jsancio Can you please confirm that we have tests for the kraft flow of stray log management?

ijuma · 2025-01-19T23:18:16Z

core/src/test/scala/unit/kafka/server/ReplicaManagerTest.scala

@@ -4732,67 +4549,6 @@ class ReplicaManagerTest {
    }
  }

-  @Test
-  def testPartitionMetadataFileCreatedAfterPreviousRequestWithoutIds(): Unit = {


@jolshan I believe this andtestPartitionFetchStateUpdatesWithTopicIdChanges are no longer necessary since KRaft always requires topic ids to be set and 4.0 only supports upgrades from clusters already running in kraft mode. Do you agree or are there reasons to try to make these tests work?

I think your conclusion makes sense.

ijuma · 2025-01-19T23:31:28Z

generator/src/main/java/org/apache/kafka/message/ApiMessageTypeGenerator.java

@@ -355,28 +362,9 @@ private void generateHeaderVersion(String type) {
                buffer.decrementIndent();
                continue;
            }
-            if (type.equals("request") && apiKey == 7) {
-                buffer.printf("// Version 0 of ControlledShutdownRequest has a non-standard request header%n");


We don't need the special case for ControlledShutdown v0 anymore (since it's been removed).

ijuma · 2025-01-19T23:40:24Z

core/src/test/scala/unit/kafka/server/ShareGroupHeartbeatRequestTest.scala

@@ -894,7 +894,7 @@ class ShareGroupHeartbeatRequestTest(cluster: ClusterInstance) {
      val allPartitionsMetadata = waitForAllPartitionsMetadata(brokersToValidate, topic, totalPartitionCount)
      (0 until totalPartitionCount - 1).foreach(i => {
        allPartitionsMetadata.get(new TopicPartition(topic, i)).foreach { partitionMetadata =>
-          assertEquals(totalPartitionCount, partitionMetadata.replicas.size)
+          assertEquals(totalPartitionCount, partitionMetadata.isr.size)


This is not exactly the same behavior, but the updated behavior is fine for the test (since isr and replicas is the same). The reason for the change is that the replicas are no longer returned by the method being used here.

ijuma · 2025-01-20T00:22:27Z

clients/src/test/java/org/apache/kafka/common/protocol/ProtocolTest.java

+public class ProtocolTest {
+
+    @Test
+    public void testToHtml() {


Surprisingly, toHtml had zero tests even though it breaks the build if it throws an exception (as it's called from one of the docs build targets).

ijuma · 2025-01-20T00:24:22Z

clients/src/main/resources/common/message/RequestHeader.json

@@ -36,7 +38,7 @@
    // ApiVersionsRequest, even if it is from a newer version.
    // Since the client is sending the ApiVersionsRequest in order to discover what
    // versions are supported, the client does not know the best version to use.
-    { "name": "ClientId", "type": "string", "versions": "1+", "nullableVersions": "1+", "ignorable": true,


Removed ignorable since the ClientId is always set now.

ijuma · 2025-01-20T02:42:52Z

core/src/main/scala/kafka/server/ReplicaManager.scala

@@ -419,42 +418,6 @@ class ReplicaManager(val config: KafkaConfig,
      brokerTopicStats.removeMetrics(topic)
  }

-  private[server] def updateStrayLogs(strayPartitions: Iterable[TopicPartition]): Unit = {


Only used by tests.

…nses and nearly remove LeaderAndIsr Kept a minimal version of LeaderAndIsr as it's used in a few tests - will remove that separately.

…tadata support

… are no longer applicable

…rted version) and add test

ijuma · 2025-01-20T06:40:30Z

core/src/test/scala/unit/kafka/server/ReplicaManagerTest.scala

      assertTrue(replicaManager.localLog(topicPartition).isDefined)
      val log2 = replicaManager.localLog(topicPartition).get
      assertFalse(log2.partitionMetadataFile.get.exists())
      assertEquals(Errors.NONE, response2.partitionErrors(topicNames).get(topicPartition))

-      // There is no file if the request an older version


These removed lines are no longer relevant.

ijuma · 2025-01-20T07:02:17Z

@chia7712 @mumrah Can one of you please review this? It would be good to get this into 4.0 so that the protocol apis are correctly specified and documented.

ijuma · 2025-01-20T09:31:46Z

Test failures are unrelated - kicked off the tests again to try for a green build.

chia7712

@ijuma thanks for this patch!

clients/src/main/resources/common/message/LeaderAndIsrRequest.json

generator/src/main/java/org/apache/kafka/message/ApiMessageTypeGenerator.java

…emove-server-protocol-versions-3.0 * apache-github/trunk: HOTFIX: ListShareGroupOffsetResult javadoc (apache#18642) KAFKA-18594: Cleanup BrokerLifecycleManager (apache#18626) KAFKA-18232: Add share group state topic prune metrics. (apache#18174) KAFKA-18568: Fix flaky test ClientIdQuotaTest (apache#18612) KAFKA-18553: Update javadoc and comments of ConfigType (apache#18567) [KAFKA-16720] AdminClient Support for ListShareGroupOffsets (1/n) (apache#18571) KAFKA-18588 Remove TopicKey.scala (apache#18624)

… comments

ijuma · 2025-01-20T16:33:43Z

@chia7712 Thanks for the review. Pushed a commit that addresses the comments. Also merged master to this branch for the flaky test fix.

chia7712

LGTM after following comments are addressed

chia7712 · 2025-01-20T18:09:30Z

core/src/test/scala/unit/kafka/server/ReplicaManagerTest.scala

-    val version = if (usesTopicIds) LeaderAndIsrRequestData.HIGHEST_SUPPORTED_VERSION else 4.toShort
-    val topicId = if (usesTopicIds) this.topicId else Uuid.ZERO_UUID
-    val topicIdOpt = if (usesTopicIds) Some(topicId) else None
+    val topicId = this.topicId


This is unnecessary

chia7712 · 2025-01-20T18:20:32Z

core/src/main/scala/kafka/log/LogManager.scala

@@ -35,7 +35,6 @@ import scala.jdk.CollectionConverters._
 import scala.collection._
 import scala.collection.mutable.ArrayBuffer
 import scala.util.{Failure, Success, Try}
-import org.apache.kafka.common.requests.{AbstractControlRequest, LeaderAndIsrRequest}


It seems we can remove AbstractControlRequest in this PR.

Good point - this can go if we move Type to LeaderAndIsrRequest.

clients/src/main/java/org/apache/kafka/common/requests/RequestHeader.java

ijuma · 2025-01-20T19:32:13Z

Assuming the tests look good, I'll merge this. If @jolshan, @cmccabe and/or @jsancio have any comments (for the questions I asked or otherwise), I'll address them in a follow-up.

…18497) Kafka 4.0 will remove support for zk mode and will require conversion to kraft before upgrading to 4.0. The minimum kraft version is 3.0 (aka 3.0-IV1). This provides an opportunity to remove exclusively server side protocols versions that only exist to allow direct upgrades from versions older than 3.0 or that are used only by zk mode. Since KRaft became production ready in 3.3, we should consider setting the baseline to 3.3. But that requires more discussion and it can be done via a separate change (KAFKA-18601). Protocol changes: * Remove RequestHeader v0 (only used by ControlledShutdown v0) * Remove WriteTxnMarkers v0 * Remove all versions of ControlledShutdown, LeaderAndIsr, StopReplica, UpdateMetadata In order to remove all versions safely, extend generator to support setting "versions" to "none". In this case, we no longer generate the `*Data` classes, but we still reserve the id for the relevant protocol api (so it doesn't get accidentally used for something else). The protocol documentation is correct after these changes. We kept a simplified version of `LeaderAndIsr{Request|Response}` because it's used by many tests that are still relevant in kraft mode. Once KAFKA-18486 is done, it may be possible to remove it (I left a comment on the ticket). Similarly, KAFKA-18487 may make it possible to remove the introduced `StopReplicaPartitionState` (left a comment on that ticket too). There are a number of places that were adjusted to include an `ApiKeys.hasValidVersion` check. Reviewers: Chia-Ping Tsai <[email protected]>

…pache#18497) Kafka 4.0 will remove support for zk mode and will require conversion to kraft before upgrading to 4.0. The minimum kraft version is 3.0 (aka 3.0-IV1). This provides an opportunity to remove exclusively server side protocols versions that only exist to allow direct upgrades from versions older than 3.0 or that are used only by zk mode. Since KRaft became production ready in 3.3, we should consider setting the baseline to 3.3. But that requires more discussion and it can be done via a separate change (KAFKA-18601). Protocol changes: * Remove RequestHeader v0 (only used by ControlledShutdown v0) * Remove WriteTxnMarkers v0 * Remove all versions of ControlledShutdown, LeaderAndIsr, StopReplica, UpdateMetadata In order to remove all versions safely, extend generator to support setting "versions" to "none". In this case, we no longer generate the `*Data` classes, but we still reserve the id for the relevant protocol api (so it doesn't get accidentally used for something else). The protocol documentation is correct after these changes. We kept a simplified version of `LeaderAndIsr{Request|Response}` because it's used by many tests that are still relevant in kraft mode. Once KAFKA-18486 is done, it may be possible to remove it (I left a comment on the ticket). Similarly, KAFKA-18487 may make it possible to remove the introduced `StopReplicaPartitionState` (left a comment on that ticket too). There are a number of places that were adjusted to include an `ApiKeys.hasValidVersion` check. Reviewers: Chia-Ping Tsai <[email protected]>

github-actions bot added core Kafka Broker generator RPC and Record code generator clients labels Jan 12, 2025

ijuma mentioned this pull request Jan 12, 2025

KAFKA-18474: Remove zkBroker listener #18477

Merged

3 tasks

ijuma force-pushed the kafka-14552-remove-server-protocol-versions-3.0 branch from a7b8a51 to 1cb6fce Compare January 19, 2025 16:06

github-actions bot added tools performance kraft labels Jan 19, 2025

ijuma force-pushed the kafka-14552-remove-server-protocol-versions-3.0 branch 3 times, most recently from 0e36dba to 48ace0f Compare January 19, 2025 21:45

Remove RequestHeader v0 and WriteTxnMarkers v0

359f956

ijuma force-pushed the kafka-14552-remove-server-protocol-versions-3.0 branch from 48ace0f to 2320dd9 Compare January 19, 2025 23:07

ijuma commented Jan 19, 2025

View reviewed changes

ijuma force-pushed the kafka-14552-remove-server-protocol-versions-3.0 branch from a30378d to 4b200e5 Compare January 19, 2025 23:28

ijuma commented Jan 19, 2025

View reviewed changes

ijuma commented Jan 20, 2025

View reviewed changes

ijuma changed the title ~~KAFKA-14552: Assume a baseline of 3.0 for server protocol versions (WIP)~~ KAFKA-14552: Assume a baseline of 3.0 for server protocol versions Jan 20, 2025

ijuma commented Jan 20, 2025

View reviewed changes

ijuma added 8 commits January 19, 2025 22:28

Remove ControlledShutdown, StopReplica, UpdateMetadata requests/respo…

44d6dd9

…nses and nearly remove LeaderAndIsr Kept a minimal version of LeaderAndIsr as it's used in a few tests - will remove that separately.

Remove usage of UpdateMetadataPartitionState from MetadataCache

d51ded7

Remove usage of StopReplicaPartitionState from ReplicaManager

bca8a32

Remove usage of generated classes about to be deleted

aa72a39

Update generator to support safe removal of protocol apis

8291605

Safely remove ControlledShutdown, LeaderAndIsr, StopReplica, UpdateMe…

6408d5c

…tadata support

Clean-up protocol definitions & comments to remove zk references that…

e4c4258

… are no longer applicable

Fix NPE in Protocol.toHtml and add basic test

b22e3dd

ijuma added 3 commits January 19, 2025 22:28

Remove topic id related tests that are no longer needed

9b10844

Log specific message when a particular api is unsupported (vs unsuppo…

438e0d9

…rted version) and add test

Add generator tests (and refactor to make that possible)

904554c

ijuma force-pushed the kafka-14552-remove-server-protocol-versions-3.0 branch from 4b200e5 to 904554c Compare January 20, 2025 06:29

ijuma commented Jan 20, 2025

View reviewed changes

Simplify LeaderAndIsrRequest

df2acf5

chia7712 reviewed Jan 20, 2025

View reviewed changes

clients/src/main/resources/common/message/LeaderAndIsrRequest.json Show resolved Hide resolved

generator/src/main/java/org/apache/kafka/message/ApiMessageTypeGenerator.java Outdated Show resolved Hide resolved

ijuma added 2 commits January 20, 2025 07:31

Fix bug in ApiMessageType, add test, adjust code and address reviewer…

1d4d2b2

… comments

Minor wording tweak

e54abb8

chia7712 approved these changes Jan 20, 2025

View reviewed changes

ijuma added 2 commits January 20, 2025 11:16

Remove AbstractControlRequest as per review comment

004ba9f

Minor clean-ups as per review comments.

8285b55

ijuma merged commit 87b37a4 into apache:trunk Jan 20, 2025
9 checks passed

ijuma deleted the kafka-14552-remove-server-protocol-versions-3.0 branch January 21, 2025 01:30

KAFKA-14552: Assume a baseline of 3.0 for server protocol versions #18497

KAFKA-14552: Assume a baseline of 3.0 for server protocol versions #18497

Uh oh!

Conversation

ijuma commented Jan 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Committer Checklist (excluded from commit message)

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 Jan 20, 2025 • 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!

ijuma commented Jan 20, 2025

Uh oh!

ijuma commented Jan 20, 2025

Uh oh!

chia7712 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ijuma commented Jan 20, 2025

Uh oh!

chia7712 left a comment

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!

Uh oh!

ijuma commented Jan 20, 2025

Uh oh!

Uh oh!

Uh oh!

ijuma commented Jan 12, 2025 •

edited

Loading

ijuma Jan 20, 2025 •

edited

Loading