KAFKA-18601: Assume a baseline of 3.3 for server protocol versions #18845

ijuma · 2025-02-09T17:04:54Z

3.3.0 was the first KRaft release that was deemed production-ready and also when KIP-778 (KRaft to KRaft upgrades) landed. Given that, it's reasonable for 4.x to only support upgrades from 3.3.0 or newer (the metadata version also needs to be set to "3.3" or newer before upgrading).

Noteworthy changes:

AlterPartition no longer includes topic names, which makes it possible to simplify AlterParitionManager logic.
Metadata versions older than IBP_3_3_IV3 have been removed and IBP_3_3_IV3 is now the minimum version.
MINIMUM_BOOTSTRAP_VERSION has been removed.
Removed isLeaderRecoverySupported, isNoOpsRecordSupported, isKRaftSupported, isBrokerRegistrationChangeRecordSupported and isInControlledShutdownStateSupported - these are always true now. Also removed related conditional code.
Removed default metadata version or metadata version fallbacks in multiple places - we now fail-fast instead of potentially using an incorrect metadata version.
Update MetadataBatchLoader.resetToImage to set hasSeenRecord based on whether image is empty - this was a previously existing issue that became more apparent after the changes in this PR.
Remove ibp parameter from BootstrapDirectory
A number of tests were not useful anymore and have been removed.

I will update the upgrade notes via a separate PR as there are a few things that need changing and it would be easier to do so that way.

Committer Checklist (excluded from commit message)

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

Copilot reviewed 61 out of 78 changed files in this pull request and generated no comments.

Files not reviewed (17)

clients/src/main/resources/common/message/AlterPartitionRequest.json: Language not supported
clients/src/main/resources/common/message/AlterPartitionResponse.json: Language not supported
core/src/main/scala/kafka/cluster/Partition.scala: Language not supported
core/src/main/scala/kafka/server/AlterPartitionManager.scala: Language not supported
core/src/main/scala/kafka/server/ControllerRegistrationManager.scala: Language not supported
core/src/main/scala/kafka/server/KafkaConfig.scala: Language not supported
core/src/main/scala/kafka/tools/StorageTool.scala: Language not supported
core/src/main/scala/kafka/tools/TestRaftServer.scala: Language not supported
core/src/test/scala/integration/kafka/coordinator/transaction/ProducerIntegrationTest.scala: Language not supported
core/src/test/scala/integration/kafka/server/KRaftClusterTest.scala: Language not supported
core/src/test/scala/integration/kafka/server/MetadataVersionIntegrationTest.scala: Language not supported
core/src/test/scala/kafka/server/RemoteLeaderEndPointTest.scala: Language not supported
core/src/test/scala/unit/kafka/cluster/PartitionLockTest.scala: Language not supported
core/src/test/scala/unit/kafka/cluster/PartitionTest.scala: Language not supported
core/src/test/scala/unit/kafka/server/AbstractApiVersionsRequestTest.scala: Language not supported
core/src/test/scala/unit/kafka/server/AlterPartitionManagerTest.scala: Language not supported
core/src/test/scala/unit/kafka/server/BrokerRegistrationRequestTest.scala: Language not supported

Comments suppressed due to low confidence (3)

clients/src/test/java/org/apache/kafka/common/requests/AlterPartitionRequestTest.java:46

The removal of the topicName field might lead to incomplete test coverage if any tests rely on it. Verify if topicName is essential for any test cases.

TopicData topicData = new TopicData().setTopicId(topicId);

clients/src/test/java/org/apache/kafka/common/requests/RequestResponseTest.java:1705

The removal of the 'topicName' field may cause unintended side effects if the 'topicName' is required elsewhere in the codebase. Please verify if 'topicName' is needed.

.setTopicId(Uuid.randomUuid())

clients/src/test/java/org/apache/kafka/common/requests/RequestResponseTest.java:1723

