KAFKA-15922: Bump MetadataVersion to support JBOD with KRaft by pprovenzano · Pull Request #14984 · apache/kafka

pprovenzano · 2023-12-11T15:39:25Z

Update the LATEST_PRODUCTION to include JBOD and fetch request for KIP-951.

Remove ELR MetadataVersion from 3.7 and push it to 3.8. Since it was not considered stable the MV is burned.

Add 3.8-IV0 as the next MetadataVersion which is currently not considered stable.

Fix tests to reflect this state.

This just moved 3.7_IV3 to 4.0_IV0

rondagostino

LGTM assuming tests pass.

OmniaGM

LGTM

rondagostino · 2023-12-11T22:27:17Z

@pprovenzano Can you look at the test failures?
60 tests have failed. There are 0 new tests failing, 60 existing failing
https://ci-builds.apache.org/blue/organizations/jenkins/Kafka%2Fkafka-pr/detail/PR-14984/2/tests

There is no short version of 4.0 until 4.0 becomes a stable version

pprovenzano · 2023-12-12T04:14:00Z

Okay, I think I have addresses the issues.

3.7-IV0 (15): Controller Registration 3.7-IV1 (16): burned in AK, reused in CE for CL fail-back 3.7-IV2 (17): JBOD 3.7-IV4 (19): Fetch RPC change (<-- “Production Ready”) 3.8-IV0 (20): ELR MVs of non Production Ready items cannot be recyled but the feature itself can be removed and consume a later MV. This is the case with ELR which is no longer 3-7-IV3 (18) and has been moved to 3-8-IV0 which is not production ready.

rondagostino

Left a few comments based on latest changes.

rondagostino · 2023-12-13T22:31:35Z

    // Add JBOD support for KRaft.
    IBP_3_7_IV2(17, "3.7", "IV2", true),

+    // IBP_3_7_IV3 was ELR related supports (KIP-966) and has been moved forward


I think we should keep the value and just not have anything associated with it. Call it "reserved", similar to IBP_3_7_IV1. WDYT?

I think that makes sense.

very small nit here -- but the comment could be a little clearer. Something like

IBP_3_7_IV3 was reserved for ELR changes (KIP-966) but has been deferred to a later MV

Okay, I updated the comment to try to make it clearer. This is where we really need the KIP.

jolshan

left one small nit, but otherwise lgtm assuming tests look good.

jsancio

Please update the PR description to explain what issue is being fixed. The description doesn't match the changes included in this PR.

Same with the PR title. Is the KAFKA-15922 also related to this PR?

jsancio · 2023-12-14T01:36:11Z

+    // IBP_3_7_IV3 was ELR related supports (KIP-966) and has been moved forward
+    IBP_3_7_IV3(18, "3.7", "IV3", false),
+
+    // Add new fetch request version for KIP-951
+    IBP_3_7_IV4(19, "3.7", "IV4", false),
+
    // Add ELR related supports (KIP-966).
-    IBP_3_7_IV3(18, "3.7", "IV3", true);
+    IBP_3_8_IV0(20, "3.8", "IV0", true);


Can you split this change into two PRs/commits? One that fixes the MV in 3.7 and one that introduced the new 3.8 MV. The one that fixes the 3.7 versions should be cherry picked to the 3.7 branch.

If we remove the tag from here should we also remove all the other code associated with ELR from the 3.7 branch?
I don't think removing it from the 3.7 branch really buys us much of anything except more code churn.

You just need to return false for isElrSupported(). Is that what you are asking?

No, you asked to split this PR into two separate PRs, one for 3.7 without any reference to 3.8 and this one. I am saying that doing that has no value because 3.8 is labeled as unstable so having it in 3.7 isn't an issue. Customers cannot use 3.8 unless they explicitly allow unstable metadata versions.
If we want 3.7 to be clean then you would also need to remove all ELR code not just the MV referencing it. This would be a BIG change because it removes tests and updates the JSON files representing the record format.

pprovenzano · 2023-12-14T03:24:02Z

Please update the PR description to explain what issue is being fixed. The description doesn't match the changes included in this PR.

Same with the PR title. Is the KAFKA-15922 also related to this PR?

