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.

Sign up for GitHub

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

Jump to bottom

KIP 848 ListGroups API #2245

Open

mahajanadhitya wants to merge 6 commits into dev_fix_formatting from feature/ListGroupsAPI

Contributor

mahajanadhitya commented Jul 5, 2024

No description provided.

mahajanadhitya requested review from a team as code owners

July 5, 2024 09:05

airlock-confluentinc bot force-pushed the feature/ListGroupsAPI branch from 8e1d351 to 385429a Compare

August 7, 2024 09:07


          ListGroups API KIP 848 (#7)

* rebase commit

* change

anchitj requested changes

View reviewed changes

src/Confluent.Kafka/Impl/NativeMethods/NativeMethods.cs Outdated

@@ @@ -742,7 +748,7 @@ long least_significant_bits @@
                               /* rd_kafka_ConfigEntry_t * */ IntPtr entry);
                       [DllImport(DllName, CallingConvention = CallingConvention.Cdecl)]
-                      internal static extern IntPtr rd_kafka_ConfigEntry_is_synonym (
+                      internal static extern IntPtr rd_kafka_ConfigEntry_is_synonym(

Member

anchitj Aug 12, 2024 •

edited

Loading

Can we remove the changes done by style fix at places which we don't intend to touch? It's unnecessarily increasing the diff size making it difficult to review the PR

mahajanadhitya added 2 commits

August 19, 2024 14:42


          ListGroups API KIP 848 (#7)

532fca2

* rebase commit

* change


          Merge branch 'airlock/feature/ListGroupsAPI' into feature/ListGroupsAPI

ef1cf95

airlock-confluentinc bot force-pushed the feature/ListGroupsAPI branch from 8452969 to 395328b Compare

August 19, 2024 09:43


          Merge pull request #12 from confluentinc/feature/ListGroupsAPI

d3996f8

feature/ListGroupsAPI

mahajanadhitya changed the base branch from master to dev_fix_formatting

August 19, 2024 09:54

anchitj requested changes

View reviewed changes

Member

anchitj left a comment

Please also make the CI pass, and confirm that you've manually tested the code locally.

src/Confluent.Kafka/Impl/NativeMethods/NativeMethods.cs Outdated

Comment on lines 567 to 578

+                      [DllImport(DllName, CallingConvention = CallingConvention.Cdecl)]
+                      internal static extern IntPtr rd_kafka_AdminOptions_set_match_consumer_group_types(
+                                      IntPtr options,
+                                      ConsumerGroupType[] groupTypes,
+                                      UIntPtr groupTypesCnt);
+                      [DllImport(DllName, CallingConvention = CallingConvention.Cdecl)]
+                      internal static extern IntPtr rd_kafka_AdminOptions_set_match_consumer_group_types(
+                                      IntPtr options,
+                                      ConsumerGroupType[] groupTypes,
+                                      UIntPtr groupTypesCnt);

Member

anchitj Aug 20, 2024

Why is it defined two times? This is causing CI failure? Same in other NativeMethods files

Contributor Author

mahajanadhitya Aug 21, 2024

Actually these were due to rebasing of the "squash & merge" commits and these did not raise any merge conflict as well. So after local build/test and format, the last rebase messed these up. Would take care for next time !

test/Confluent.Kafka.IntegrationTests/Tests/AdminClient_ListDescribeConsumerGroups.cs Outdated

Comment on lines 66 to 67

		var groupID = Guid.NewGuid().ToString();
		var nonExistentGroupID = Guid.NewGuid().ToString();

Member

anchitj Aug 20, 2024

Renamed to groupId and nonExistentGroupId

Contributor Author

mahajanadhitya Aug 21, 2024

ok

test/Confluent.Kafka.IntegrationTests/Tests/AdminClient_ListDescribeConsumerGroups.cs

+                                  BootstrapServers = bootstrapServers
+                              }).Build())
+                              {
+                                  var listOptionsWithTimeout = new Admin.ListConsumerGroupsOptions() { RequestTimeout = TimeSpan.FromSeconds(30) };

Member

anchitj Aug 20, 2024

Not being used

test/Confluent.Kafka.IntegrationTests/Tests/AdminClient_ListDescribeConsumerGroups.cs Outdated

-                              LogToFile("KIP 848 Admin operations changes still aren't " +
-                                        "available");
+                              LogToFile("start AdminClient_ListDescribeConsumerGroups with Consumer Protocol");
+                              const string clientID = "test.client";

Member

anchitj Aug 20, 2024

clientId. Name variables using camelCase

Contributor Author

mahajanadhitya Aug 21, 2024

since it was id only then kept d capital as it is a short form. But will reflect this in the change.

test/Confluent.Kafka.IntegrationTests/Tests/AdminClient_ListDescribeConsumerGroups.cs

                           if (!TestConsumerGroupProtocol.IsClassic())
                           {
-                              LogToFile("KIP 848 Admin operations changes still aren't " +

Member

anchitj Aug 20, 2024

Why was this removed?

Contributor Author

mahajanadhitya Aug 21, 2024

Because these will be operational once the PR goes in and even in preview mode if unsafe api kafka server setting is set, then these are operational.

test/Confluent.Kafka.IntegrationTests/Tests/AdminClient_ListDescribeConsumerGroups.cs Outdated

@@ @@ -63,16 +63,68 @@ private void checkConsumerGroupDescription( @@
                       [Theory, MemberData(nameof(KafkaParameters))]
                       public void AdminClient_ListDescribeConsumerGroups(string bootstrapServers)
                       {
+                          var groupID = Guid.NewGuid().ToString();

Member

anchitj Aug 20, 2024

Please add another similar test creating Classic group

Contributor Author

mahajanadhitya Aug 21, 2024

Actually the way 848 integration testing work is that we run 2 instances of Semaphore CI where one version runs for all the test having no broker min version, so assume only classic group protocol is used.
Now currently semaphore will only run some parts of test that needs verifying which means with broker version > 3.8.0.0 and environment variable for the consumer group protocol chosen to be consumer type.

examples/AdminClient/Program.cs Outdated

+                          var groupTypesList = new List<ConsumerGroupType>();
+                          var isType = false;
+                          var isState = false;
+                          for (int i = 0; i < commandArgs.Length; i++)

Member

anchitj Aug 20, 2024

Use foreach like foreach (var t in commandArgs)

Contributor Author

mahajanadhitya Aug 21, 2024

ok

examples/AdminClient/Program.cs

+                                  }
+                                  else
+                                  {
+                                      Console.WriteLine("usage: .. <bootstrapServers> list-consumer-groups [-states <match_state_1> <match_state_2> ... <match_state_N>] [-types <group_type_1> .. <group_type_M>]");

Member

anchitj Aug 20, 2024

Missing <timeout_seconds>

Contributor Author

mahajanadhitya Aug 21, 2024

None of the usage in codebase mentions timeout seconds.

Contributor Author

mahajanadhitya Aug 21, 2024

For the examples convention I am saying

Member

anchitj Aug 27, 2024 •

edited

Loading

See the remove line 602(just below this comment), it had < timeout_seconds>

examples/AdminClient/Program.cs

-                                  timeout = TimeSpan.FromSeconds(Int32.Parse(commandArgs[0]));
+                                  if (isState)
+                                  {
+                                      Console.WriteLine("usage: .. <bootstrapServers> list-consumer-groups [-states <match_state_1> <match_state_2> ... <match_state_N>] [-types <group_type_1> .. <group_type_M>]");

Member

anchitj Aug 20, 2024

Improve the error message, state that this was a Duplicate states argument. Same for types

Contributor Author

mahajanadhitya Aug 21, 2024

No the duplicate state is Invalid args but still the example will run and yield a error like Invalid args.
This is basically error with exit code so user has put in wrong input format for the example.
like -state_cnt=2 state_1 -group_type_cnt=..
The above will error because only 1 state was given.

Contributor Author

mahajanadhitya Aug 21, 2024

Please suggest something if you feel it is still not sufficient.


          In-sync PR Changes

35d67c2

Contributor Author

mahajanadhitya commented Sep 10, 2024

confluent-kafka-dotnet KIP 848 ListGroups API
Branch Name : feature/ListGroupsAPI
Example : confluent-kafka-dotnet/examples/AdminClient/Program.cs

Running the example : dotnet run <bootstrapServers> list-consumer-groups [-states <match_state_1> <match_state_2> ... <match_state_N>] [-types <group_type_1> .. <group_type_M>]
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.

Make sure to install the Librdkafka feature/ListGroupsAPI and making the dotnet build with the local librdkafka

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

Formatting done via,
$ dotnet format

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

Integration Testing done via :
Consumer
$ export TEST_CONSUMER_GROUP_PROTOCOL=consumer
$ dotnet test --filter "FullyQualifiedName=Confluent.Kafka.IntegrationTests.Tests.AdminClient_ListDescribeConsumerGroups"
Classic
$ export TEST_CONSUMER_GROUP_PROTOCOL=classic
$ dotnet test --filter "FullyQualifiedName=Confluent.Kafka.IntegrationTests.Tests.AdminClient_ListDescribeConsumerGroups"


          Merge pull request #34 from confluentinc/feature/ListGroupsAPI

e274123

KIP 848 ListGroups API for the support of New Consumer Group Type 'Consumer'

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet