[KIP-848 EA] Admin API for listing consumer groups now has #1267

mahajanadhitya · 2024-08-09T08:51:02Z

an optional filter to return only groups of given types

milindl

Thanks for the changes! Looks mostly okay.

Remember to add it to CHANGELOG.md also

kafka/adminapi_test.go

milindl · 2024-08-13T04:03:20Z

kafka/adminapi_test.go

+
+	listres, err := a.ListConsumerGroups(
+		ctx, SetAdminRequestTimeout(time.Second),
+		SetAdminMatchConsumerGroupTypes([]ConsumerGroupType{classicGroupType, classicGroupType}))


Suggested change

SetAdminMatchConsumerGroupTypes([]ConsumerGroupType{classicGroupType, classicGroupType}))

SetAdminMatchConsumerGroupTypes([]ConsumerGroupType{classicGroupType, classicGroupType}))

Why is the same group type included twice?

It is to test the INVALID_ARGS error code with duplicate group types.

But your test is not checking for invalid args error code below, so change that. You're checking for deadline exceeded still.

kafka/build_darwin_amd64.go

kafka/integration_test.go

milindl · 2024-08-13T04:09:23Z

kafka/integration_test.go

+		// Generating a new topic/groupID to ensure a fresh group/topic is created.
+		rand.Seed(time.Now().Unix())
+		groupID := fmt.Sprintf("%s-%d", testconf.GroupID, rand.Int())
+		topic := fmt.Sprintf("%s-%d", testconf.TopicName, rand.Int())
+		nonExistentGroupID := fmt.Sprintf("%s-nonexistent-%d", testconf.GroupID, rand.Int())


Move above the "if" block, and remove from below too, so they're not duplicated

Same for creation of adminclient, topic creation, etc.

Actually this is done voluntarily because currently in semaphore we run the whole tests with broker version < 3.8.0.0 and then when we run with higher broker version on semaphore to test the consumer group protocol we only check the case of consumer group that is why we return at the end.

Okay, so since there is nothing common between the tests, why don't you split them? I don't see why we should have an entirely different test in an if block if there's any shared code between them, it just makes it harder to read.

TestAdminClient_ListAndDescribeConsumerGroupsClassic and TestAdminClient_ListConsumerGroupsConsumer

milindl · 2024-08-21T03:14:15Z

kafka/adminapi_test.go

