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

Better error handling in base consumer implementation. #479

Open
wants to merge 23 commits into
base: develop
Choose a base branch
from

Conversation

hodgestar
Copy link
Contributor

Currently vumi.service consumers don't handle errors very well. Errors should be logged , rejected and sent back to RabbitMQ for retrying (for a start).

@ghost ghost assigned hodgestar Mar 4, 2013
self.paused = self.start_paused
self._consume_loop = LoopingCall(self._consume)
self._consume_done = self._consume_loop.start(0)
return succeed(self)
Copy link
Member

Choose a reason for hiding this comment

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

Why does this need to return a deferred?

@hodgestar
Copy link
Contributor Author

Ready for review. There are two equivalent implementations of _consume. One with inlineCallbacks one with out. I have a slight preference for the one with, @jerith prefers the one without. Other opinions welcome.

@jerith
Copy link
Member

jerith commented Mar 6, 2013

Since it turned out to be a little more convoluted than I'd thought to get rid of the @inlineCallbacks, I don't mind keeping it around for this.

@smn
Copy link
Member

smn commented Apr 2, 2013

👍 from my side on this change. The only thing we may be overlooking is that previously the consume method could return False if the message was to be nackd, currently we'd need to raise an error. I don't think anything relies on that obscure behaviour, just noting it here for the odd-chance we see weird behaviour after it lands.

@smn
Copy link
Member

smn commented Apr 9, 2013

What's stopping this from landing? Am I missing some major objections from people?

@hodgestar
Copy link
Contributor Author

Although I like the errback support and plan to keep it, I think the real solution here is:

  • Add a special exception that can be raised to indicate that a message should not be requeued.
  • Catch that exception in the consumer.
  • Add a configuration option for a dead-letter exchange.

Thoughts?

@hodgestar
Copy link
Contributor Author

Modification (suggested by @jerith):

  • Add support for not requeuing messages that the same worker has attempted to process N times in the past M minutes (where N and M are configurable and default to 2 and 1 respectively).

@hodgestar
Copy link
Contributor Author

I've actually changed my mind on how this should be done initially. I think we should:

  • dead-letter messages that result in uncaught exceptions straight away if the consumer has a dead-letter exchange (and requeue such messages if the consumer does not).
  • add a tool for viewing dead-lettered exchanges, seeing queues and requeueing either individual messages or all messages in a queue back to their previous queue.
  • for this PR I propose we write a small command-line tool and ticket an HTTP API and GUI for later.

@smn
Copy link
Member

smn commented Apr 17, 2013

I think the existing rabbitmq management plugin already goes a long way to seeing exchanges, queues and has the ability to inspect queues & re-queue.

@ghost ghost assigned jerith and hodgestar Dec 3, 2013
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

3 participants