Feature delete consumergroups #2040

swenzel · 2020-04-24T15:56:20Z

I didn't find any issue requesting this, but I could use it so I just went ahead and implemented it. It's kind of a work in progress, just wanted to get it out for early feedback.
Tests are running fine and it looks good but what's still missing is testing if group coordinator discovery works properly when there are different brokers.
Also I'm not quite sure how to handle errors. Right now they are ignored and it's left for the caller to inspect the result but we could also just raise any errors.
WDYT?

This change is

jeffwidman · 2020-04-24T17:18:44Z

kafka/admin/client.py

@@ -341,6 +343,30 @@ def _find_coordinator_id(self, group_id):
        response = future.value
        return self._find_coordinator_id_process_response(response)

+    def _find_many_coordinator_ids(self, group_ids):


Does Java have this method? When I wrote a bunch of this code two years ago I don't recall seeing anything that handled multiple group_ids at once, but I haven't been spelunking in their code for a while...

I don't know, this one is mostly copy pasted from the method above. I went through https://kafka.apache.org/protocol but it doesn't look like there is an API that can provide the coordinator id for more than one group. Not even the DescribeGroups API returns the coordinators.

Looking at this more, I certainly think it'd be convenient to have this method... we could also use it within the describe_consumer_groups() method to emit all the group coordinator requests in parallel: #2124

swenzel · 2020-05-03T13:27:07Z

@jeffwidman I ran this on a cluster with 4 nodes and it deleted 3k groups without an issue... I suppose coordinator discovery works well.
Considering how much work is required on the test suite to make the tests use multiple brokers I'd just leave it as it is now unless you really want me to extend the tests.
Any opinion on how to handle failed deletes? Raise exception vs. leave it for the caller to check?

swenzel · 2020-06-20T12:40:02Z

ping @dpkp @jeffwidman

kafka/admin/client.py

jeffwidman · 2020-09-17T06:52:32Z

test/test_admin_integration.py

+    consumergroups = {group_id for group_id, _ in kafka_admin_client.list_consumer_groups()}
+    assert "test2-group1" not in consumergroups
+    assert "test2-group2" in consumergroups
+    assert "test2-group3" not in consumergroups


missing newline at end

Fixed. Btw. an editorconfig and/or pre-commit config could help with this 😉

Good call.. PR welcome: #2125

jeffwidman · 2020-09-17T06:58:16Z

kafka/admin/client.py

+            results.extend(self._convert_delete_groups_response(f.value))
+        return results
+
+    def _convert_delete_groups_response(self, response):


This is outside the scope of this PR, but this client code is a little inconsistent currently between _convert_X(response) and _X_process_response(response)... it'd be nice to make the naming more consistent... but I took a quick look and it's IMO not worth fixing as how they are used varies slightly... somethings are converting things into requests, some are parsing responses, and some are doing pre/post conversion of one type into another...

Yeah, I also thought about this one a while... In the end my reasoning was something like this: the convert methods seem to have little to no logic and really do only conversions. The process methods involve more computation. So I took the former naming.

jeffwidman · 2020-09-17T06:59:55Z

kafka/admin/client.py

@@ -341,6 +343,30 @@ def _find_coordinator_id(self, group_id):
        response = future.value
        return self._find_coordinator_id_process_response(response)

+    def _find_many_coordinator_ids(self, group_ids):


Looking at this more, I certainly think it'd be convenient to have this method... we could also use it within the describe_consumer_groups() method to emit all the group coordinator requests in parallel: #2124

jeffwidman · 2020-09-17T07:01:51Z

Considering how much work is required on the test suite to make the tests use multiple brokers I'd just leave it as it is now unless you really want me to extend the tests.

I am fine with leaving as-is... agreed that doing multiple brokers is a lot of extra work. It's probably better done as a pytest fixture that spins up multiple brokers and multiple consumer groups that are guaranteed to have their coordinators spread across the brokers, as we could leverage that for several unit tests...

jeffwidman · 2020-09-17T07:11:04Z

Any opinion on how to handle failed deletes? Raise exception vs. leave it for the caller to check?

That's really tough... in list_consumer_groups() and describe_consumer_groups() we immediately raise if any group throws an error... but those are non-destructive operations, so re-running them is fine.

Here I'm hesitant to raise since everything is sent async so some groups may complete and others may fail... it is idempotent to simply re-run, but if some will always fail with errors like "group unknown/ doesn't exist" etc, then you want to know what happened to the others in the list... did they delete successfully? So I'd probably lean (slightly) toward just returning the error codes and letting the caller inspect to see which completed and which failed.

