KIP 848 ListGroups API #4777

mahajanadhitya · 2024-07-05T09:52:32Z

No description provided.

- Mock handler implementation - Rename current consumer protocol from generic to classic - Mock handler with automatic or manual assignment - More consumer group metadata getters - Test helpers - Expedite next HB after FindCoordinator doing it with an exponential backoff to avoid tight loops - Configurable session timeout and HB interval - Fix mock handler ListOffsets response LeaderEpoch instead of CurrentLeaderEpoch - Integration tests passing with AK trunk - Improve documentation and KIP 848 specific mock tests - Add mock tests for unknown topic id in metadata request and partial reconciliation - Make test 0147 more reliable - Fix test 0106 after HB timeout change - Exclude test case with AK trunk - Rename rd_kafka_buf_write_tags to rd_kafka_buf_write_tags_empty - Trivup 0.12.5 can run a KafkaCluster directly with KRaft and AK trunk - Trivup 0.12.6 build with a specific commit

emasab

First comments, on second commit only, about implementation except tests and example

.semaphore/semaphore.yml

examples/alter_consumer_group_offsets.c

examples/consumer.c

src/rdkafka_mock.h

src/rdkafka_mock_handlers.c

src/rdkafka_admin.c

src/rdkafka.c

src/rdkafka_op.c

* [KIP-848] integration tests passing - Mock handler implementation - Rename current consumer protocol from generic to classic - Mock handler with automatic or manual assignment - More consumer group metadata getters - Test helpers - Expedite next HB after FindCoordinator doing it with an exponential backoff to avoid tight loops - Configurable session timeout and HB interval - Fix mock handler ListOffsets response LeaderEpoch instead of CurrentLeaderEpoch - Integration tests passing with AK trunk - Improve documentation and KIP 848 specific mock tests - Add mock tests for unknown topic id in metadata request and partial reconciliation - Make test 0147 more reliable - Fix test 0106 after HB timeout change - Exclude test case with AK trunk * Trivup 0.12.5 can run a KafkaCluster directly with KRaft and AK trunk * Trivup 0.12.6 build with a specific commit * rebase commit * Rebased * change * changes * changes * Style fix * PR comments * changes * minor * whitespace --------- Co-authored-by: Emanuele Sabellico <esabellico@confluent.io> Co-authored-by: Anchit Jain <anjain@confluent.io>

* [KIP-848] integration tests passing - Mock handler implementation - Rename current consumer protocol from generic to classic - Mock handler with automatic or manual assignment - More consumer group metadata getters - Test helpers - Expedite next HB after FindCoordinator doing it with an exponential backoff to avoid tight loops - Configurable session timeout and HB interval - Fix mock handler ListOffsets response LeaderEpoch instead of CurrentLeaderEpoch - Integration tests passing with AK trunk - Improve documentation and KIP 848 specific mock tests - Add mock tests for unknown topic id in metadata request and partial reconciliation - Make test 0147 more reliable - Fix test 0106 after HB timeout change - Exclude test case with AK trunk * Trivup 0.12.5 can run a KafkaCluster directly with KRaft and AK trunk * Trivup 0.12.6 build with a specific commit * rebase commit * Rebased * change * changes * changes * Style fix * PR comments * changes * minor * whitespace * pull and push changes --------- Co-authored-by: Emanuele Sabellico <esabellico@confluent.io> Co-authored-by: Anchit Jain <anjain@confluent.io>

emasab

Second pass about tests and example

.semaphore/semaphore.yml

src/rdkafka.c

src/rdkafka_op.c

tests/test.c

emasab · 2024-08-09T08:17:25Z

tests/0081-admin.c

+                rd_kafka_consumer_group_type_t consumer_type =
+                    RD_KAFKA_CONSUMER_GROUP_TYPE_CONSUMER;


Same: when test_consumer_group_protocol_classic() it should search for CONSUMER, othewise CLASSIC

Yes, these are true but since the test is similar for other clients which do not have has_match_types functionality like in librdkafka, so only these were agreed upon. Otherwise, we would diverge a lot !

I agree with @emasab. This test will fail when the consumer is created with CLASSIC protocol. Please fix this.

tests/0081-admin.c

