KAFKA-17011: Fix a bug preventing features from supporting v0 by cmccabe · Pull Request #16421 · apache/kafka

cmccabe · 2024-06-21T22:17:28Z

As part of KIP-584, brokers expose a range of supported versions for each feature. For example, metadata.version might be supported from 1 to 21. (Note that feature level ranges are always inclusive, so this would include both level 1 and 21.)

These supported ranges are supposed to be able to include 0. For example, it should be possible for a broker to support a kraft.version between 0 and 1. However, in older software versions, there is an assertion in org.apache.kafka.common.feature.SupportedVersionRange that prevents this. This causes problems when the older software attempts to deserialize an ApiVersionsResponse containing such a range.

In order to resolve this dilemma, this PR bumps the version of ApiVersionsRequest from 3 to 4. Clients which send v4 promise to be able to handle ranges including 0. Clients which send v3 will not be exposed to these ranges. The feature will show up as having a minimum version of 1 instead. This work is part
of KIP-1022.

Similarly, this PR also introduces a new version of BrokerRegistrationRequest, and specifies that the
older versions of that RPC cannot handle supported version ranges including 0. Therefore, 0 is translated to 1 in the older requests.

As part of KIP-584, brokers expose a range of supported versions for each feature. For example, metadata.version might be supported from 1 to 21. (Note that feature level ranges are always inclusive, so this would include both level 1 and 21.) These supported ranges are supposed to be able to include 0. For example, it should be possible for a broker to support a kraft.version between 0 and 1. However, in older software versions, there is an assertion in org.apache.kafka.common.feature.SupportedVersionRange that prevents this. This causes problems when the older software attempts to deserialize an ApiVersionsResponse containing such a range. In order to resolve this dilemma, this PR bumps the version of ApiVersionsRequest from 3 to 4. Clients which send v4 promise to be able to handle ranges including 0. Clients which send v3 will not be exposed to these ranges -- the feature will simply be omitted from the response. This work is part of KIP-1022.

jolshan · 2024-06-24T20:51:46Z

Did we check the kraft upgrade system tests? I can kick off a job. Looks like there may also be some issue with the build.

junrao

@cmccabe : Thanks for the PR. Left a minor comment.

cmccabe · 2024-06-25T00:05:50Z

It seems simpler to just pass the ApiVersionsResponse version as a parameter to the Supplier (hence making it a Function, I guess)

Updated.

jolshan · 2024-06-25T15:47:08Z

Let's take a look at testUnsupportedApiVersionsRequestWithVersionProvidedByTheBroker

cmccabe · 2024-06-25T17:57:29Z

It looks like the test was checking the ApiVersionsRequest header version against 3. So it needed to be updated to 4

cmccabe · 2024-06-26T20:23:21Z

The test failures pass locally.

junrao

@cmccabe : Thanks for the updated PR. Just a few minor comments.

junrao

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

Also, I asked a followup question in the mailing list on whether it's better to suppress V0 features completely or just exclude V0 from such features. It would be useful to reach consensus on that.

junrao

@cmccabe : Thanks for the updated PR. The code LGTM. There was still some discussion in the mailing list for KIP-1022 on the best approach for addressing this issue. We probably need to reach some consensus there first.

Also, are the following 2 test failures related?
Build / JDK 8 and Scala 2.12 / kafka.admin.ListOffsetsIntegrationTest."testThreeRecordsInOneBatchHavingDifferentCompressionTypeWithServer(String).quorum=kraft"
Build / JDK 8 and Scala 2.12 / org.apache.kafka.tools.GetOffsetShellTest.testTopicPartitionsArgWithInternalIncluded [2] Type=Raft-Isolated, MetadataVersion=4.0-IV0,Security=PLAINTEXT

jolshan · 2024-07-03T17:29:17Z

#16183 will wait for this PR in order to not break tests.

chia7712

@cmccabe thanks for this nice fix. some comments are left. PTAL

chia7712 · 2024-07-08T19:32:52Z

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


MinSupportedVersion -> MinVersion

Note: in v0-v3, features with MinSupportedVersion = 0 must be left out.

This needs to be adjusted accordingly since we now keep the feature but returns 1 as the minSupportedVersion.

This needs to be adjusted accordingly since we now keep the feature but returns 1 as the minSupportedVersion.

done

chia7712 · 2024-07-08T20:10:43Z

