KAFKA-17492 skip features with minVersion of 0 instead of replacing 0 with 1 when BrokerRegistrationRequest < 4 by m1a2st · Pull Request #17128 · apache/kafka

m1a2st · 2024-09-08T15:37:47Z

Jira: https://issues.apache.org/jira/browse/KAFKA-17492

Due to the BrokerRegistrationRequest minVersion is setting from 0 to 1, thus ClusterControlManger#processRegistrationFeature check the version will throw an error, fix it by below suggestion

make 3.9+ broker be able to register to 3.8 controller
skip features with minVersion of 0 instead of replacing 0 with 1 when BrokerRegistrationRequest < 4

2. make 3.9+ controller be able to accept unknown features from future broker
skip check of unknown features. see following sample code

Committer Checklist (excluded from commit message)

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

chia7712 · 2024-09-08T17:27:15Z

make 3.9+ controller be able to accept unknown features from future broker

noticed that I had raised a question for it: #16421 (comment)

junrao

@m1a2st : Thanks for the PR. Left one comment.

junrao · 2024-09-09T16:56:02Z

-            } else {
-                return new BrokerRegistrationRequest(data, version);
-            }
+            return new BrokerRegistrationRequest(data, version);


We need to skip features with minSupportedVersion of 0 in v3 or less.

We need to do the same in ApiVersionResponse.

We need to change the comment like the following in ApiVersionResponse.json and BrokerRegistrationRequest.json.

Note: in v0-v3, features with MinSupportedVersion = 0 show up with MinSupportedVersion = 1.

for another: please update the following comment:

// Workaround for KAFKA-17011: for BrokerRegistrationRequest versions older than 4, // translate minSupportedVersion = 0 to minSupportedVersion = 1.

Sorry, I don't understand the change here. The correct change (that matches the PR description) would be to remove features that had a minVersion of 0 when handling older RPCs, right? But instead the code just seems to pass everything through, ignoring RPC version. What am I missing?

chia7712 · 2024-09-09T18:33:46Z

