Skip to content

Fix list_consumer_groups() to query all brokers #1635

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.

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

Merged
merged 1 commit into from
Nov 18, 2018

Conversation

jeffwidman
Copy link
Contributor

@jeffwidman jeffwidman commented Nov 13, 2018

Fix list_consumer_groups() to query all brokers

Previously, this only queried the controller. In actuality, the Kafka
protocol requires that the client query all brokers in order to get the
full list of consumer groups.

Note: The Java code (as best I can tell) doesn't allow limiting this to
specific brokers. And on the surface, this makes sense... you typically
don't care about specific brokers.

However, the inverse is true... consumer groups care about knowing their
group coordinator so they don't have to repeatedly query to find it.

In fact, a Kafka broker will only return the groups that it's a
coordinator for. While this is an implementation detail that is not
guaranteed by the upstream broker code, and technically should not be
relied upon, I think it very unlikely to change.

So monitoring scripts that fetch the offsets or describe the consumers
groups of all groups in the cluster can simply issue one call per broker
to identify all the coordinators, rather than having to issue one call
per consumer group. For an ad-hoc script this doesn't matter, but for
a monitoring script that runs every couple of minutes, this can be a big
deal. I know in the situations where I will use this, this matters more
to me than the risk of the interface unexpectedly breaking.


This change is Reviewable

@jeffwidman jeffwidman force-pushed the fix-list-groups-to-query-all-brokers branch 10 times, most recently from 431fd87 to 3b2f047 Compare November 18, 2018 16:38
@jeffwidman jeffwidman force-pushed the fix-list-groups-to-query-all-brokers branch from 3b2f047 to e48796f Compare November 18, 2018 23:31
Previously, this only queried the controller. In actuality, the Kafka
protocol requires that the client query all brokers in order to get the
full list of consumer groups.

Note: The Java code (as best I can tell) doesn't allow limiting this to
specific brokers. And on the surface, this makes sense... you typically
don't care about specific brokers.

However, the inverse is true... consumer groups care about knowing their
group coordinator so they don't have to repeatedly query to find it.

In fact, a Kafka broker will only return the groups that it's a
coordinator for. While this is an implementation detail that is not
guaranteed by the upstream broker code, and technically should not be
relied upon, I think it very unlikely to change.

So monitoring scripts that fetch the offsets or describe the consumers
groups of all groups in the cluster can simply issue one call per broker
to identify all the coordinators, rather than having to issue one call
per consumer group. For an ad-hoc script this doesn't matter, but for
a monitoring script that runs every couple of minutes, this can be a big
deal. I know in the situations where I will use this, this matters more
to me than the risk of the interface unexpectedly breaking.
@jeffwidman jeffwidman force-pushed the fix-list-groups-to-query-all-brokers branch from e48796f to fec82e3 Compare November 18, 2018 23:32
@jeffwidman
Copy link
Contributor Author

jeffwidman commented Nov 18, 2018

travis tests passed before the rebase, so rebasing to cleanup the diff in the Github UI (in case there is later discussion on this) and now merging w/o waiting for travis

@jeffwidman jeffwidman merged commit 232a2d6 into master Nov 18, 2018
@jeffwidman jeffwidman deleted the fix-list-groups-to-query-all-brokers branch November 18, 2018 23:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants