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.

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

KAFKA-17050: Revert group.version #16482

Open
wants to merge 6 commits into
base: trunk
Choose a base branch
from
Open

Conversation

dajac
Copy link
Contributor

@dajac dajac commented Jun 28, 2024

This patch partially reverts group.version in trunk. I kept the GroupVersion class but removed it from Features so it is not advertised. I also kept all the changes in the test framework. I removed the logic to require group.version=1 to enable the new consumer rebalance protocol. The new protocol is enabled based on the static configuration.

For the context, I prefer to revert it in trunk now so we don't forget to revert it in the 3.9 release. I will bring it back for the 4.0 release.

Committer Checklist (excluded from commit message)

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

@dajac dajac added the KIP-848 label Jun 28, 2024
Copy link
Contributor

@chia7712 chia7712 left a comment

Choose a reason for hiding this comment

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

@@ -74,7 +74,6 @@ abstract class AbstractApiVersionsRequestTest(cluster: ClusterInstance) {
assertEquals(MetadataVersion.latestTesting().featureLevel(), apiVersionsResponse.data().supportedFeatures().find(MetadataVersion.FEATURE_NAME).maxVersion())

assertEquals(0, apiVersionsResponse.data().supportedFeatures().find(GroupVersion.FEATURE_NAME).minVersion())
Copy link
Contributor

Choose a reason for hiding this comment

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

this should be removed too. Also, the expected value line#72 should be changed to 1 as group.version is reverted

@dajac
Copy link
Contributor Author

dajac commented Jul 1, 2024

@chia7712 Could you please take a second look?

@chia7712
Copy link
Contributor

chia7712 commented Jul 1, 2024

@dajac could you please fix the build error

@dajac
Copy link
Contributor Author

dajac commented Jul 1, 2024

@chia7712 Done. Sorry for that.

@chia7712
Copy link
Contributor

chia7712 commented Jul 1, 2024

I guess testConsumerGroupDescribeIsInaccessibleWhenDisabledByGroupVersion needs to be reverted also.

https://github.com/apache/kafka/blob/trunk/core/src/test/scala/unit/kafka/server/ConsumerGroupDescribeRequestsTest.scala#L75

@dajac
Copy link
Contributor Author

dajac commented Jul 1, 2024

@chia7712 Right. I just removed it.

Copy link
Contributor

@chia7712 chia7712 left a comment

Choose a reason for hiding this comment

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

LGTM if no related error :)

@dajac
Copy link
Contributor Author

dajac commented Jul 1, 2024

There are a few failed tests that I need to fix…

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
2 participants