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

Add admin API client #122

Merged
merged 3 commits into from
Aug 11, 2019
Merged

Add admin API client #122

merged 3 commits into from
Aug 11, 2019

Conversation

benesch
Copy link
Collaborator

@benesch benesch commented Apr 25, 2019

There's probably a few more bugs here to shake out, but this is mostly ready to go.

This is based off of #119/#120. It's not technically tied to any of the changes in #119/#120, if for some reason we want to land this before those PRs.

Fix #92.

@benesch benesch force-pushed the admin-client branch 3 times, most recently from 131dcd4 to 6143701 Compare April 26, 2019 20:33
@benesch
Copy link
Collaborator Author

benesch commented Apr 26, 2019

Ok, fixed a few bugs and got tests passing. This is at least ready for review! (No rush! I realize this is quite a bit of code.)

@flavray flavray self-requested a review April 29, 2019 10:06
@mahulst
Copy link

mahulst commented Jun 23, 2019

I've used this branch to create and delete topics, and it works like a charm. Except for testing, is there anything else I can do to help out and get this PR merged?

@Totoketchup
Copy link

This looks great and it would be useful. What is the status of this PR ?

@benesch
Copy link
Collaborator Author

benesch commented Aug 4, 2019

We've been using this patch successfully at @MaterializeInc for a while now. It's waiting on a review from either @fede1024 or @flavray, both of whom I think have little time to dedicate to maintenance these days, unfortunately.

@fede1024
Copy link
Owner

fede1024 commented Aug 5, 2019

Hi, thank you for contributing and sorry for the very very slow response time on our side. The change looks good. Would you mind resolving the configs?

@benesch
Copy link
Collaborator Author

benesch commented Aug 6, 2019

It's quite alright, @fede1024! Thank you for finding the time. I've solved the rebase conflicts; hopefully should be good to go once CI passes.

@benesch benesch force-pushed the admin-client branch 6 times, most recently from e16edca to d871af4 Compare August 6, 2019 16:31
After diving into the librdkafka source, this is a valid return value
for metadata.orig_broker_id().
@benesch
Copy link
Collaborator Author

benesch commented Aug 6, 2019

Woohoo, it's green! The last commit is new, and it took a bit to figure out what was going on. Looks like we're now tickling a different code path inside librdkafka in test_metadata and getting metadata from bootstrap "broker" -1 instead of 0, which is perfectly valid and expected (see confluentinc/librdkafka@f50747e).

@fede1024 fede1024 merged commit caa1d92 into fede1024:master Aug 11, 2019
@fede1024
Copy link
Owner

Great work, thank you!

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.

Add admin client functions
4 participants