-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Add group coordinator lookup #1641
Conversation
195a889
to
f1133fe
Compare
f1133fe
to
20a7357
Compare
20a7357
to
a5c1821
Compare
a5c1821
to
14acb4d
Compare
We need a way to send a request to the group coordinator. I spent a day and a half trying to implement a `_send_request_to_group_coordinator()` that included: 1. caching the value of the group coordinator so that it wouldn't have to be repeatedly looked up on every call. This is particularly important because the `list_consumer_groups()`, `list_consumer_group_offsets()`, and `describe_consumer_groups()` will frequently be used by monitoring scripts. I know across the production clusters that I support, using a cached value will save ~1M calls per day. 2. clean and consistent error handling. This is difficult because the responses are inconsistent about error codes. Some have a top-level error code, some bury it within the description of the actual item. 3. Avoiding tight coupling between this method and the request/response classes... the custom parsing logic for errors etc, given that it's non-standard, should live in the callers, not here. So finally I gave up and just went with this simpler solution and made it so the callers can optionally bypass this if they somehow already know the group coordinator.
14acb4d
to
98ea788
Compare
# TODO add support for dynamically picking version of | ||
# GroupCoordinatorRequest which was renamed to FindCoordinatorRequest. | ||
# When I experimented with this, GroupCoordinatorResponse_v1 didn't | ||
# match GroupCoordinatorResponse_v0 and I couldn't figure out why. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe that v1 adds the idea of a group type. All groups I have seen are consumers, but I think Kafka Streams may use group coordination for producers or even other abstract groups. In any event, we'd need to add a parameter for group type I believe.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good thought.
I'm going to merge this for now and will add that as part of switching it to a FindCoordinatorRequest (which is a bigger change of renaming things across the client/consumer code). Added it to #1630 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Emm, TransactionCoordinator is looked up by that too)
Add group coordinator lookup
We need a way to send a request to the group coordinator.
I spent a day and a half trying to implement a
_send_request_to_group_coordinator()
that included:
to be repeatedly looked up on every call. This is particularly important
because the
list_consumer_groups()
,list_consumer_group_offsets()
,and
describe_consumer_groups()
will frequently be used by monitoringscripts. I know across the production clusters that I support, using a
cached value will save ~1M calls per day.
responses are inconsistent about error codes. Some have a top-level
error code, some bury it within the description of the actual item.
classes... the custom parsing logic for errors etc, given that it's
non-standard, should live in the callers, not here.
So finally I gave up and just went with this simpler solution and made
it so the callers can optionally bypass this if they somehow already
know the group coordinator.
This change is