KAFKA-17018: update MetadataVersion for the Kafka release 3.9 by cmccabe · Pull Request #16841 · apache/kafka

cmccabe · 2024-08-08T23:07:29Z

Mark 3.9-IV0 as stable. Metadata version 3.9-IV0 should return Fetch version 17.
Move ELR to 4.0-IV0. Remove 3.9-IV1 since it's no longer needed.
Create a new 4.0-IV1 MV for KIP-848.

- Mark 3.9-IV0 as stable. Metadata version 3.9-IV0 should return Fetch version 17. - Move ELR to 4.0-IV0. Remove 3.9-IV1 since it's no longer needed. - Create a new 4.0-IV1 MV for KIP-848.

junrao

@cmccabe : Thanks for the PR. Left a few comments.

junrao · 2024-08-09T18:11:17Z

     * IT CANNOT BE CHANGED.</strong>
     */
-    public static final MetadataVersion LATEST_PRODUCTION = IBP_3_8_IV0;
+    public static final MetadataVersion LATEST_PRODUCTION = IBP_3_9_IV0;


Hmm, ListOffsetRequest has latestVersionUnstable set to true. So, it's kind of weird to have a production MV depending on a unstable PRC.

@clolov @showuon @chia7712 : Is KIP-1005 done? Could we set latestVersionUnstable to false for ListOffsetRequest in 3.9?

Is KIP-1005 done? Could we set latestVersionUnstable to false for ListOffsetRequest in 3.9?

KIP-1005 is completed and backport to 3.9, so yes latestVersionUnstable should be false now.

@junrao : yes, KIP-1005 will ship in AK 3.9.

@chia7712 do you plan to update

kafka/clients/src/main/resources/common/message/ListOffsetsRequest.json

Line 42 in df04887

"latestVersionUnstable": true,

do you plan to update

I assumed @cmccabe will update it in this PR 😄

Will file a jira and PR for trunk and 3.9 later

Yes -- sorry I was imprecise. If the api version is advertised by a MV that is stable it sould also be stable.

I didn't realize ListOffset was being used in the replication path. I guess this is just for tiered storage.

However, by default, v9 of ListOffset is not supported with latestVersionUnstable set to true. So, I still don't understand why some existing tests don't fail. We probably need to understand the testing gap. If we have sufficient test coverage, we probably don't need additional protocols.

yes, you are right. we should have a test for replica path. I just write a IT which can produce following error when LIST_OFFSET has unstable version:

[2024-08-14 03:27:13,119] ERROR Closing socket for 127.0.0.1:36885-127.0.0.1:48854-3 because of error (kafka.network.Processor:76) org.apache.kafka.common.errors.InvalidRequestException: Received request api key LIST_OFFSETS with version 9 which is not enabled

Also, it seems we create IT with unstable APIs always [0] and I guess that is why we don't see failed tests.

Summary