+	ctx, cancel := context.WithTimeout(context.Background(), expDuration)
+	defer cancel()
+
+  listres, err := a.ListConsumerGroups(


gofmt needs to be run

milindl · 2024-08-21T03:15:49Z

kafka/adminapi_test.go

@@ -464,7 +467,10 @@ func testAdminAPIsListConsumerGroups(
 		t.Fatalf("Expected ConsumerGroupTypeFromString to work for Unknown type")
 	}

-	listres, err := a.ListConsumerGroups(
+	ctx, cancel := context.WithTimeout(context.Background(), expDuration)


I think this code doesn't run. Are the tests running when you are dynamically linking librdkafka? This is a compilation error because := and not =

milindl · 2024-08-21T03:16:54Z

kafka/adminapi_test.go

+
+	listres, err := a.ListConsumerGroups(
+		ctx, SetAdminRequestTimeout(time.Second),
+		SetAdminMatchConsumerGroupTypes([]ConsumerGroupType{classicGroupType, classicGroupType}))


But your test is not checking for invalid args error code below, so change that. You're checking for deadline exceeded still.

milindl · 2024-08-21T03:21:11Z

kafka/integration_test.go

+		// Generating a new topic/groupID to ensure a fresh group/topic is created.
+		rand.Seed(time.Now().Unix())
+		groupID := fmt.Sprintf("%s-%d", testconf.GroupID, rand.Int())
+		topic := fmt.Sprintf("%s-%d", testconf.TopicName, rand.Int())
+		nonExistentGroupID := fmt.Sprintf("%s-nonexistent-%d", testconf.GroupID, rand.Int())


Okay, so since there is nothing common between the tests, why don't you split them? I don't see why we should have an entirely different test in an if block if there's any shared code between them, it just makes it harder to read.

TestAdminClient_ListAndDescribeConsumerGroupsClassic and TestAdminClient_ListConsumerGroupsConsumer

milindl

Some more comments, please run the tests

mahajanadhitya · 2024-09-06T08:36:39Z

confluent-kafka-go KIP 848 ListGroups API

Branch Name

feature/ListGroupsAPI

Build

Install the corresponding Librdkafka and build with go build -tags dynamic ./...

Unit Tests

confluent-kafka-go/kafka/adminapi_test.go via testAdminAPIsListConsumerGroups
go test -tags dynamic -timeout 30s -run ^TestAdminAPIs

Cluster

Spawn the trivup cluster in librdkafka/tests
./interactive_broker_version.py 'trunk@f6c9feea76d01a46319b0ca602d70aa855057b07' --port 9092 --kraft --conf '{"conf":["unstable.api.versions.enable=true","group.coordinator.new.enable=true","group.protocol=classic,consumer"]}'

Example

confluent-kafka-go/examples/admin_list_consumer_groups/admin_list_consumer_groups.go
go run -tags dynamic admin_list_consumer_groups.go localhost:9092 [-states <state1> <state2> ...] [-types <type1> <type2> ...]

Integration Tests

Remember to set the Environment Variable TEST_CONSUMER_GROUP_PROTOCOL to consumer/classic
confluent-kafka-go/kafka/integration_test.go via TestAdminClient_ListConsumerGroups
go test -v -tags dynamic -run ^TestIntegration -testify.m TestAdminClient_ListConsumerGroups

confluent-cla-assistant · 2024-10-09T18:03:01Z

🎉 All Contributor License Agreements have been signed. Ready to merge.
_{Please push an empty commit if you would like to re-run the checks to verify CLA status for all contributors.}

emasab · 2024-10-09T18:44:03Z

Converting to draft until we merge v2.6.0-RC2

Milind is OOO

and tests Co-authored-by: mahajanadhitya <[email protected]>

pranavrth

All doubts cleared over a call.
LGTM!.

mahajanadhitya requested review from a team as code owners August 9, 2024 08:51

milindl reviewed Aug 13, 2024

View reviewed changes

milindl reviewed Aug 21, 2024

View reviewed changes

milindl previously requested changes Aug 21, 2024

View reviewed changes

airlock-confluentinc bot force-pushed the feature/ListGroupsAPI branch from db9f908 to 1734ea3 Compare October 9, 2024 18:02

emasab changed the title ~~ListGroups API KIP 848~~ [KIP-848 EA] Admin API for listing consumer groups now has Oct 9, 2024

emasab marked this pull request as draft October 9, 2024 18:43

airlock-confluentinc bot force-pushed the feature/ListGroupsAPI branch from b7f17bf to 3a31587 Compare October 10, 2024 13:57

emasab marked this pull request as ready for review October 10, 2024 13:57

emasab and others added 3 commits October 10, 2024 19:04

Reviewed implementation

dce1182

and tests Co-authored-by: mahajanadhitya <[email protected]>

Address comment

c7fa7e2

Use command line parser

69b9e96

airlock-confluentinc bot force-pushed the feature/ListGroupsAPI branch from 3a31587 to 69b9e96 Compare October 10, 2024 17:05

pranavrth approved these changes Oct 10, 2024

View reviewed changes

emasab merged commit 654a47b into master Oct 10, 2024
3 checks passed

emasab deleted the feature/ListGroupsAPI branch October 10, 2024 19:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[KIP-848 EA] Admin API for listing consumer groups now has #1267

[KIP-848 EA] Admin API for listing consumer groups now has #1267

mahajanadhitya commented Aug 9, 2024 •

edited by emasab

Loading

milindl left a comment

milindl Aug 13, 2024

mahajanadhitya Aug 16, 2024

milindl Aug 21, 2024 •

edited

Loading

milindl Aug 13, 2024

milindl Aug 13, 2024

mahajanadhitya Aug 16, 2024

milindl Aug 21, 2024

milindl Aug 21, 2024

milindl Aug 21, 2024

milindl Aug 21, 2024 •

edited

Loading

milindl Aug 21, 2024

milindl left a comment

mahajanadhitya commented Sep 6, 2024

confluent-cla-assistant bot commented Oct 9, 2024

emasab commented Oct 9, 2024

pranavrth left a comment

	SetAdminMatchConsumerGroupTypes([]ConsumerGroupType{classicGroupType, classicGroupType}))
	SetAdminMatchConsumerGroupTypes([]ConsumerGroupType{classicGroupType, classicGroupType}))

[KIP-848 EA] Admin API for listing consumer groups now has #1267

[KIP-848 EA] Admin API for listing consumer groups now has #1267

Conversation

mahajanadhitya commented Aug 9, 2024 • edited by emasab Loading

milindl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

milindl Aug 21, 2024 • edited Loading

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

milindl Aug 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

milindl left a comment

Choose a reason for hiding this comment

mahajanadhitya commented Sep 6, 2024

confluent-kafka-go KIP 848 ListGroups API

Branch Name

Build

Unit Tests

Cluster

Example

Integration Tests

confluent-cla-assistant bot commented Oct 9, 2024

emasab commented Oct 9, 2024

pranavrth left a comment

Choose a reason for hiding this comment

mahajanadhitya commented Aug 9, 2024 •

edited by emasab

Loading

milindl Aug 21, 2024 •

edited

Loading

milindl Aug 21, 2024 •

edited

Loading