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

Handle SyncGroup responses with a non-zero error and no assignments #896

Merged
merged 1 commit into from
Apr 19, 2021
Merged

Handle SyncGroup responses with a non-zero error and no assignments #896

merged 1 commit into from
Apr 19, 2021

Conversation

BrentWheeldon
Copy link
Contributor

When the error code is non-zero there isn't necessarily (ever?) an assignment. Currently the decoder raises a TypeError ("no implicit conversion of nil into String") - this updates the logic to only try to decode the assignment if there are any bytes.

@dasch
Copy link
Contributor

dasch commented Apr 19, 2021

Shouldn't we raise an error in that scenario?

@BrentWheeldon
Copy link
Contributor Author

@dasch the appropriate error is raised by Protocol.handle_error, at least in the situation I ran into (because of code in ConsumerGroup#synchronize). I'm not overly familiar with the internals of this gem but it seems like the error raising isn't generally happening in the response objects, but rather in the code that's making the request and using the response (like ConsumerGroup#synchronize in the situation I ran into).

Having said that, definitely happy to make this raise if you think that's appropriate. Would calling Protocol.handle_response in the .decode method make the most sense in that case?

When the error code is non-zero there isn't necessarily (ever?) an assignment. Currently the decoder raises a `TypeError` ("no implicit conversion of nil into String") - this updates the logic to only try to decode the assignment if there are any bytes.
@dasch
Copy link
Contributor

dasch commented Apr 19, 2021

No, you're right – and if the error is already being raised then it's fine 👍

@dasch
Copy link
Contributor

dasch commented Apr 19, 2021

Thanks!

@dasch dasch merged commit 36e6a4b into zendesk:master Apr 19, 2021
@BrentWheeldon BrentWheeldon deleted the BrentWheeldon/group-sync-failure-responses branch April 19, 2021 13:04
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