-                    ". The broker wants a version between " + feature.minSupportedVersion() + " and " +
-                    feature.maxSupportedVersion() + ", inclusive.");
-        }
+        finalizedFeatures.get(feature.name()).ifPresent(


@m1a2st please take a look at #16421 (comment). We don't need to remove the check of ClusterControlManager

cmccabe · 2024-09-09T18:34:46Z

-                    ". The broker wants a version between " + feature.minSupportedVersion() + " and " +
-                    feature.maxSupportedVersion() + ", inclusive.");
-        }
+        finalizedFeatures.get(feature.name()).ifPresent(


This change is incorrect. If the minVersion is 0, the feature must be set.

cmccabe · 2024-09-09T18:43:36Z

I pushed a commit which does what the PR description says (skip features with minVersion of 0)...

take a look

[edit: there are probably some unit tests that need to be fixed up]

chia7712

@cmccabe thanks for updating this PR. I will verify this patch on 3.9

chia7712 · 2024-09-09T18:58:03Z

                    BrokerRegistrationRequestData.Feature feature = iter.next();
                    if (feature.minSupportedVersion() == 0) {
-                        feature.setMinSupportedVersion((short) 1);
+                        iter.remove();


How about using removeIf?

newData.features().removeIf(feature -> feature.minSupportedVersion() == 0);

chia7712 · 2024-09-09T20:34:21Z

Run the transactions_upgrade_test.py, transactions_mixed_versions_test.py, and kraft_upgrade_test.py with this patch and #17084 on my local. all pass.

I will merge it if no failed tests.

cmccabe · 2024-09-09T23:47:42Z

I did some cleanup and added the removeIf simplification that was suggested

junrao

@cmccabe : Thanks for the updated PR. A couple of minor comments.

junrao · 2024-09-10T00:00:26Z

-                // at versions less than 4, we must set the minimum version for these features
-                // to 1 rather than 0. See KAFKA-17011 for details.
-                key.setMinVersion((short) 1);
+                // at versions less than 4, we must omit these features. See KAFKA-17492.


We need to change the comment like the following in ApiVersionResponse.json and BrokerRegistrationRequest.json.

Note: in v0-v3, features with MinSupportedVersion = 0 show up with MinSupportedVersion = 1.

@m1a2st Could you please address @junrao comment? That includes following files.

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

Line 50 in b22e848

"about": "Features supported by the broker. Note: in v0-v3, features with MinSupportedVersion = 0 show up with MinSupportedVersion = 1.",

https://github.com/apache/kafka/blob/trunk/clients/src/main/resources/common/message/BrokerRegistrationRequest.json#L51

junrao · 2024-09-10T00:00:58Z

    assertEquals(expectedMinVersion, key.minVersion())
  }
+
+  def assertFeatureMissing(


Could this be private?

m1a2st · 2024-09-10T01:19:33Z

Thanks for @cmccabe, @chia7712, @junrao helping for this issue, I will complete this PR

chia7712 · 2024-09-10T02:15:13Z

-                // at versions less than 4, we must set the minimum version for these features
-                // to 1 rather than 0. See KAFKA-17011 for details.
-                key.setMinVersion((short) 1);
+                // Note: For API versions v0-v3, features with MinSupportedVersion = 0 


This comment is not accurate. Could you consider using @cmccabe's comment?

// Some older clients will have deserialization problems if a feature's // minimum supported level is 0. Therefore, when preparing ApiVersionResponse // at versions less than 4, we must omit these features. See KAFKA-17492.

chia7712 · 2024-09-10T04:16:53Z

@m1a2st please add 3.8 version to following e2e:

https://github.com/apache/kafka/blob/trunk/tests/kafkatest/tests/core/zookeeper_migration_test.py#L89

Some older clients will have deserialization problems if a KIP-584 feature's minimum supported level is 0. Therefore, when preparing an ApiVersionResponse for a version less than 4, we must omit features with a minimum supported level of 0. This is reasonable to do because older software could not handle setting these features to a level other than 0 in any case. A similar problem applies to BrokerRegistrationRequest. Here we must omit features with minVersion = 0 when the RPC version is less than 4. Note that this fix supersedes an earlier compatibility preservation effort in KAFKA-17011.

chia7712 · 2024-09-10T08:22:00Z

    { "name":  "SupportedFeatures", "type": "[]SupportedFeatureKey", "ignorable": true,
      "versions":  "3+", "tag": 0, "taggedVersions": "3+",
-      "about": "Features supported by the broker. Note: in v0-v3, features with MinSupportedVersion = 0 show up with MinSupportedVersion = 1.",
+      "about": "Features supported by the broker. Note: in v0-v3, features blocked with MinSupportedVersion = 0.",


Pardon me, I don't catch "features blocked with MinSupportedVersion = 0."

Also, please fix BrokerRegistrationRequest.json too

Pardon me, I don't catch "features blocked with MinSupportedVersion = 0."

I change it to omit, it is more accurancy in this case

chia7712

@m1a2st thanks for your patch. two minor comments are left. PTAL

chia7712 · 2024-09-10T08:41:35Z

    },
    { "name": "Features", "type": "[]Feature",
-      "about": "The features on this broker. Note: in v0-v3, features with MinSupportedVersion = 0 show up with MinSupportedVersion = 1.", "versions": "0+", "fields": [
+      "about": "The features on this broker. Note: in v0-v3, features omit with MinSupportedVersion = 0.", "versions": "0+", "fields": [


How about "features with MinSupportedVersion = 0 are omitted"?

chia7712 · 2024-09-10T08:41:40Z

    { "name":  "SupportedFeatures", "type": "[]SupportedFeatureKey", "ignorable": true,
      "versions":  "3+", "tag": 0, "taggedVersions": "3+",
-      "about": "Features supported by the broker. Note: in v0-v3, features with MinSupportedVersion = 0 show up with MinSupportedVersion = 1.",
+      "about": "Features supported by the broker. Note: in v0-v3, features omit with MinSupportedVersion = 0.",


How about "features with MinSupportedVersion = 0 are omitted"?

chia7712 · 2024-09-10T15:37:29Z

The failed tests pass on my local, and following links are the related jira. The new e2e works on my local. I will merge it to trunk and 3.9 later if no objections.

https://issues.apache.org/jira/browse/KAFKA-17514
https://issues.apache.org/jira/browse/KAFKA-16174
https://issues.apache.org/jira/browse/KAFKA-15104
https://issues.apache.org/jira/browse/KAFKA-14533
https://issues.apache.org/jira/browse/KAFKA-17515
https://issues.apache.org/jira/browse/KAFKA-15772
https://issues.apache.org/jira/browse/KAFKA-15411
https://issues.apache.org/jira/browse/KAFKA-16501
https://issues.apache.org/jira/browse/KAFKA-15146
https://issues.apache.org/jira/browse/KAFKA-16377

cmccabe

LGTM. Thanks @m1a2st @junrao @chia7712

jolshan · 2024-09-10T16:29:23Z

Thanks folks for fixing this 🙏

junrao

@m1a2st : Thanks for the updated PR. Just a couple of minor comments.

junrao · 2024-09-10T16:30:52Z

    { "name":  "SupportedFeatures", "type": "[]SupportedFeatureKey", "ignorable": true,
      "versions":  "3+", "tag": 0, "taggedVersions": "3+",
-      "about": "Features supported by the broker. Note: in v0-v3, features with MinSupportedVersion = 0 show up with MinSupportedVersion = 1.",
+      "about": "Features supported by the broker. Note: in v0-v3, features MinSupportedVersion = 0 are omitted.",


features MinSupportedVersion = 0 are omitted => features with MinSupportedVersion = 0 are omitted

junrao · 2024-09-10T16:31:15Z

    },
    { "name": "Features", "type": "[]Feature",
-      "about": "The features on this broker. Note: in v0-v3, features with MinSupportedVersion = 0 show up with MinSupportedVersion = 1.", "versions": "0+", "fields": [
+      "about": "The features on this broker. Note: in v0-v3, features MinSupportedVersion = 0 are omitted.", "versions": "0+", "fields": [


features MinSupportedVersion = 0 are omitted => features with MinSupportedVersion = 0 are omitted

junrao

@m1a2st : Thanks for the updated PR. LGTM

chia7712 · 2024-09-10T17:08:12Z

The last commit does not impact source code, so I'm going to merge it to test full e2e with #17084

… with 1 when BrokerRegistrationRequest < 4 (#17128) The 3.8 controller assumes the unknown features have min version = 0, but KAFKA-17011 replace the min=0 by min=1 when BrokerRegistrationRequest < 4. Hence, to support upgrading from 3.8.0 to 3.9, this PR changes the implementation of ApiVersionsResponse (<4) and BrokerRegistrationRequest (<4) to skip features with supported minVersion of 0 instead of replacing 0 with 1 Reviewers: Jun Rao <junrao@gmail.com>, Colin P. McCabe <cmccabe@apache.org>, Chia-Ping Tsai <chia7712@gmail.com>

junrao · 2024-09-10T20:10:51Z

@m1a2st : Could we update https://cwiki.apache.org/confluence/display/KAFKA/KIP-1022%3A+Formatting+and+Updating+Features and the mailing list with the changes in this PR?

In order to resolve this dilemma, we will create a new version 4 for ApiVersionsRequest. Clients which send v4 promise to be able to handle ranges including 0. Clients which send v3 will never see a range that starts with 0. Such ranges will be translated into ranges starting with 1 instead.

Similarly, for BrokerRegistrationRequest, we will create a new version 4. Servers which accept v4 promise to be able to handle ranges including 0. Servers which do not will get ranges that only start with 1, rather than 0.

m1a2st · 2024-09-11T00:52:48Z

I will update this KIP https://cwiki.apache.org/confluence/display/KAFKA/KIP-1022%3A+Formatting+and+Updating+Features

… with 1 when BrokerRegistrationRequest < 4 (apache#17128) The 3.8 controller assumes the unknown features have min version = 0, but KAFKA-17011 replace the min=0 by min=1 when BrokerRegistrationRequest < 4. Hence, to support upgrading from 3.8.0 to 3.9, this PR changes the implementation of ApiVersionsResponse (<4) and BrokerRegistrationRequest (<4) to skip features with supported minVersion of 0 instead of replacing 0 with 1 Reviewers: Jun Rao <junrao@gmail.com>, Colin P. McCabe <cmccabe@apache.org>, Chia-Ping Tsai <chia7712@gmail.com>

m1a2st marked this pull request as draft September 9, 2024 04:59

junrao reviewed Sep 9, 2024

View reviewed changes

chia7712 mentioned this pull request Sep 9, 2024

KAFKA-17011: Fix a bug preventing features from supporting v0 #16421

Merged

chia7712 changed the title ~~KAFKA-17492: Can't register 3.9+ broker to 3.8 controller since 3.8 controller assume the default version of feature is zero~~ KAFKA-17492 skip features with minVersion of 0 instead of replacing 0 with 1 when BrokerRegistrationRequest < 4 Sep 9, 2024

chia7712 reviewed Sep 9, 2024

View reviewed changes

cmccabe reviewed Sep 9, 2024

View reviewed changes

chia7712 reviewed Sep 9, 2024

View reviewed changes

cmccabe force-pushed the gh-KAFKA-17492 branch from 0478cb3 to 201827f Compare September 9, 2024 23:45

junrao reviewed Sep 10, 2024

View reviewed changes

m1a2st marked this pull request as ready for review September 10, 2024 01:19

chia7712 reviewed Sep 10, 2024

View reviewed changes

chia7712 mentioned this pull request Sep 10, 2024

KAFKA-17497: Add e2e for zk migration with old controller #17131

Merged

3 tasks

m1a2st force-pushed the gh-KAFKA-17492 branch from a66f458 to b706bf2 Compare September 10, 2024 05:55

chia7712 reviewed Sep 10, 2024

View reviewed changes

addressed by comment

85bdd76

m1a2st force-pushed the gh-KAFKA-17492 branch from c2babe3 to 85bdd76 Compare September 10, 2024 08:44

chia7712 approved these changes Sep 10, 2024

View reviewed changes

cmccabe approved these changes Sep 10, 2024

View reviewed changes

junrao reviewed Sep 10, 2024

View reviewed changes

update by comments

81c14de

junrao approved these changes Sep 10, 2024

View reviewed changes

chia7712 merged commit e311716 into apache:trunk Sep 10, 2024

cmccabe mentioned this pull request Sep 10, 2024

KAFKA-17492: skip KIP-584 features with minVersion of 0 in older RPCs #17143

Closed

Conversation

m1a2st commented Sep 8, 2024 • edited by chia7712 Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Committer Checklist (excluded from commit message)

Uh oh!

chia7712 commented Sep 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

Choose a reason for hiding this comment

Uh oh!

cmccabe commented Sep 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chia7712 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chia7712 commented Sep 9, 2024

Uh oh!

cmccabe commented Sep 9, 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!

chia7712 Sep 10, 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!

m1a2st commented Sep 10, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chia7712 commented Sep 10, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

m1a2st Sep 10, 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!

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!

chia7712 commented Sep 10, 2024

Uh oh!

cmccabe left a comment

Choose a reason for hiding this comment

Uh oh!

jolshan commented Sep 10, 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!

junrao left a comment

Choose a reason for hiding this comment

Uh oh!

m1a2st commented Sep 8, 2024 •

edited by chia7712

Loading

chia7712 commented Sep 8, 2024 •

edited

Loading

cmccabe commented Sep 9, 2024 •

edited

Loading

chia7712 Sep 10, 2024 •

edited

Loading

m1a2st Sep 10, 2024 •

edited

Loading