The original PRs for KAFKA-15922 are what got us to this point. My PR here was just meant to bump the latest stable version to the MV supporting JBOD. I've added a little more to reduce the number of PRs that need to be committed to streamline things. All the changes though are related to KAFKA-15922. I have updated the comment to try to reflect the current state of changes. I have no idea what to change the title to.

ijuma · 2023-12-14T07:41:18Z

Java 21 build passed, others have failures that look unrelated:

Build / JDK 11 and Scala 2.13 / org.apache.kafka.connect.mirror.integration.MirrorConnectorsIntegrationBaseTest.testSyncTopicConfigs()
Build / JDK 11 and Scala 2.13 / kafka.network.ConnectionQuotasTest.testListenerConnectionRateLimitWhenActualRateAboveLimit()
Build / JDK 11 and Scala 2.13 / kafka.network.SocketServerTest.testSaslReauthenticationFailureWithKip152SaslAuthenticate()
Build / JDK 11 and Scala 2.13 / org.apache.kafka.trogdor.coordinator.CoordinatorTest.testTaskRequestWithOldStartMsGetsUpdated()
Build / JDK 17 and Scala 2.13 / kafka.api.ConsumerBounceTest.testConsumptionWithBrokerFailures()
Build / JDK 17 and Scala 2.13 / org.apache.kafka.tiered.storage.integration.TransactionsWithTieredStoreTest.testBumpTransactionalEpoch(String).quorum=kraft
Build / JDK 8 and Scala 2.12 / org.apache.kafka.connect.mirror.integration.MirrorConnectorsIntegrationTransactionsTest.testSyncTopicConfigs()
Build / JDK 8 and Scala 2.12 / kafka.api.DelegationTokenEndToEndAuthorizationWithOwnerTest.testProduceConsumeWithWildcardAcls(String).quorum=kraft
Build / JDK 8 and Scala 2.12 / kafka.api.SaslMultiMechanismConsumerTest.testCoordinatorFailover(String, String).quorum=zk.groupProtocol=generic
Build / JDK 8 and Scala 2.12 / kafka.network.ConnectionQuotasTest.testListenerConnectionRateLimitWhenActualRateAboveLimit()
Build / JDK 8 and Scala 2.12 / org.apache.kafka.controller.QuorumControllerTest.testBalancePartitionLeaders()
Build / JDK 8 and Scala 2.12 / org.apache.kafka.tiered.storage.integration.TransactionsWithTieredStoreTest.testBumpTransactionalEpoch(String).quorum=kraft
Build / JDK 8 and Scala 2.12 / org.apache.kafka.tiered.storage.integration.TransactionsWithTieredStoreTest.testSendOffsetsToTransactionTimeout(String).quorum=kraft
Build / JDK 8 and Scala 2.12 / org.apache.kafka.tools.MetadataQuorumCommandTest.testDescribeQuorumStatusSuccessful [5] Type=Raft-Combined, MetadataVersion=3.8-IV0, Security=PLAINTEXT
Build / JDK 11 and Scala 2.13 / kafka.api.TransactionsBounceTest.testWithGroupId()

rondagostino

LGTM. One missing test that we can add separately (it was always missing -- it is not missing due to this PR). I will merge this and then cherry-pick it to 3.7. Thanks for the adjustments!

rondagostino · 2023-12-14T14:52:52Z

