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.

Sign up for GitHub

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

Jump to bottom
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 •
edited by michaelklishin
Loading

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?

kjnilsson
kjnilsson requested changes Feb 21, 2019
View reviewed changes
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
@dcorbacho @kjnilsson
Poison handling in quorum queues
8a3d3e6 
Drop messages that exceed the configured number of delivery attemps.
If a DLX is configured, dead letter them.

[#163513253]
@dcorbacho @kjnilsson
Include poison message handling in snapshot testing
6980462 
[#163513253]
@dcorbacho @kjnilsson
Remove poison message from indexes
e662841 
[#163513253]
@kjnilsson
Fix rabbit_fifo poison handling
723972b 
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]
@kjnilsson
Add nodeup commands to rabbit_fifo_prop
9e9eeb5 
So that nodeups also can stand a chance to be tested
@kjnilsson
fix dialyzer issues
99ee5fb
@hairyhum hairyhum mentioned this pull request Feb 23, 2019
Closed
3 tasks
@kjnilsson
Restructure rabbit_fifo state
be93004 
To keep all static and rarely changed data in a nested sub-structure.
This will reduce gc pressure somewhat.

[#163513253]
@kjnilsson
Move rabbit_fifo unit tests into own SUITE
20bc8c4
@kjnilsson
Formatting
fecd996
@kjnilsson kjnilsson mentioned this pull request Feb 25, 2019
Merged
@michaelklishin michaelklishin merged commit b987346 into master Feb 25, 2019
@michaelklishin michaelklishin deleted the poison-handling-qq branch February 25, 2019 21:58
@Tobion Tobion mentioned this pull request Oct 24, 2019
Merged
Tobion added a commit to symfony/symfony that referenced this pull request Oct 25, 2019
@Tobion
bug #34107 [Messenger] prevent infinite redelivery loops and blocked …
13853d5 
…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
Reviewers

@kjnilsson kjnilsson kjnilsson requested changes

@michaelklishin michaelklishin michaelklishin approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@dcorbacho @kjnilsson @michaelklishin
0