The removal of the 'topicName' field may cause unintended side effects if the 'topicName' is required elsewhere in the codebase. Please verify if 'topicName' is needed.

.setTopicId(Uuid.randomUuid())

tools/src/test/java/org/apache/kafka/tools/reassign/ReassignPartitionsCommandTest.java

core/src/main/scala/kafka/cluster/Partition.scala

metadata/src/main/java/org/apache/kafka/image/node/MetadataNode.java

metadata/src/main/java/org/apache/kafka/metadata/bootstrap/BootstrapDirectory.java

ijuma · 2025-02-10T06:04:27Z

metadata/src/test/java/org/apache/kafka/image/ImageDowngradeTest.java

-     * Test downgrading to a MetadataVersion that doesn't support FeatureLevelRecord.
-     */
-    @Test
-    public void testPremodernVersion() {


We don't support these anymore, so remove it.

ijuma · 2025-02-10T06:04:48Z

metadata/src/test/java/org/apache/kafka/image/ImageDowngradeTest.java

-     * Test downgrading to a MetadataVersion that doesn't support inControlledShutdown.
-     */
-    @Test
-    public void testPreControlledShutdownStateVersion() {


We now always have the controlled shutdown state.

ijuma · 2025-02-10T21:02:12Z

server-common/src/test/java/org/apache/kafka/server/common/MetadataVersionTest.java

-    }
-
-    @Test
-    public void testKRaftVersions() {


This did not seem to add much value after the changes in this PR.

chia7712

@ijuma thanks for this patch. overall LGTM. some minor comments remains.

metadata/src/main/java/org/apache/kafka/metadata/bootstrap/BootstrapDirectory.java

metadata/src/test/java/org/apache/kafka/metadata/bootstrap/BootstrapMetadataTest.java

metadata/src/main/java/org/apache/kafka/controller/FeatureControlManager.java

tools/src/test/java/org/apache/kafka/tools/reassign/ReassignPartitionsCommandTest.java

metadata/src/main/java/org/apache/kafka/controller/ReplicationControlManager.java

metadata/src/main/java/org/apache/kafka/controller/QuorumController.java

metadata/src/main/java/org/apache/kafka/metadata/BrokerRegistration.java

…ssume-baseline-3.3-for-server * apache-github/trunk: KAFKA-18366 Remove KafkaConfig.interBrokerProtocolVersion (apache#18820) KAFKA-18658 add import control for examples module (apache#18812) MINOR: Java version and TLS documentation improvements (apache#18822) KAFKA-18743 Remove leader.imbalance.per.broker.percentage as it is not supported by Kraft (apache#18821) KAFKA-18225 ClientQuotaCallback#updateClusterMetadata is unsupported by kraft (apache#18196) KAFKA-18441: Remove flaky tag on KafkaAdminClientTest#testAdminClientApisAuthenticationFailure (apache#18847) MINOR: fix KStream#to incorrect javadoc (apache#18838) KAFKA-18763: changed the assertion statement for acknowledgements to include only successful acks (apache#18846) MINOR: Accept specifying consumer group assignors by their short names (apache#18832)

…silently doing the wrong thing

ijuma · 2025-02-15T20:35:42Z

core/src/test/scala/integration/kafka/server/KRaftClusterTest.scala

@@ -437,18 +438,6 @@ class KRaftClusterTest {
    }
  }

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


It's no longer possible to set the bootstrap metadata version to lower than 3.3-IV3.

…llowing FeaturesDelta to create a FeaturesImage with no metadata version

ijuma · 2025-02-18T18:18:13Z

@junrao @chia7712 The tests for JDK 23 were all passing for the last run (the JDK 17 build failed, but it didn't look related to this PR).

I think there's only one outstanding issue: how to handle the case if we read an old metadata version from the log. I'll update the relevant comment later today. Does the rest look good to you?

…pen frequently enough that it's not worth handling this edge case.

ijuma · 2025-02-18T20:13:56Z

I resolved the outstanding item, so no known issues remain in this PR.

chia7712

@ijuma thanks for this patch. a couple of comments remain.

chia7712 · 2025-02-18T20:48:59Z

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

@@ -92,8 +87,7 @@ object AlterPartitionManager {
      scheduler = scheduler,
      time = time,
      brokerId = config.brokerId,
-      brokerEpochSupplier = brokerEpochSupplier,
-      metadataVersionSupplier = () => metadataCache.metadataVersion()


we can remove metadataCache from the apply method

chia7712 · 2025-02-18T21:24:07Z

metadata/src/main/java/org/apache/kafka/image/writer/ImageWriterOptions.java

        }

        public Builder(MetadataImage image) {
-            this.metadataVersion = image.features().metadataVersion();
+            this.metadataVersion = image.features().metadataVersionOrThrow();
            this.isEligibleLeaderReplicasEnabled = image.features().isElrEnabled();
        }

        public Builder setMetadataVersion(MetadataVersion metadataVersion) {


After removing those if-else, it seems we don't need the builder anymore. Maybe we can rewrite ImageWriterOptions by record class

This is a general pattern for the metadata module, I would rather not change it as part of this PR. We can have that discussion via a separate JIRA if useful.

chia7712 · 2025-02-18T21:30:59Z

metadata/src/test/java/org/apache/kafka/image/MetadataVersionChangeTest.java

@@ -30,31 +30,31 @@
 @Timeout(value = 40)
 public class MetadataVersionChangeTest {

-    private static final MetadataVersionChange CHANGE_3_0_IV1_TO_3_3_IV0 =
-        new MetadataVersionChange(IBP_3_0_IV1, IBP_3_3_IV0);
+    private static final MetadataVersionChange CHANGE_MINUMUM_TO_LATEST =


typo: MINUMUM -> MINIMUM

chia7712 · 2025-02-18T21:37:01Z

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

@@ -114,7 +107,7 @@ class DefaultAlterPartitionManager(
  // and re-created, we cannot have two entries in this Map especially if we cannot 
  // use an AlterPartition request version which supports topic ids in the end because 
  // the two updates with the same name would be merged together.
-  private[server] val unsentIsrUpdates: util.Map[TopicPartition, AlterPartitionItem] = new ConcurrentHashMap[TopicPartition, AlterPartitionItem]()
+  private[server] val unsentIsrUpdates = new ConcurrentHashMap[TopicIdPartition, AlterPartitionItem]()


Could you please update the docs according to the change?

chia7712 · 2025-02-18T21:51:56Z

metadata/src/test/java/org/apache/kafka/controller/ClusterControlManagerTest.java

-    @Test
-    public void testRegisterControlWithOlderMetadataVersion() {
-        FeatureControlManager featureControl = new FeatureControlManager.Builder().
-            setMetadataVersion(MetadataVersion.IBP_3_3_IV0).


This test appears necessary as it verifies MV which is older than IBP_3_7_IV0. Perhaps we could simply change IBP_3_3_IV0 to IBP_3_3_IV3 instead.

I deleted that because we have testRegistrationWithUnsupportedMetadataVersion. Does that address your concern?

It appears that testRegistrationWithUnsupportedMetadataVersion is designed to test broker registration, while testRegisterControlWithOlderMetadataVersion is for controller registration.

Good catch, I had missed that. I'll add the test back with IBP_3_6_IV2 since that's the newest IBP that still fails.

chia7712 · 2025-02-18T22:00:21Z

metadata/src/test/java/org/apache/kafka/image/writer/ImageWriterOptionsTest.java

@@ -48,40 +48,33 @@
 public class ImageWriterOptionsTest {
    @Test
    public void testDefaultLossHandler() {
-        ImageWriterOptions options = new ImageWriterOptions.Builder().build();
+        ImageWriterOptions options = new ImageWriterOptions.Builder(MetadataVersion.latestProduction()).build();
        assertEquals("stuff", assertThrows(UnwritableMetadataException.class,
                () -> options.handleLoss("stuff")).loss());
    }

    @Test
    public void testSetMetadataVersion() {


If we rewrite ImageWriterOptions by record class, this testSetMetadataVersion is unnecessary as all are pure setters.

I agree that this test isn't useful since the special logic for the setter was removed - even without removing the builder. I removed it.

ijuma · 2025-02-19T00:22:13Z

@chia7712 I addressed your comments.

chia7712

LGTM

ijuma · 2025-02-19T13:29:44Z

I will go ahead and merge this. If @junrao has any additional comments, I will submit a separate PR to address those.

mumrah · 2025-02-19T13:46:04Z

Thanks for pushing this over the line @ijuma. It's great to see these cleanups and removals. "IBP" is all but a memory now :)

…pache#18845) 3.3.0 was the first KRaft release that was deemed production-ready and also when KIP-778 (KRaft to KRaft upgrades) landed. Given that, it's reasonable for 4.x to only support upgrades from 3.3.0 or newer (the metadata version also needs to be set to "3.3" or newer before upgrading). Noteworthy changes: 1. `AlterPartition` no longer includes topic names, which makes it possible to simplify `AlterParitionManager` logic. 2. Metadata versions older than `IBP_3_3_IV3` have been removed and `IBP_3_3_IV3` is now the minimum version. 3. `MINIMUM_BOOTSTRAP_VERSION` has been removed. 4. Removed `isLeaderRecoverySupported`, `isNoOpsRecordSupported`, `isKRaftSupported`, `isBrokerRegistrationChangeRecordSupported` and `isInControlledShutdownStateSupported` - these are always `true` now. Also removed related conditional code. 5. Removed default metadata version or metadata version fallbacks in multiple places - we now fail-fast instead of potentially using an incorrect metadata version. 6. Update `MetadataBatchLoader.resetToImage` to set `hasSeenRecord` based on whether image is empty - this was a previously existing issue that became more apparent after the changes in this PR. 7. Remove `ibp` parameter from `BootstrapDirectory` 8. A number of tests were not useful anymore and have been removed. I will update the upgrade notes via a separate PR as there are a few things that need changing and it would be easier to do so that way. Reviewers: Chia-Ping Tsai <[email protected]>, Jun Rao <[email protected]>, David Arthur <[email protected]>, Colin P. McCabe <[email protected]>, Justine Olshan <[email protected]>, Ken Huang <[email protected]>

…18845) 3.3.0 was the first KRaft release that was deemed production-ready and also when KIP-778 (KRaft to KRaft upgrades) landed. Given that, it's reasonable for 4.x to only support upgrades from 3.3.0 or newer (the metadata version also needs to be set to "3.3" or newer before upgrading). Noteworthy changes: 1. `AlterPartition` no longer includes topic names, which makes it possible to simplify `AlterParitionManager` logic. 2. Metadata versions older than `IBP_3_3_IV3` have been removed and `IBP_3_3_IV3` is now the minimum version. 3. `MINIMUM_BOOTSTRAP_VERSION` has been removed. 4. Removed `isLeaderRecoverySupported`, `isNoOpsRecordSupported`, `isKRaftSupported`, `isBrokerRegistrationChangeRecordSupported` and `isInControlledShutdownStateSupported` - these are always `true` now. Also removed related conditional code. 5. Removed default metadata version or metadata version fallbacks in multiple places - we now fail-fast instead of potentially using an incorrect metadata version. 6. Update `MetadataBatchLoader.resetToImage` to set `hasSeenRecord` based on whether image is empty - this was a previously existing issue that became more apparent after the changes in this PR. 7. Remove `ibp` parameter from `BootstrapDirectory` 8. A number of tests were not useful anymore and have been removed. I will update the upgrade notes via a separate PR as there are a few things that need changing and it would be easier to do so that way. Reviewers: Chia-Ping Tsai <[email protected]>, Jun Rao <[email protected]>, David Arthur <[email protected]>, Colin P. McCabe <[email protected]>, Justine Olshan <[email protected]>, Ken Huang <[email protected]>

…solated_mode_upgrade (#19003) #18845 assumed a baseline of 3.3 for server protocol versions so that the lower version couldn't roll up to 4.0. Hence, the `TestUpgrade#test_combined_mode_upgrad` and `test_isolated_mode_upgrade` failed for the 3.1 and 3.2 versions. e2e tests result with this patch on jenkins: ![Screenshot from 2025-02-22 13-22-17](https://github.com/user-attachments/assets/2de6f707-8281-4f30-b5d0-83dd4de9666d) e2e tests result with this patch on local machine: ![Screenshot from 2025-02-22 13-28-16](https://github.com/user-attachments/assets/2e5e563a-1ac4-4894-ba30-593304697d1d) Reviewers: David Jacot <[email protected]>

ijuma requested a review from Copilot February 9, 2025 17:04

github-actions bot added core Kafka Broker tools kraft storage Pull requests that target the storage module clients labels Feb 9, 2025

Copilot AI reviewed Feb 9, 2025

View reviewed changes

ijuma commented Feb 9, 2025

View reviewed changes

tools/src/test/java/org/apache/kafka/tools/reassign/ReassignPartitionsCommandTest.java Show resolved Hide resolved

ijuma force-pushed the kafka-18601-assume-baseline-3.3-for-server branch from 3643409 to a7fbe00 Compare February 10, 2025 03:49

ijuma commented Feb 10, 2025

View reviewed changes

core/src/main/scala/kafka/cluster/Partition.scala Outdated Show resolved Hide resolved

ijuma force-pushed the kafka-18601-assume-baseline-3.3-for-server branch 3 times, most recently from 4b19a3d to 890c3b2 Compare February 10, 2025 05:52

ijuma commented Feb 10, 2025

View reviewed changes

metadata/src/main/java/org/apache/kafka/image/node/MetadataNode.java Show resolved Hide resolved

ijuma commented Feb 10, 2025

View reviewed changes

metadata/src/main/java/org/apache/kafka/metadata/bootstrap/BootstrapDirectory.java Show resolved Hide resolved

ijuma commented Feb 10, 2025

View reviewed changes

KAFKA-18601: Assume a baseline of 3.3 for server protocol versions

e654cdb

ijuma force-pushed the kafka-18601-assume-baseline-3.3-for-server branch from 890c3b2 to e654cdb Compare February 10, 2025 06:06

ijuma added 2 commits February 10, 2025 07:31

Fix a few tests and tweak error messages

059866e

Minor improvements, address review comments and fix tests

86d1053

ijuma force-pushed the kafka-18601-assume-baseline-3.3-for-server branch from 9622ce9 to 86d1053 Compare February 10, 2025 20:54

Formatting fixes

97e2c43

ijuma commented Feb 10, 2025

View reviewed changes

chia7712 reviewed Feb 10, 2025

View reviewed changes

chia7712 mentioned this pull request Feb 10, 2025

KAFKA-18366 Remove KafkaConfig.interBrokerProtocolVersion #18820

Merged

3 tasks

ijuma added 2 commits February 10, 2025 15:22

Minor tweak

b60fbe4

github-actions bot added the performance label Feb 11, 2025

Remove ibp parameter from BootstrapDirectory

9fb5b3d

Fix test failures

a551f46

Yunyung mentioned this pull request Feb 15, 2025

KAFKA-18803: The acls would appear at the wrong level of the metadata shell "tree" #18916

Merged

3 tasks

Make FeaturesImage.metadataVersion optional and fail-fast instead of …

5834410

…silently doing the wrong thing

ijuma commented Feb 15, 2025

View reviewed changes

ijuma added 2 commits February 15, 2025 12:44

Minor tweak

afb9d97

More 3_3_IV3 -> MINIMUM_VERSION

d5860c4

chia7712 mentioned this pull request Feb 16, 2025

KAFKA-18795: Remove Records#downConvert #18897

Merged

3 tasks

ijuma added 3 commits February 18, 2025 06:38

Fix several tests

4505d0a

Fix ReconfigurableQuorumIntegrationTest and QuorumControllerTest by a…

7fcb280

…llowing FeaturesDelta to create a FeaturesImage with no metadata version

Minor clean-ups in ReconfigurableQuorumIntegrationTest

d505508

ijuma force-pushed the kafka-18601-assume-baseline-3.3-for-server branch from eb55590 to d505508 Compare February 18, 2025 14:42

"3-3-IV3" -> MINIMUM_VERSION.toString

07a728b

Remove metadata version fallback from FeaturesDelta - snapshots hap…

52fd223

…pen frequently enough that it's not worth handling this edge case.

chia7712 reviewed Feb 18, 2025

View reviewed changes

Address review feedback.

6c5a0f9

chia7712 approved these changes Feb 19, 2025

View reviewed changes

ijuma merged commit 3dba312 into apache:trunk Feb 19, 2025
9 checks passed

ijuma deleted the kafka-18601-assume-baseline-3.3-for-server branch February 19, 2025 13:35

This was referenced Feb 21, 2025

MINOR: Move the ELR default version to 4.1 #18954

Merged

KAFKA-18844: Stale features information in QuorumController#registerBroker #18997

Open

frankvicky mentioned this pull request Feb 22, 2025

MINOR: Fix fail e2e TestUpgrade#test_combined_mode_upgrade and test_isolated_mode_upgrade #19003

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

KAFKA-18601: Assume a baseline of 3.3 for server protocol versions #18845

KAFKA-18601: Assume a baseline of 3.3 for server protocol versions #18845

ijuma commented Feb 9, 2025 •

edited

Loading

ijuma Feb 10, 2025 •

edited

Loading

ijuma Feb 10, 2025

ijuma Feb 10, 2025

chia7712 left a comment

ijuma Feb 15, 2025

ijuma commented Feb 18, 2025

ijuma commented Feb 18, 2025

chia7712 left a comment

chia7712 Feb 18, 2025

chia7712 Feb 18, 2025

ijuma Feb 18, 2025

chia7712 Feb 18, 2025

chia7712 Feb 18, 2025

chia7712 Feb 18, 2025

ijuma Feb 18, 2025

chia7712 Feb 18, 2025

ijuma Feb 18, 2025

chia7712 Feb 18, 2025

ijuma Feb 18, 2025 •

edited

Loading

ijuma commented Feb 19, 2025

chia7712 left a comment

ijuma commented Feb 19, 2025

mumrah commented Feb 19, 2025

KAFKA-18601: Assume a baseline of 3.3 for server protocol versions #18845

KAFKA-18601: Assume a baseline of 3.3 for server protocol versions #18845

Conversation

ijuma commented Feb 9, 2025 • edited Loading

Committer Checklist (excluded from commit message)

Choose a reason for hiding this comment

ijuma Feb 10, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chia7712 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ijuma commented Feb 18, 2025

ijuma commented Feb 18, 2025

chia7712 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ijuma Feb 18, 2025 • edited Loading

Choose a reason for hiding this comment

ijuma commented Feb 19, 2025

chia7712 left a comment

Choose a reason for hiding this comment

ijuma commented Feb 19, 2025

mumrah commented Feb 19, 2025

ijuma commented Feb 9, 2025 •

edited

Loading

ijuma Feb 10, 2025 •

edited

Loading

ijuma Feb 18, 2025 •

edited

Loading