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

[admin] Add consumer group bindings for KIP-88, 222, 518, 396 (partial) #857

Closed

Conversation

milindl
Copy link
Contributor

@milindl milindl commented Sep 9, 2022

Currently in a draft state because of the fact that librdkafka implementation isn't fully done, and so this should not be merged.
Tested in conjunction with https://github.com/edenhill/librdkafka/tree/feature/list-and-alter-offsets (and depends on this branch).

This includes #852 (by @vsantwana) - the DeleteGroups implementation.
The rest of the bindings are - ListConsumerGroups, DescribeConsumerGroups, AlterConsumerGroupOffsets, and ListConsumerGroupOffsets.

I have some doubts about how I've implemented the options for list/describe consumer groups, so any feedback would be helpful for that

@milindl milindl requested a review from a team September 9, 2022 06:44
@milindl milindl force-pushed the feature/list-and-alter-offsets branch from af98bbe to 742930f Compare September 22, 2022 09:36
@milindl milindl changed the title [admin] Add ListConsumerGroupOffsets, AlterConsumerGroupOffsets and DeleteGroups bindings [admin] Add consumer group bindings for KIP-222/396 Sep 22, 2022
@milindl milindl force-pushed the feature/list-and-alter-offsets branch from 742930f to 59e4ba7 Compare September 22, 2022 09:46
@milindl milindl changed the title [admin] Add consumer group bindings for KIP-222/396 [admin] Add consumer group bindings for KIP-88, 222, 518, 396 (partial) Oct 3, 2022
@milindl milindl force-pushed the feature/list-and-alter-offsets branch from 299c2df to cd3057d Compare December 5, 2022 07:15
@milindl milindl force-pushed the feature/list-and-alter-offsets branch 2 times, most recently from 16bc999 to 48839ff Compare December 5, 2022 08:12
@milindl milindl force-pushed the feature/list-and-alter-offsets branch from 48839ff to 52e8fc4 Compare December 5, 2022 08:13
@milindl milindl force-pushed the feature/list-and-alter-offsets branch from ee98ebe to 2c96638 Compare December 8, 2022 10:21
kafka/adminapi.go Outdated Show resolved Hide resolved
kafka/adminapi.go Outdated Show resolved Hide resolved
kafka/adminapi.go Outdated Show resolved Hide resolved
kafka/adminoptions.go Outdated Show resolved Hide resolved
kafka/adminoptions.go Outdated Show resolved Hide resolved
kafka/adminapi.go Outdated Show resolved Hide resolved
kafka/adminapi_test.go Outdated Show resolved Hide resolved
@milindl milindl force-pushed the feature/list-and-alter-offsets branch from b3dcdad to 5b58e8c Compare December 15, 2022 08:50
@milindl
Copy link
Contributor Author

milindl commented Dec 15, 2022

Addressed most changes, except a few doubts left as comment replies.

Copy link
Contributor

@emasab emasab left a comment

Choose a reason for hiding this comment

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

Good job @milindl ! I'm approving this when we rebase this on the new version.

Also changes the tests so we don't end up deleting the topic specified in
testconf.json.
kafka/kafka.go Outdated
}

// GroupTopicPartitions represents a consumer group's TopicPartitions.
type GroupTopicPartitions struct {
Copy link
Contributor

Choose a reason for hiding this comment

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

We could name it ConsumerGroupTopicPartitions to keep naming consistent with method name, as Pranav is doing in the Python PR. The Java class has the same name but it's not used by the client.

ConsumerGroupTopicPartitions, add DescribeConsumerGroupsResult
@emasab
Copy link
Contributor

emasab commented Jan 13, 2023

Moved to #923, using an internal branch for CI

@emasab emasab closed this Jan 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants