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

log decode errors, do not ack. #281

Merged
merged 1 commit into from Mar 14, 2014

Conversation

bkw
Copy link
Contributor

@bkw bkw commented Feb 6, 2014

In case of decoding errors (most likely caused be an installed json library that cannot deal with kombu's message buffers) log the error and do NOT ack the message. Otherwise we lose messages.

This is a precursor for another pr coming up, that fixes the decoding errors.

@@ -135,6 +135,10 @@ def on_nova(self, body, message):
(e, json.loads(str(message.body))))
raise

def on_decode_error(self, message, exc):
_get_child_logger().exception("Decode Error: %s" % exc)
# no NOT call message.ack(), otherwise the message will be lost
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/no Not/Do Not/

perhaps?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed, thanks

@SandyWalsh
Copy link
Contributor

Where is this actually called?

@bkw
Copy link
Contributor Author

bkw commented Feb 6, 2014

in kombu serialization.

@bkw
Copy link
Contributor Author

bkw commented Feb 6, 2014

or if you mean where on_decode_error is called, it is also a hook from the kombu mixin: https://kombu.readthedocs.org/en/latest/reference/kombu.mixins.html#kombu-mixins

@bkw
Copy link
Contributor Author

bkw commented Feb 6, 2014

rebased to current master

@bkw
Copy link
Contributor Author

bkw commented Feb 6, 2014

rebased again

@thomasem
Copy link

Are we still pursuing the feature this PR is related to??

@bkw
Copy link
Contributor Author

bkw commented Feb 28, 2014

Yes, but the other PR is only slightly related.
This one is useful on its own since it prohibits losing messages that could not be decoded for whatever reason. I strongly vote for merging this one, it helps diagnosing problems like #287.

@thomasem
Copy link

Going back through all that transpired, yeah, I agree. Hehe, that auto ack from the mixin base class in the on_decode_error is kind of scary.

@anujm
Copy link

anujm commented Mar 13, 2014

Can you rebase this from master, bkw?

@bkw
Copy link
Contributor Author

bkw commented Mar 14, 2014

rebased again.

anujm pushed a commit that referenced this pull request Mar 14, 2014
@anujm anujm merged commit 9d1a93a into rackerlabs:master Mar 14, 2014
@anujm
Copy link

anujm commented Mar 14, 2014

Thanks!

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.

None yet

4 participants