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

added error check for partition list #90

Merged
merged 1 commit into from
Jun 26, 2018

Conversation

accelerated
Copy link
Contributor

Based on issue 89

@mfontanini
Copy link
Owner

Okay only now I checked this PR and the associated issue. What a ridiculous API... The function returns no error but then there can actually be errors? That's completely unexpected.

Anyway, I think this should be alright. My only concern is if this will actually throw for cases where it currently is silently failing (and there's actually not a critical error). Do you know what could cause e.g. assign to throw? Obviously assuming you're using it properly and not assigning some bogus topic name or something like that.

@accelerated
Copy link
Contributor Author

accelerated commented Jun 21, 2018

Hi, yes I agree, if any partitions had an error, the API should have returned an error as well instead of success. The APIs I gave as example are not the only ones that have this behavior (you can grep rdkafka.h file for more but since you're not using all of them it does not matter here), so honestly I'm not sure what type of failures trigger it, maybe assigning invalid partitions, getting metadata for invalid partitions, setting bad offsets that are not within the special offset values, etc. I think that could be a question for librdkafka owner but he seems very busy at the moment.
In my case I wanted to make sure that if I pause/resume before the assignment is made, the pause works and all partitions are actually paused.

@accelerated
Copy link
Contributor Author

@mfontanini is this ok?

@mfontanini mfontanini merged commit 577bbb0 into mfontanini:master Jun 26, 2018
@mfontanini
Copy link
Owner

Yep, thanks!

@accelerated accelerated deleted the partition_errors branch October 10, 2018 18:29
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