broker-to-broker: the LIST_OFFSET version is based on MV [1], and we use IT to protect us from updating MV with an unstable version (https://issues.apache.org/jira/browse/KAFKA-17336)

new-client-to-old-broker: the builder of LIST_OFFSET will check the timestamp and its matched version (https://issues.apache.org/jira/browse/KAFKA-17331)

old-client-to-new-broker: the broker will check the timestamp and its matched version (https://issues.apache.org/jira/browse/KAFKA-17331)

I didn't realize ListOffset was being used in the replication path. I guess this is just for tiered storage.

@jolshan not really, the request is used to truncate in the replication path. I have attached a sample code in https://issues.apache.org/jira/browse/KAFKA-17336. we will complete it tomorrow.

[0]

kafka/core/src/test/scala/unit/kafka/utils/TestUtils.scala

Line 307 in 8d29bc1

props.put(ServerConfigs.UNSTABLE_FEATURE_VERSIONS_ENABLE_CONFIG, "true")

[1]

kafka/server-common/src/main/java/org/apache/kafka/server/common/MetadataVersion.java

Line 474 in 8d29bc1

public short listOffsetRequestVersion() {

Thanks. https://issues.apache.org/jira/browse/KAFKA-17336 should be good to not release with unstable versions that break api usage.

Also, it seems we create IT with unstable APIs always [0] and I guess that is why we don't see failed tests.

Thanks for the investigation, @chia7712. Maybe we just need to disable unstable API/MV in a few existing integration tests.

Maybe we just need to disable unstable API/MV in a few existing integration tests.

yep, we are looking for the similar tests

junrao · 2024-08-09T18:20:53Z

-    // TEST_2 released right before MV 3.9-IVO was released, and it depends on this metadata version
-    TEST_2(2, MetadataVersion.IBP_3_9_IV0, Collections.singletonMap(MetadataVersion.FEATURE_NAME, MetadataVersion.IBP_3_9_IV0.featureLevel()));
+    // TEST_2 released right before MV 4.0-IVO was released, and it depends on this metadata version
+    TEST_2(2, MetadataVersion.IBP_4_0_IV0, Collections.singletonMap(MetadataVersion.FEATURE_NAME, MetadataVersion.IBP_4_0_IV0.featureLevel()));


@jolshan : What MV should TEST_2 depend on? Do we need to keep changing it with new MVs? It would be useful to add a comment to document this.

I'm sorry I haven't gotten a chance to update this. I was hoping to have one of the test versions be the latest unstable version, but I believe I updated the tests for them to work either way now. Let me double check.

If so we can just keep at 3.9.

Ok. This is the test that will fail if we don't change the version to 4.0. https://github.com/apache/kafka/blame/98cdf9717049e87ba34bb5161276577fcb8bd1c4/server-common/src/test/java/org/apache/kafka/server/common/FeaturesTest.java#L137

We have a similar test here for real features, so if we think this is sufficient, we can remove the test I linked above. https://github.com/apache/kafka/blame/98cdf9717049e87ba34bb5161276577fcb8bd1c4/server-common/src/test/java/org/apache/kafka/server/common/FeaturesTest.java#L51

Thanks, Justine. Could we just use MetadataVersion.latestTesting() to avoid having to keep updating this with new MVs?

LatestTesting doesn't quite work since it is not guaranteed that latestTesting is unstable right?

Well, the only case when LatestTesting is stable is that there is no unstable MV, right?

Right -- in that case this test will fail. https://github.com/apache/kafka/blame/98cdf9717049e87ba34bb5161276577fcb8bd1c4/server-common/src/test/java/org/apache/kafka/server/common/FeaturesTest.java#L137

My understanding is that we don't create a new MV when we mark one as stable, so for example, if all the MVs were marked stable, the test would fail.

Alternatively, I could write the test to check if the latest MV is stable, and if so, skip the check.

We should probably just document that the last MV should always be unstable. I don't think it's ever been stable since "unstable" was created (we don't seem to stop developing features...)

Should we do this in a follow-on so that we can unblock 3.9?

junrao · 2024-08-09T18:27:08Z

     */
    @ParameterizedTest
-    @ValueSource(strings = {"3.4-IV0", "3.5-IV2", "3.6-IV0", "3.7-IV2", "3.9-IV1"})
+    @ValueSource(strings = {"3.4-IV0", "3.5-IV2", "3.6-IV0", "3.7-IV2", "4.0-IV0"})


I had a followup comment in #16347 (comment). Do we need to add 4.0-IV0 for testNoLeaderEpochBumpOnEmptyTargetIsr?

It would not make sense to add 4.0-IV0 to testNoLeaderEpochBumpOnEmptyTargetIsr, since in 4.0 an empty target ISR actually WILL trigger a leader epoch bump.

The reason is because once KIP-966 is implemented, empty ISRs will actually be possible, whereas they are not prior to KIP-966

junrao

@cmccabe : Thanks for the explanation. LGTM assuming we will have a followup PR to address #16841 (comment).

chia7712 · 2024-08-12T20:36:41Z

Thanks for the explanation. LGTM assuming we will have a followup PR to address #16841 (comment).

will file PR later (https://issues.apache.org/jira/browse/KAFKA-17319)

- Mark 3.9-IV0 as stable. Metadata version 3.9-IV0 should return Fetch version 17. - Move ELR to 4.0-IV0. Remove 3.9-IV1 since it's no longer needed. - Create a new 4.0-IV1 MV for KIP-848. Reviewers: Jun Rao <junrao@gmail.com>, Chia-Ping Tsai <chia7712@gmail.com>, Justine Olshan <jolshan@confluent.io>

…#16901) After some discussion on: #16841 (comment) We decided it is best for test version to always map to MetadataVersion.latestTesting. We should always have one unstable MV (either because there is a feature being worked on OR when we mark the latest version stable we create a new one). Reviewers: Jun Rao <junrao@apache.org>

KAFKA-17018: update MetadataVersion for the Kafka release 3.9

b6d843d

- Mark 3.9-IV0 as stable. Metadata version 3.9-IV0 should return Fetch version 17. - Move ELR to 4.0-IV0. Remove 3.9-IV1 since it's no longer needed. - Create a new 4.0-IV1 MV for KIP-848.

junrao reviewed Aug 9, 2024

View reviewed changes

junrao approved these changes Aug 12, 2024

View reviewed changes

cmccabe merged commit 132e097 into apache:trunk Aug 12, 2024

cmccabe deleted the KAFKA-17018 branch August 12, 2024 23:30

junrao mentioned this pull request Aug 13, 2024

KAFKA-17331: Throw unsupported version exception if the server does NOT support EarliestLocalSpec and LatestTieredSpec #16873

Merged

3 tasks

chia7712 mentioned this pull request Aug 15, 2024

KAFKA-17336 Add IT to make sure the production MV does not use unstable version of LIST_OFFSET #16893

Merged

3 tasks

jolshan mentioned this pull request Aug 16, 2024

KAFKA-17355: Update TestFeatureVersion 2 to always be based on MetadataVersion.latestTesting #16901

Merged

3 tasks

Conversation

cmccabe commented Aug 8, 2024

Uh oh!

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

Choose a reason for hiding this comment

Uh oh!

chia7712 Aug 12, 2024 • 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!

jolshan Aug 13, 2024 • 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!

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!

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!

junrao left a comment

Choose a reason for hiding this comment

Uh oh!

chia7712 commented Aug 12, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

chia7712 Aug 12, 2024 •

edited

Loading

jolshan Aug 13, 2024 •

edited

Loading