Skip to content
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

Feature/multithreading #1801

Closed
wants to merge 5 commits into from

Conversation

davidheitman
Copy link
Contributor

@davidheitman davidheitman commented May 8, 2019

This allows multithreading for list_consumer_groups() and describe_consumer_groups(), so brokers are access in parallel rather than sequentially, greatly improving performance.


This change is Reviewable

@jeffwidman
Copy link
Collaborator

jeffwidman commented May 17, 2019

I understand what you're trying to do here, I literally ran into this problem just two days ago at work.

However, I wouldn't solve it at this layer... the underlying KafkaClient instance already runs an event loop and returns a future. So it might be cleaner to leverage that to make the requests non-blocking/parallel. That's how the Java client does it.

The biggest problem with this approach is that it's a bit more invasive because all the rest of these methods have to manage futures/callbacks, but that is probably the right way to go long term.

Because the alternative here of layering on threading on top of an event loop feels a bit convoluted... like we took non-blocking, made it blocking, then tried to make it non-blocking again with threads.

Would you be interesting in trying to convert KafkaAdminClient to use futures instead?

@davidheitman
Copy link
Contributor Author

Certainly, great suggestion. Not something I've worked with before, but I'll do some reading and see what I can do.

@davidheitman
Copy link
Contributor Author

Haven't tested this yet, but you're talking about something like this I think: davidheitman@ba16a65#diff-c893e6ff8a43607853864c15224c1802

@davidheitman
Copy link
Contributor Author

And tested. Works like a charm, I'll open a PR for the new branch.

@davidheitman
Copy link
Contributor Author

#1807

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