librdkafka icon indicating copy to clipboard operation
librdkafka copied to clipboard

KIP-396: Implement AlterConsumerGroupOffsets and ListConsumerGroupOffsets admin APIs

Open lesterfan opened this issue 4 years ago • 2 comments

This is a very rough implementation of KIP-396. I tried to follow the Java implementation and have the alterConsumerGroupOffsets() API call just send an OffsetCommitRequest and the listConsumerGroupOffsets() API call just send an OffsetFetchRequest. I did run into some issues, and I thought it would be good to just share this rough (but working) patch here with these caveats and get some feedback:

  • There already is code in librdkafka to send an OffsetCommitRequst, parse an OffsetCommitResponse, send an OffsetFetchRequest, and parse an OffsetFetchResponse. It would be ideal to re-use them for these admin API calls. However, these function's interfaces expect an rd_kafka_broker_t * and other parameters which we don't have access to in the admin API. I couldn't figure out how to refactor the code to re-use these functions and ended up copy-pasting a large part of the implementation of these functions in these admin API calls, leading to significant code duplication.
  • In the OffsetFetchRequest, there is an option to set topics to NULL if we want to fetch offsets for all topics. This is a useful feature to expose in the admin API, and I thought it would make sense to have a NULL rd_kafka_topic_partition_list* correspond to having a NULL list of topics for this request. However, many helper functions like rd_kafka_topic_partition_list_copy don't account for cases where the toppar list is NULL, so I added a check there. This widens the contract of these helper functions so I wasn't sure if it was the correct way of going about this.

lesterfan avatar Jun 02 '21 17:06 lesterfan

Hi @lesterfan. Thanks for your contribution! Sorry for letting you wait so much. I'm taking your commit and adding some changes on top of that to use the same request serializers and response deserializers and to support the require_stable admin option. I'm also completing implementation of KIP-222 as this PR has listConsumerGroupOffsets that belongs to KIP-222 and KIP-88 that is necessary for KIP-222. I've moved your commit to this branch as we have some new builds that don't have to run on forked repos. If you have some questions or want to send a PR please send it to that branch.

emasab avatar Aug 08 '22 23:08 emasab

@emasab Thank you for building on top of this work! I'm happy to help however I can with the caveat that I am no longer working on the product which is using this patch. Also, cc @hallfox and @mlongob who worked on this with me and may be interested.

lesterfan avatar Aug 09 '22 04:08 lesterfan

This has been included in #3995, thanks!

emasab avatar Jan 04 '23 12:01 emasab