+  def testSendV4ApiVersionsRequest(quorum: String): Unit = {
+    val response = sendApiVersionsRequest(4)
+    if (quorum.equals("kraft")) {
+      assertFeatureHasMinVersion("group.version", response.data().supportedFeatures(), 0)


group.version is already reverted by #16482

jolshan · 2024-07-08T20:23:00Z

-            return new BrokerRegistrationRequest(data, version);
+            if (version < 4) {
+                // Workaround for KAFKA-17011: for BrokerRegistrationRequest versions older than 4,
+                // exclude support version ranges that begin with 0.


I think we still need to finalize/agree here about omission vs. using 1 for older versions of the api.

Right now we have omit for broker registration, but using version 1 for ApiVersions. We also have the option to use 1 for both or omit for both. Note: In 3.8, we omit for both requests.

I think there is some benefit in not including a version with 0 to older controllers rather than 1, but can also understand that it is confusing to have different approaches for different APIs.

@junrao I know you had some opinions here.

It seems that it's simpler for the old BrokerRegistrationRequest to be consistent with the older version of ApiVersionResponse, i.e., to include the feature but set the minSupportedVersion to 1, instead of 0.

OK. Just like in ApiVersionsResponse, we will translate min version = 0 to min version = 1 here.

junrao

@cmccabe : Thanks for the updated PR. A few more comments.

junrao · 2024-07-08T21:34:32Z

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


Note: in v0-v3, features with MinSupportedVersion = 0 must be left out.

This needs to be adjusted accordingly since we now keep the feature but returns 1 as the minSupportedVersion.

junrao · 2024-07-08T21:35:39Z

    },
    { "name": "Features", "type": "[]Feature",
-      "about": "The features on this broker", "versions": "0+", "fields": [
+      "about": "The features on this broker. Note: in v0-v3, features with MinSupportedVersion = 0 must be left out.", "versions": "0+", "fields": [


Note: in v0-v3, features with MinSupportedVersion = 0 must be left out.

This needs to be adjusted accordingly since we now keep the feature but returns 1 as the minSupportedVersion.

junrao · 2024-07-08T21:48:56Z

-            return new BrokerRegistrationRequest(data, version);
+            if (version < 4) {
+                // Workaround for KAFKA-17011: for BrokerRegistrationRequest versions older than 4,
+                // exclude support version ranges that begin with 0.


It seems that it's simpler for the old BrokerRegistrationRequest to be consistent with the older version of ApiVersionResponse, i.e., to include the feature but set the minSupportedVersion to 1, instead of 0.

junrao

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

junrao · 2024-07-09T19:27:24Z

            }))
    }
+    if (unstableFeatureVersionsEnabled) {
+      features.put("kraft.version", new SupportedVersionRange(1))


Should we add kraft.version to PRODUCTION_FEATURES, which is supposed to be the place that tracks all features?

I will do that in a follow-on PR.

I'm a little confused here. This has one argument? And is kraft.version part of this PR?

I'm a little confused here. This has one argument?

There's a constructor that creates a range 0-N based on just being given N. I will change it to be the two-argument constructor since I guess that's confusing.

And is kraft.version part of this PR?

I want to have it here for test purposes. I will create it for real in a follow-on PR, if this PR ever concludes.

I guess I was confused because I didn't see it in the file.

kafka/clients/src/main/java/org/apache/kafka/clients/admin/SupportedVersionRange.java

Line 38 in 0b11971

SupportedVersionRange(final short minVersion, final short maxVersion) {

but 🤷‍♀️

junrao · 2024-07-09T19:30:36Z

+        private Map<String, Short> finalizedFeatures = null;
+        private long finalizedFeaturesEpoch = 0;
+        private boolean zkMigrationEnabled = false;
+        private boolean suppressFeatureLevel0 = false;


suppressFeatureLevel0 => alterFeatureLevel0 ?

junrao · 2024-07-09T19:31:06Z

+            return this;
+        }
+
+        public Builder setSuppressFeatureLevel0(boolean suppressFeatureLevel0) {


suppressFeatureLevel0 => alterFeatureLevel0 ?

junrao · 2024-07-09T19:31:46Z

-        final boolean zkMigrationEnabled
+    private static SupportedFeatureKeyCollection maybeFilterSupportedFeatureKeys(
+        Features<SupportedVersionRange> latestSupportedFeatures,
+        boolean suppressV0


suppressV0 => alterV0 ?

junrao · 2024-07-09T19:32:33Z


+    @ParameterizedTest
+    @ValueSource(booleans = {false, true})
+    public void testSuppressV0Features(boolean suppressV0Features) {


testSuppressV0Features => testAlterV0Features ?
suppressV0Features => alterV0Features ?

junrao · 2024-07-09T19:33:53Z

  def enabledApis: collection.Set[ApiKeys]

-  def apiVersionResponse(throttleTimeMs: Int): ApiVersionsResponse
+  def apiVersionResponse(throttleTimeMs: Int, suppressFeatureLevel0: Boolean): ApiVersionsResponse


suppressFeatureLevel0 => alterFeatureLevel0 ?

junrao · 2024-07-09T19:34:09Z

-  override def apiVersionResponse(throttleTimeMs: Int): ApiVersionsResponse = {
+  override def apiVersionResponse(
+    throttleTimeMs: Int,
+    suppressFeatureLevel0: Boolean


suppressFeatureLevel0 => alterFeatureLevel0 ?

junrao · 2024-07-09T19:34:22Z

-    val supportedFeatures = brokerFeatures.supportedFeatures
+  override def apiVersionResponse(
+    throttleTimeMs: Int,
+    suppressFeatureLevel0: Boolean


suppressFeatureLevel0 => alterFeatureLevel0 ?

jolshan · 2024-07-10T16:07:16Z

Looks like there are still quite a few failures for ApiVersionsRequestTest and BrokerFeaturesTest

junrao

@cmccabe : Thanks for the updated PR. Added one question below. Also, there were a bunch of ApiVersionsRequestTest failures.

junrao · 2024-07-10T16:30:33Z


        // Change expected message to reflect latest MetadataVersion (SupportedMaxVersion increases when adding a new version)
+        assertEquals("Feature: kraft.version\tSupportedMinVersion: 0\t" +
+                "SupportedMaxVersion: 1\tFinalizedVersionLevel: 0\t", outputWithoutEpoch(features.get(0)));


How does this test work? Do we explicitly set the FinalizedVersionLevel for kraft.version to 0 somewhere?

Do we explicitly set the FinalizedVersionLevel for kraft.version to 0 somewhere?

anything not enabled is at level 0

cmccabe · 2024-07-10T16:56:43Z

The context here is that we’re adding kraft.version in 3.9. But not in this PR. I did add kraft.version to supportedFeatures, simply so that I could verify that the RPC results looked like what I wanted. This will cause no compatibility problems since there is no way to set kraft.version yet.

jolshan

lgtm as long as tests pass. (For the followup that adds kraft.version, let's try to follow the conventions for new features so we don't hardcode everything 👍 )

junrao

@cmccabe : Thanks for the updated PR. LGTM assuming the tests pass.

jolshan · 2024-07-10T23:08:10Z

Test failures are unrelated. I will merge.

…#16421) As part of KIP-584, brokers expose a range of supported versions for each feature. For example, metadata.version might be supported from 1 to 21. (Note that feature level ranges are always inclusive, so this would include both level 1 and 21.) These supported ranges are supposed to be able to include 0. For example, it should be possible for a broker to support a kraft.version between 0 and 1. However, in older software versions, there is an assertion in org.apache.kafka.common.feature.SupportedVersionRange that prevents this. This causes problems when the older software attempts to deserialize an ApiVersionsResponse containing such a range. In order to resolve this dilemma, this PR bumps the version of ApiVersionsRequest from 3 to 4. Clients which send v4 promise to be able to handle ranges including 0. Clients which send v3 will not be exposed to these ranges. The feature will show up as having a minimum version of 1 instead. This work is part of KIP-1022. Similarly, this PR also introduces a new version of BrokerRegistrationRequest, and specifies that the older versions of that RPC cannot handle supported version ranges including 0. Therefore, 0 is translated to 1 in the older requests. Reviewers: Jun Rao <junrao@apache.org>, Chia-Ping Tsai <chia7712@gmail.com>, Justine Olshan <jolshan@confluent.io>

chia7712 · 2024-09-05T11:34:09Z

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


There is another issue caused by changing the version from 0 to 1. #15685 add the check which assumes the "default version" is 0

https://github.com/apache/kafka/blob/3.8/metadata/src/main/java/org/apache/kafka/controller/ClusterControlManager.java#L470

Hence, the features having min=1 can NOT pass the check. see following error message:

[2024-09-05 11:22:22,146] INFO [QuorumController id=2] registerBroker: event failed with UnsupportedVersionException in 127 microseconds. Exception message: Unable to register because the broker does not support version 0 of group.version. It wants a version between 1 and 1, inclusive. (org.apache.kafka.controller.QuorumController)

@cmccabe @junrao @dajac FYI

@chia7712 Thanks. Is there a way to reproduce this (e.g. an existing test)?

Is there a way to reproduce this (e.g. an existing test)?

run the following command with #17084

TC_PATHS="tests/kafkatest/tests/core/kraft_upgrade_test.py::TestKRaftUpgrade.test_combined_mode_upgrade" \ /bin/bash tests/docker/run_tests.sh

Originally I thought that we could not set version 0 with the storage or update tools since it would treat it as disabling the feature. But @dajac pointed out to me that this is happening by setting the MV and that picks default features.

However, the min version 1 should only be the case for older request versions. I thought we fixed this to so we wouldn't have the requirement for version 4 and for 4.0 where group version is introduced.

When I last talked to Colin about this, I believe he said we could not disable the feature if the min version is > 0.

When I last talked to Colin about this, I believe he said we could not disable the feature if the min version is > 0.

@cmccabe : Could you confirm if this is case? How would people upgrade from an old release where a feature is not turned on?

I would suspect you have to upgrade the feature before the upgrade to the new release, but we can let Colin confirm.

Checked with Colin offline. To summarize, if we do increase minVersion in the future, we need to bump up the default version for that feature. An old release may need to first upgrade to a bridge release where the new default version could be set. Given that, we don't need to change the logic in ClusterControlManager.

Given that, we don't need to change the logic in ClusterControlManager.

I will sync it to KAFKA-17429 and #17128

cmccabe force-pushed the KAFKA-17011 branch from a874288 to 1c1e556 Compare June 21, 2024 23:24

jolshan reviewed Jun 21, 2024

View reviewed changes

Comment thread core/src/main/scala/kafka/network/SocketServer.scala Outdated

junrao reviewed Jun 24, 2024

View reviewed changes

Comment thread core/src/main/scala/kafka/network/SocketServer.scala Outdated

pass through the ApiVersionsResponse version to ChannelBuilders

6235178

Update NetworkClientTest

1dc5f99

junrao reviewed Jun 26, 2024

View reviewed changes

cmccabe added 3 commits June 27, 2024 14:11

Merge branch 'trunk' into KAFKA-17011

c0e3c7c

Add BrokerRegistrationRequest v4

3e07a3b

Address review comments

ab1b566

junrao reviewed Jun 27, 2024

View reviewed changes

Comment thread clients/src/main/resources/common/message/BrokerRegistrationRequest.json

Comment thread clients/src/test/java/org/apache/kafka/common/requests/BrokerRegistrationRequestTest.java Outdated

address review feedback

234ea4b

junrao reviewed Jul 2, 2024

View reviewed changes

cmccabe added 2 commits July 8, 2024 10:39

Merge branch 'trunk' into KAFKA-17011

04ecd25

change approach to KAFKA-17011

890c184

chia7712 reviewed Jul 8, 2024

View reviewed changes

jolshan reviewed Jul 8, 2024

View reviewed changes

junrao reviewed Jul 8, 2024

View reviewed changes

cmccabe added 2 commits July 9, 2024 12:12

fixes

6ad6749

Merge branch 'trunk' into KAFKA-17011

c1ba1f5

junrao reviewed Jul 9, 2024

View reviewed changes

cmccabe added 2 commits July 9, 2024 15:44

address comments

85a827a

fix tests

d6eb186

junrao reviewed Jul 10, 2024

View reviewed changes

fix tests etc.

f4bd3cc

jolshan approved these changes Jul 10, 2024

View reviewed changes

chia7712 approved these changes Jul 10, 2024

View reviewed changes

junrao approved these changes Jul 10, 2024

View reviewed changes

jolshan merged commit ede289d into apache:trunk Jul 10, 2024

chia7712 reviewed Sep 5, 2024

View reviewed changes

This was referenced Sep 8, 2024

KAFKA-17492 skip features with minVersion of 0 instead of replacing 0 with 1 when BrokerRegistrationRequest < 4 #17128

Merged

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

Merged

cmccabe deleted the KAFKA-17011 branch September 9, 2024 18:31

BenPope mentioned this pull request May 7, 2025

[CORE-9876] kafka: Support ApiVersions v4 redpanda-data/redpanda#26057

Merged

7 tasks

Conversation

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

Uh oh!

Uh oh!

Uh oh!

jolshan commented Jun 24, 2024

Uh oh!

junrao left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

cmccabe commented Jun 25, 2024

Uh oh!

jolshan commented Jun 25, 2024

Uh oh!

cmccabe commented Jun 25, 2024

Uh oh!

cmccabe commented Jun 26, 2024

Uh oh!

junrao left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

junrao left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

junrao left a comment

Choose a reason for hiding this comment

Uh oh!

jolshan commented Jul 3, 2024

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!

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!

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!

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

cmccabe commented Jun 21, 2024 •

edited

Loading

cmccabe Jul 10, 2024 •

edited

Loading

chia7712 Sep 5, 2024 •

edited

Loading