8000 Poison handling in quorum queues by dcorbacho · Pull Request #1889 · rabbitmq/rabbitmq-server · GitHub
[go: up one dir, main page]

Skip to content

Poison handling in quorum queues #1889

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

Merged
merged 12 commits into from
Feb 25, 2019
Merged

Poison handling in quorum queues #1889

merged 12 commits into from
Feb 25, 2019

Conversation

dcorbacho
Copy link
Contributor
@dcorbacho dcorbacho commented Feb 18, 2019

Proposed Changes

Drop messages that exceed the configured number of delivery attempts.
If a DLX is configured, dead letter them.

Part of #502.

Types of Changes

  • Bug fix (non-breaking change which fixes issue #NNNN)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause an observable behavior change in existing systems)
  • Documentation improvements (corrections, new content, etc)
  • Cosmetic change (whitespace, formatting, etc)

Checklist

  • I have read the CONTRIBUTING.md document
  • I have signed the CA (see https://cla.pivotal.io/sign/rabbitmq)
  • All tests pass locally with my changes
  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation (if appropriate)
  • Any dependent changes have been merged and published in related repositories

@dcorbacho dcorbacho requested a review from kjnilsson February 18, 2019 22:39
@kjnilsson
Copy link
Contributor

Looks generally fine but it needs to be tested through rabbit_fifo_props which I am pretty sure will fail as we need to persist the delivery count in the dehydrated state as well message bytes. Perhaps we should add the bytes to the headers map and use this as our per message dehydrated data?

Copy link
Contributor
@kjnilsson kjnilsson left a comment

Choose a reason for hiding this comment

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

When reaching the delivery_limit this code only avoids adding it back to the returns queue. It also needs to remove it from the index and the consumer's checked_out map. Probably best to use the complete function in some way or other.

Also needs eunit tests for this feature.

@kjnilsson kjnilsson force-pushed the poison-handling-qq branch 2 times, most recently from b604bb3 to f358b89 Compare February 22, 2019 12:31
dcorbacho and others added 6 commits February 22, 2019 14:14
Drop messages that exceed the configured number of delivery attemps.
If a DLX is configured, dead letter them.

[#163513253]
By ensuring the delivery count is retained when "dehydrating" the state
in preparation for snapshotting. Now the entire message header map is
stored which will take additional space w.r.t to keynamne duplication
although this can be optimised.

Also updated the property test to generate fake pids for multiple nodes
so that multi node scenarios are better covered.

[#163513253]
So that nodeups also can stand a chance to be tested
To keep all static and rarely changed data in a nested sub-structure.
This will reduce gc pressure somewhat.

[#163513253]
@michaelklishin michaelklishin merged commit b987346 into master Feb 25, 2019
@michaelklishin michaelklishin deleted the poison-handling-qq branch February 25, 2019 21:58
Tobion added a commit to symfony/symfony that referenced this pull request Oct 25, 2019
…queues (Tobion)

This PR was merged into the 4.3 branch.

Discussion
----------

[Messenger] prevent infinite redelivery loops and blocked queues

| Q             | A
| ------------- | ---
| Branch?       | 4.3
| Bug fix?      | yes
| New feature?  | no <!-- please update src/**/CHANGELOG.md files -->
| Deprecations? | no <!-- please update UPGRADE-*.md and src/**/CHANGELOG.md files -->
| Tickets       | Fix #32055
| License       | MIT
| Doc PR        |

This PR solves a very common fitfall of amqp redeliveries. It's for example explained in https://blog.forma-pro.com/rabbitmq-redelivery-pitfalls-440e0347f4e0
Newer RabbitMQ versions provide a solution for this by itself but only for quorum queues and not the classic ones, see rabbitmq/rabbitmq-server#1889

This PR adds a middleware that throws a RejectRedeliveredMessageException when a message is detected that has been redelivered by AMQP.

The middleware runs before the HandleMessageMiddleware and prevents redelivered messages from being handled directly. The thrown exception is caught by the worker and will trigger the retry logic according to the retry strategy.

AMQP redelivers messages when they do not get acknowledged or rejected. This can happen when the connection times out or an exception is thrown before acknowledging or rejecting. When such errors happen again while handling the redelivered message, the message would get redelivered again and again. The purpose of this middleware is to prevent infinite redelivery loops and to unblock the queue by republishing the redelivered messages as retries with a retry limit and potential delay.

Commits
-------

d211904 [Messenger] prevent infinite redelivery loops and blocked queues
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.

3 participants
0