[kafka_consumer] Cleanup group coordinator lookup #1628
Conversation
|
Note that it is late at night and I have not actually tested this code yet, so I may have fat-fingered something (which hopefully the tests catch, and if not, I'm sure I'll find tomorrow when I deploy this). But wanted to get the PR up to give folks a headstart on reviewing, as I'm planning to do additional cleanup tomorrow that depends on this. |
|
Hello again @jeffwidman! Could you please rebase to get our new CI config? All new-style tests run on Travis now so forks will trigger builds. |
|
@ofek that's great news. I love gitlab, but the inability to run on forks is a killer. Rebased. I have not had a chance to deploy this myself, but if the tests pass, this should be good to merge because it's in the hotpath. |
|
tests are failing, and a quick look through travis reveals somewhat inscrutable logs so not an obvious fix, maybe later tonight I can dig into it further |
|
Hello Jeff! Have you had a chance to work on this? |
|
This issue has been automatically marked as stale because it has not had activity in the last 30 days. Note that the issue will not be automatically closed, but this notification will remind us to investigate why there's been inactivity. Thank you for participating in the Datadog open source community. |
|
@ofek my apologies, I am still interested in this, but am out on paternity leave / vacation / moving much of July / August, so won't be able to get to this until Sept/Oct time frame. For now it can sit, or someone else is welcome to pick it up. |
|
@jeffwidman No worries, congratulations! See you in September 😄 |
|
This issue has been automatically marked as stale because it has not had activity in the last 30 days. Note that the issue will not be automatically closed, but this notification will remind us to investigate why there's been inactivity. Thank you for participating in the Datadog open source community. |
You must have a known group coordinator to lookup the offsets for a consumer group that stores its offsets in Kafka. I do not understand why this code tries to handle unknown group coordinators by firing requests to all the brokers as that seems like a waste of network calls. This only refactors the existing code to be cleaner. There is room for additional error handling and logging. Plus potentially caching the group coordinator value on the client's cluster metadata rather than re-fetching it every time... instead just assume it's still the old one, and if not catch the error and trigger a group coordinator lookup. But that is left for a later PR, this one merely cleans up the existing logic to make it easier to refactor later.
|
Closing in favor of #2730 as that takes care of cleaning up the group coordinator lookup by relying on some other code from dpkp/kafka-python#1641 |
You must have a known group coordinator to lookup the offsets for a
consumer group that stores its offsets in Kafka. I do not understand
why this code tries to handle unknown group coordinators by
firing requests to all the brokers as that seems like a waste of network
calls.
This only refactors the existing code to be cleaner. There is room for
additional error handling and logging. Plus potentially caching the
group coordinator value on the client's cluster metadata rather than
re-fetching it every time... instead just assume it's still the old one,
and if not catch the error and trigger a group coordinator lookup.
But that is left for a later PR, this one merely cleans up the existing
logic to make it easier to refactor later.