-        if (this.isAtLeast(IBP_3_7_IV0)) {
+        if (this.isAtLeast(IBP_3_7_IV4)) {


Noting that we do not have a test for this. In the interest of time, let's add a test for this in a follow-on PR rather than here unless we find other issues.

rondagostino · 2023-12-14T14:54:32Z

    }

-    @ClusterTest(clusterType = Type.KRAFT, metadataVersion = MetadataVersion.IBP_3_7_IV0)
+    @ClusterTest(clusterType = Type.KRAFT, metadataVersion = MetadataVersion.IBP_3_7_IV4)


Seem that this change is unnecessary, but it doesn't hurt, either - so let's leave it alone in the interest of time unless we find something else that definitely needs fixing.

Moves ELR from MetadataVersion IBP_3_7_IV3 into the new IBP_3_8_IV0 because the ELR feature was not completed before 3.7 reached feature freeze. Leaves IBP_3_7_IV3 empty -- it is a no-op and is not reused for anything. Adds the new MetadataVersion IBP_3_7_IV4 for the FETCH request changes from KIP-951, which were mistakenly never associated with a MetadataVersion. Updates the LATEST_PRODUCTION MetadataVersion to IBP_3_7_IV4 to declare both KRaft JBOD and the KIP-951 changes ready for production use. Reviewers: Omnia G H Ibrahim <o.g.h.ibrahim@gmail.com>, Ron Dagostino <rdagostino@confluent.io>, Ismael Juma <ismael@juma.me.uk>, José Armando García Sancio <jsancio@apache.org>, Justine Olshan <jolshan@confluent.io>

rondagostino · 2023-12-14T15:10:20Z

Merged to 3.7

…14984) Moves ELR from MetadataVersion IBP_3_7_IV3 into the new IBP_3_8_IV0 because the ELR feature was not completed before 3.7 reached feature freeze. Leaves IBP_3_7_IV3 empty -- it is a no-op and is not reused for anything. Adds the new MetadataVersion IBP_3_7_IV4 for the FETCH request changes from KIP-951, which were mistakenly never associated with a MetadataVersion. Updates the LATEST_PRODUCTION MetadataVersion to IBP_3_7_IV4 to declare both KRaft JBOD and the KIP-951 changes ready for production use. Reviewers: Omnia G H Ibrahim <o.g.h.ibrahim@gmail.com>, Ron Dagostino <rdagostino@confluent.io>, Ismael Juma <ismael@juma.me.uk>, José Armando García Sancio <jsancio@apache.org>, Justine Olshan <jolshan@confluent.io>

pprovenzano added 2 commits December 11, 2023 10:32

Bump MetadataVersion to support JBOD with KRaft

ed07b44

All Metadata versions post JBOD are not in 4.0.

d2f3ddb

This just moved 3.7_IV3 to 4.0_IV0

rondagostino approved these changes Dec 11, 2023

View reviewed changes

OmniaGM approved these changes Dec 11, 2023

View reviewed changes

We aren't going to support migration in 4.0 because ZK won't be in 4.0

45dacd5

There is no short version of 4.0 until 4.0 becomes a stable version

ijuma reviewed Dec 12, 2023

View reviewed changes

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

pprovenzano added 3 commits December 12, 2023 10:31

Update comment

e2f61f1

Another missed test fixed.

2af4988

pprovenzano requested review from OmniaGM, ijuma and rondagostino December 13, 2023 22:20

rondagostino reviewed Dec 13, 2023

View reviewed changes

pprovenzano added 2 commits December 13, 2023 18:53

Fixup with comments from Ron

ae4357f

Missed a test

9c9cd1f

jolshan approved these changes Dec 14, 2023

View reviewed changes

jsancio reviewed Dec 14, 2023

View reviewed changes

Update comment to try to make it a little clearer

018d785

rondagostino approved these changes Dec 14, 2023

View reviewed changes

rondagostino merged commit b0e99b5 into apache:trunk Dec 14, 2023

junrao mentioned this pull request Apr 8, 2024

KAFKA-16480: Bump ListOffsets version, IBP version and mark last version of ListOffsets as unstable #15673

Closed

		if (this.isAtLeast(IBP_3_7_IV0)) {
		if (this.isAtLeast(IBP_3_7_IV4)) {

Conversation

pprovenzano commented Dec 11, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rondagostino left a comment

Choose a reason for hiding this comment

Uh oh!

OmniaGM left a comment

Choose a reason for hiding this comment

Uh oh!

rondagostino commented Dec 11, 2023

Uh oh!

pprovenzano commented Dec 12, 2023

Uh oh!

Uh oh!

rondagostino left a comment

Choose a reason for hiding this comment

Uh oh!

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jolshan left a comment

Choose a reason for hiding this comment

Uh oh!

jsancio left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pprovenzano Dec 14, 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!

jsancio Dec 14, 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!

pprovenzano Dec 14, 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!

pprovenzano commented Dec 14, 2023

Uh oh!

ijuma commented Dec 14, 2023

Uh oh!

rondagostino 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!

rondagostino commented Dec 14, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

pprovenzano commented Dec 11, 2023 •

edited

Loading

pprovenzano Dec 14, 2023 •

edited

Loading

jsancio Dec 14, 2023 •

edited

Loading

pprovenzano Dec 14, 2023 •

edited

Loading