emasab · 2024-08-09T08:52:09Z

examples/list_consumer_groups.c

@@ -296,7 +313,7 @@ int main(int argc, char **argv) {
        /*
         * Parse common options
         */
-        while ((opt = getopt(argc, argv, "b:X:d:")) != -1) {
+        while ((opt = getopt(argc, argv, "b:X:d")) != -1) {


Remove this change as d parameter requires an arg

Update the usage message with state count and group types arguments

examples/list_consumer_groups.c

tests/test.c

src/rdkafka_op.c

pranavrth

First pass without tests

pranavrth · 2024-08-15T22:08:13Z

src/rdkafka_admin.h

@@ -445,6 +451,7 @@ struct rd_kafka_ConsumerGroupListing_s {
        /** Is it a simple consumer group? That means empty protocol_type. */
        rd_bool_t is_simple_consumer_group;
        rd_kafka_consumer_group_state_t state; /**< Consumer group state. */
+        rd_kafka_consumer_group_type_t group_type; /**< Consumer group type. */


I would name it type instead of group_type

Actually this is to maintain parity with the convention such as rk_type etc.. Also in other clients such as go type is a keyword.

This is type in java as well. Please change it.

examples/list_consumer_groups.c

src/rdkafka.h

src/rdkafka_admin.c

src/rdkafka_request.c

pranavrth · 2024-08-16T09:40:54Z

Build is failing

mahajanadhitya · 2024-08-16T10:39:01Z

Build is failing

Do you have the warnings flag on, if so the warnings are not for my changes atleast. The build runs fine for me.

pranavrth · 2024-08-19T11:04:35Z

I am talking about Semaphore build in the PR.

pranavrth

Please fix the failing tests in the build as well.

tests/0080-admin_ut.c

src/rdkafka.h

pranavrth · 2024-08-19T12:28:46Z

examples/list_consumer_groups.c

@@ -184,24 +187,30 @@ int64_t parse_int(const char *what, const char *str) {
 static void
 cmd_list_consumer_groups(rd_kafka_conf_t *conf, int argc, char **argv) {


Running list_consumer_groups without any argument is causing Segmentation Fault. It should show usage instead. Fix this.

src/rdkafka_admin.c

src/rdkafka_request.c

pranavrth · 2024-08-19T23:23:39Z

tests/0081-admin.c

+                while (1) {
+                        rkev = rd_kafka_queue_poll(q, tmout_multip(20 * 1000));
+                        TEST_SAY("ListConsumerGroups: got %s in %.3fms\n",
+                                 rd_kafka_event_name(rkev),
+                                 TIMING_DURATION(&timing) / 1000.0f);
+                        if (rkev == NULL)
+                                continue;
+                        if (rd_kafka_event_error(rkev))
+                                TEST_SAY("%s: %s\n", rd_kafka_event_name(rkev),
+                                         rd_kafka_event_error_string(rkev));
+
+                        if (rd_kafka_event_type(rkev) ==
+                            RD_KAFKA_EVENT_LISTCONSUMERGROUPS_RESULT) {
+                                break;
+                        }
+
+                        rd_kafka_event_destroy(rkev);
+                }


Extract out this part to a separate function. This is repeated below as well.

pranavrth · 2024-08-19T23:25:43Z

tests/0081-admin.c

+        if (has_match_types) {
+                /* With the options of the consumer protocol we should get 0
+                 * valid cnt */
+                rd_kafka_AdminOptions_t *optionwithconsumer =


option_with_consumer. Same at other places as well.

pranavrth · 2024-08-19T23:32:03Z

tests/0081-admin.c

+quickexit:
+        rd_kafka_queue_destroy(mainq);
+
+        rd_kafka_destroy(rk);


This is not even used. Remove this change.

pranavrth · 2024-08-19T23:32:26Z

tests/0081-admin.c

@@ -2825,6 +3035,72 @@ static void do_test_ListConsumerGroups(const char *what,

        rd_kafka_event_destroy(rkev);

+        if (has_match_types) {


What are we testing in this block?

pranavrth · 2024-08-19T23:42:29Z

tests/0081-admin.c

+                rd_kafka_consumer_group_type_t consumer_type =
+                    RD_KAFKA_CONSUMER_GROUP_TYPE_CONSUMER;


I agree with @emasab. This test will fail when the consumer is created with CLASSIC protocol. Please fix this.

pranavrth · 2024-08-19T23:51:00Z

tests/0081-admin.c

I don't understand why we need alot of new code to test only the newly added field type. It should have been very simple. @emasab was also suggesting the same thing in another comment.

if broker version doesn't support type, then we should have UNKNOWN type. else if (test_consumer_group_protocol_consumer()) type should be CONSUMER else type should be CLASSIC.

Even if we need this much code, there is alot of duplication that can be optimized.

Let's discuss this over a call about these test changes.

pranavrth · 2024-08-19T23:53:12Z

Apart from this the test 0081 is crashing in my local as well when running with broker v3.7.0 with classic protocol.

mahajanadhitya · 2024-09-02T16:09:13Z

Librdkafka KIP 848 ListGroups API
Branch Name : feature/ListGroupsAPI
Example : examples/list_consumer_groups

Running the example : ./list_consumer_groups -b localhost:9092 state_cnt_int type_cnt_int [ state_1_int state_2_int ] [ type_1_int type_2_int ]
Types can only be Consumer(1), Classic(2). Any other type int would give error as Unknown(0) is not accepted and others do not exist
Types only populates the options for the request which MIGHT NOT BE USED by broker, depending on the version used.

Running the integration test(do_test_ListConsumerGroups) : TESTS=0081 make

Running the unit tests(do_test_ListConsumerGroups) : TESTS=0080 make

Any warnings via configure, make or make install are not subject to my changes.

Formatting done via,
$ make style-fix

Integration Testing done with :

Apache Kafka Server version 3.8 (with/without(unset) TEST_CONSUMER_GROUP_PROTOCOL=consumer/classic)
Apache Kafka Server version 3.7
$ TESTS=0081 make

If any test fail, it is either because any consumer_group did not get destroy and had its information rolled over in the next spawn, or the Apache Kafka Version (trunk) instead of 3.8 or 3.7 ! I can discuss the same in the office hours as well !

pranavrth

Some minor comments. Checking more.

tests/0081-admin.c

pranavrth · 2024-09-10T08:33:15Z

tests/0081-admin.c


+        /* match_types would not work if the broker version is below 3.8.0.0 */
+        if (has_match_types && match_types)
+                match_types = rd_true;


Don't reuse the same variable.

pranavrth · 2024-09-10T08:34:44Z

tests/0081-admin.c

+                } else {
+                        TEST_SAY(
+                            "'TEST_CONSUMER_GROUP_PROTOCOL' Environment "
+                            "variable not set properly, cane be consumer or "
+                            "classic\n");
+                        match_types = rd_false;
+                }


This is not possible. Remove this.

pranavrth · 2024-09-10T08:54:04Z

tests/0081-admin.c

+                test_ListConsumerGroups_helper(
+                    rk, option_group_protocol_not_in_use, q,
+                    list_consumer_groups, TEST_LIST_CONSUMER_GROUPS_CNT, 0,
+                    group_protocol_not_in_use, rd_false);


use has_match_states here as well.

pranavrth · 2024-09-10T13:06:54Z

src/rdkafka_admin.h

@@ -445,6 +451,7 @@ struct rd_kafka_ConsumerGroupListing_s {
        /** Is it a simple consumer group? That means empty protocol_type. */
        rd_bool_t is_simple_consumer_group;
        rd_kafka_consumer_group_state_t state; /**< Consumer group state. */
+        rd_kafka_consumer_group_type_t group_type; /**< Consumer group type. */


This is type in java as well. Please change it.

examples/list_consumer_groups.c

src/rdkafka_admin.c

pranavrth

Some more minor comments.

examples/list_consumer_groups.c

src/rdkafka.h

src/rdkafka_admin.c

pranavrth · 2024-09-12T07:43:55Z

src/rdkafka_admin.c

+                            "Only a known group type is allowed, UNKNOWN Group "
+                            "Type is not allowed");


Just say "UNKNOWN group type is not allowed."

src/rdkafka_admin.c

confluent-cla-assistant · 2024-10-03T08:21:00Z

All contributors need to sign the Contributor License Agreement here before this PR can be approved.
❌ mahajanadhitya
_{Please push an empty commit if you would like to re-run the checks to verify CLA status for all contributors.}

pranavrth

Code and example part seems fine. Just a few minor comments.

pranavrth · 2024-10-03T08:52:32Z

src/rdkafka.h

+rd_kafka_consumer_group_type_name(rd_kafka_consumer_group_type_t type);
+
+/**
+ * @brief Returns a code for a group type name.


I think we should specify that we compare ignoring the case.

examples/consumer.c

src/rdkafka_admin.c

pranavrth · 2024-11-05T18:24:07Z

Closed with #4860

mahajanadhitya requested a review from a team as a code owner July 5, 2024 09:52

airlock-confluentinc bot force-pushed the feature/ListGroupsAPI branch from 3b45306 to aa99639 Compare August 7, 2024 09:10

[KIP-848] ListGroups v5 with group type filter

9deaa8b

emasab changed the base branch from master to dev_kip848_mock_handler_and_integration_tests August 8, 2024 09:57

emasab changed the base branch from dev_kip848_mock_handler_and_integration_tests to master August 8, 2024 09:57

emasab requested changes Aug 8, 2024

View reviewed changes

mahajanadhitya changed the base branch from master to dev_kip848_mock_handler_and_integration_tests August 8, 2024 18:42

mahajanadhitya and others added 3 commits August 9, 2024 09:17

Addressed first round of comments

e98a232

emasab requested changes Aug 9, 2024

View reviewed changes

tests/test.c Outdated Show resolved Hide resolved

src/rdkafka_op.c Outdated Show resolved Hide resolved

PR Comments Change 2

24f248a

pranavrth requested changes Aug 16, 2024

View reviewed changes

PR Changes 3 (#18)

67f1043

pranavrth requested changes Aug 19, 2024

View reviewed changes

PR changes 4 (#23)

94037a0

mahajanadhitya added 2 commits September 6, 2024 13:16

Refactor Changes

46c37fb

Doxygen Error Fix

6ba0894

pranavrth requested changes Sep 10, 2024

View reviewed changes

pranavrth requested changes Sep 12, 2024

View reviewed changes

Address remaining comments

ad1655b

pranavrth requested changes Oct 3, 2024

View reviewed changes

airlock-confluentinc bot force-pushed the dev_kip848_mock_handler_and_integration_tests branch from aa99639 to 19ab07f Compare October 3, 2024 19:00

airlock-confluentinc bot force-pushed the dev_kip848_mock_handler_and_integration_tests branch from 19ab07f to 3d70ed0 Compare October 18, 2024 08:28

airlock-confluentinc bot force-pushed the dev_kip848_mock_handler_and_integration_tests branch from 53ea4ba to 50c4130 Compare October 28, 2024 20:24

pranavrth closed this Nov 5, 2024

		rd_kafka_consumer_group_type_t consumer_type =
		RD_KAFKA_CONSUMER_GROUP_TYPE_CONSUMER;

		@@ -184,24 +187,30 @@ int64_t parse_int(const char what, const char str) {
		static void
		cmd_list_consumer_groups(rd_kafka_conf_t conf, int argc, char *argv) {

		@@ -2825,6 +3035,72 @@ static void do_test_ListConsumerGroups(const char *what,

		rd_kafka_event_destroy(rkev);

		if (has_match_types) {

		"Only a known group type is allowed, UNKNOWN Group "
		"Type is not allowed");

KIP 848 ListGroups API #4777

KIP 848 ListGroups API #4777

Conversation

mahajanadhitya commented Jul 5, 2024

emasab left a comment

Choose a reason for hiding this comment

emasab left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pranavrth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pranavrth commented Aug 16, 2024

mahajanadhitya commented Aug 16, 2024

pranavrth commented Aug 19, 2024

pranavrth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pranavrth commented Aug 19, 2024

mahajanadhitya commented Sep 2, 2024

pranavrth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pranavrth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

confluent-cla-assistant bot commented Oct 3, 2024

pranavrth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pranavrth commented Nov 5, 2024