jeffwidman · 2020-09-17T07:15:12Z

@swenzel can you rebase to fix conflicts?

Take a look at my feedback above... the comment and newline are nits but if you're already rebasing be nice to fix these.

Update fork

jeffwidman · 2020-09-17T16:16:41Z

Thanks @swenzel!

* Add consumergroup related errors * Add DeleteGroups to protocol.admin * Implement delete_groups feature on KafkaAdminClient

jeffwidman reviewed Apr 24, 2020

View reviewed changes

swenzel marked this pull request as draft April 27, 2020 09:27

swenzel marked this pull request as ready for review May 3, 2020 13:30

jeffwidman reviewed Sep 17, 2020

View reviewed changes

kafka/admin/client.py Outdated Show resolved Hide resolved

jeffwidman approved these changes Sep 17, 2020

View reviewed changes

jeffwidman mentioned this pull request Sep 17, 2020

Switch describe_consumer_groups() to use _find_many_coordinator_ids() #2124

Closed

swenzel and others added 12 commits September 17, 2020 14:47

Merge pull request #1 from dpkp/master

f31b57c

Update fork

create tests for delete_consumer_groups command

a91e8dd

improve tests

64253aa

add consumergroup related errors

4b76d41

add DeleteGroups to protocol.admin

804faf4

implemente delete_groups feature on KafkaAdminClient

16cd36a

fix indentation of test code

792cf2b

fix delete_consumer_groups docstring

fa8af0c

fix indentation and use unique consumergroup ids for tests

11d8020

add newline at end of test_admin_integration.py

12f0a00

remove outdated note in admin/client.py

52bde48

use random group ids in test_admin_integration.py

2f678ae

swenzel force-pushed the feature-delete-consumergroups branch from 6d78133 to 2f678ae Compare September 17, 2020 13:13

jeffwidman mentioned this pull request Sep 17, 2020

Add a .editorconfig file #2125

Open

jeffwidman merged commit 16a0b31 into dpkp:master Sep 17, 2020

gabriel-tincu pushed a commit to aiven/kafka-python that referenced this pull request Sep 22, 2020

Feature: delete consumergroups (dpkp#2040)

577a071

* Add consumergroup related errors * Add DeleteGroups to protocol.admin * Implement delete_groups feature on KafkaAdminClient

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature delete consumergroups #2040

Feature delete consumergroups #2040

swenzel commented Apr 24, 2020 •

edited by dpkp

Loading

jeffwidman Apr 24, 2020

swenzel Apr 25, 2020

jeffwidman Sep 17, 2020 •

edited

Loading

swenzel Sep 17, 2020

swenzel commented May 3, 2020

swenzel commented Jun 20, 2020

jeffwidman Sep 17, 2020

swenzel Sep 17, 2020

jeffwidman Sep 17, 2020

jeffwidman Sep 17, 2020 •

edited

Loading

swenzel Sep 17, 2020

jeffwidman Sep 17, 2020 •

edited

Loading

jeffwidman commented Sep 17, 2020

jeffwidman commented Sep 17, 2020

jeffwidman commented Sep 17, 2020

jeffwidman commented Sep 17, 2020

Feature delete consumergroups #2040

Feature delete consumergroups #2040

Conversation

swenzel commented Apr 24, 2020 • edited by dpkp Loading

jeffwidman Apr 24, 2020

Choose a reason for hiding this comment

swenzel Apr 25, 2020

Choose a reason for hiding this comment

jeffwidman Sep 17, 2020 • edited Loading

Choose a reason for hiding this comment

swenzel Sep 17, 2020

Choose a reason for hiding this comment

swenzel commented May 3, 2020

swenzel commented Jun 20, 2020

jeffwidman Sep 17, 2020

Choose a reason for hiding this comment

swenzel Sep 17, 2020

Choose a reason for hiding this comment

jeffwidman Sep 17, 2020

Choose a reason for hiding this comment

jeffwidman Sep 17, 2020 • edited Loading

Choose a reason for hiding this comment

swenzel Sep 17, 2020

Choose a reason for hiding this comment

jeffwidman Sep 17, 2020 • edited Loading

Choose a reason for hiding this comment

jeffwidman commented Sep 17, 2020

jeffwidman commented Sep 17, 2020

jeffwidman commented Sep 17, 2020

jeffwidman commented Sep 17, 2020

swenzel commented Apr 24, 2020 •

edited by dpkp

Loading

jeffwidman Sep 17, 2020 •

edited

Loading

jeffwidman Sep 17, 2020 •

edited

Loading

jeffwidman Sep 17, 2020 •

edited

Loading