Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

KAFKA-17011: SupportedFeatures.MinVersion incorrectly blocks v0 (3.8) #16420

Closed
wants to merge 5 commits into from
Closed

KAFKA-17011: SupportedFeatures.MinVersion incorrectly blocks v0 (3.8) #16420

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
1cd3967
Do not return 0 in apiversons response
jolshan Jun 21, 2024
38a8b07
Add unit test
jolshan Jun 21, 2024
6b897b4
Suppress 0 versions for older versions
jolshan Jun 22, 2024
58a9cfc
test fixes
jolshan Jun 24, 2024
1faf2ab
Add comment
jolshan Jun 25, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 13 additions & 1 deletion clients/src/main/java/org/apache/kafka/common/requests/ApiVersionsResponse.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@
import java.util.Map;
import java.util.Optional;
import java.util.Set;
import java.util.stream.Collectors;

/**
* Possible error codes:
Expand Down Expand Up @@ -258,11 +259,22 @@ private static ApiVersionsResponseData createApiVersionsResponseData(
final long finalizedFeaturesEpoch,
final boolean zkMigrationEnabled
) {
Features<SupportedVersionRange> backwardsCompatibleFeatures = Features.supportedFeatures(latestSupportedFeatures.features().entrySet()
.stream().filter(entry -> {
SupportedVersionRange supportedVersionRange = entry.getValue();
return supportedVersionRange.min() != 0;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure whether I fail to catch your description.

so for now we will change 0 to 1 in the response in order to be backwards compatible.

the code looks like it gets rid of "0" instead of changing to from 0 to 1.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For another, does this PR mean Admin#describeFeatures can't see the feature "group.version" from the broker running in 3.8.0?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey sorry, the comment was from the old implementation, the new way is to omit as you mentioned.
I believe describeFeatures will not be able to show the version for this release.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps it's useful to add a comment on why we are filtering version 0 features.

})
.collect(Collectors.toMap(
entry -> entry.getKey(),
entry -> entry.getValue()
))
);

final ApiVersionsResponseData data = new ApiVersionsResponseData();
data.setThrottleTimeMs(throttleTimeMs);
data.setErrorCode(error.code());
data.setApiKeys(apiKeys);
data.setSupportedFeatures(createSupportedFeatureKeys(latestSupportedFeatures));
data.setSupportedFeatures(createSupportedFeatureKeys(backwardsCompatibleFeatures));
data.setFinalizedFeatures(createFinalizedFeatureKeys(finalizedFeatures));
data.setFinalizedFeaturesEpoch(finalizedFeaturesEpoch);
data.setZkMigrationReady(zkMigrationEnabled);
Expand Down
20 changes: 20 additions & 0 deletions clients/src/test/java/org/apache/kafka/common/requests/ApiVersionsResponseTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -275,6 +275,26 @@ public void testIntersect() {
assertEquals(expected, ApiVersionsResponse.intersect(other, thisVersion).get());
}

@Test
public void testZeroVersionNotReturned() {
String featureName = "test.feature.version";
ApiVersionsResponse response = ApiVersionsResponse.createApiVersionsResponse(
10,
RecordVersion.V1,
Features.supportedFeatures(Collections.singletonMap(featureName, new SupportedVersionRange((short) 0, (short) 1))),
Collections.emptyMap(),
ApiVersionsResponse.UNKNOWN_FINALIZED_FEATURES_EPOCH,
null,
ListenerType.BROKER,
true,
false,
true
);

// Feature should not be in the supported features due to the 0 version.
assertEquals(null, response.data().supportedFeatures().find(featureName));
}

private void verifyVersions(short forwardableAPIKey,
short minVersion,
short maxVersion,
Expand Down