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

Merged
merged 7 commits into from
Jul 2, 2024
Merged

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 The Next Generation of the Consumer Rebalance Protocol 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…

@dajac dajac merged commit e7a7580 into apache:trunk Jul 2, 2024
1 check failed
@dajac dajac deleted the KAFKA-17050-trunk branch July 2, 2024 14:21
chirag-wadhwa5 pushed a commit to chirag-wadhwa5/kafka that referenced this pull request Jul 3, 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.

Reviewers: Chia-Ping Tsai <chia7712@gmail.com>
abhi-ksolves pushed a commit to ksolves/kafka that referenced this pull request Jul 31, 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.

Reviewers: Chia-Ping Tsai <chia7712@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
KIP-848 The Next Generation of the Consumer Rebalance Protocol
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants