REVERTED Pass the message to rabbit_backing_queue:discard callback

[noxdafox]

Proposed Changes

The rabbit_backing_queue:discard callback is passing the message ID to the implementer. This is often not enough to carry on some necessary work as it's seen in the rabbit_priority_queue comment.

In my particular case, this makes it hard to fix the following issue:
noxdafox/rabbitmq-message-deduplication#96

In the above issue a consumer starts consuming with noAck over an empty queue. A publisher publishes a single message which gets forwarded directly to the consumer. In this case, the discard callback is invoked instead of publish_delivered and therefore it's header is not removed from the deduplication cache.

Problem is the discard callback only forwards the message ID and not the whole message not providing enough context for the implementer.

This patch adjust the rabbit_backing_queue behaviour passing the whole message to the discard callback instead of its sole ID.

This implementation has some drawback as, when using mirrored queues, more data will now be exchanged between the master node and the slaves. This will be detrimental to empty HA queue performance.

Yet the following observations should be considered:

• This only applies when messages are consumed directly without acknowledgment from an empty queue
• HA/mirrored queues are not the ideal choice when high performance are a need
• The consumption without acknowledgment is a questionable pattern when combined with HA/mirrored queues
• HA/mirrored queues do not provide much value compared to normal ones when empty

Types of Changes

What types of changes does your code introduce to this project?
Put an x in the boxes that apply

• 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)
• Build system and/or CI

Checklist

Put an x in the boxes that apply.
You can also fill these out after creating the PR.
If you're unsure about any of them, don't hesitate to ask on the mailing list.
We're here to help!
This is simply a reminder of what we are going to look for before merging your code.

• I have read the CONTRIBUTING.md document
• I have signed the CA (see https://cla.pivotal.io/sign/rabbitmq)
• I have added tests that prove my fix is effective or that my feature works
• All tests pass locally with my changes
• If relevant, I have added necessary documentation to https://github.com/rabbitmq/rabbitmq-website
• If relevant, I have added this change to the first version(s) in release-notes that I expect to introduce it

[michaelklishin]

Let's limit this to 3.12.0. Thank you, @noxdafox!

[michaelklishin]

This change is not rolling-upgrade safe: if the leader of the queue is on an old node, the new ones would not be able to handle this updated message. Now, this can be addressed by making 3.12 nodes support for message IDs and messages. But old nodes won't support them.

@noxdafox I'm afraid in the interest of shipping 3.12.0 soon I have to revert this. Back to the drawing board.

[michaelklishin]

Using a feature flag on the hot path has risks. Perhaps the PR should be adjusted so that all modules support both message IDs and message records, and backported to 3.11.x.

Then, at some point, we can pass the entire message record along.

[noxdafox]

I did not consider the rolling updates scenario during the implementation of this.

@michaelklishin I am happy to help covering this as well but I might need some guidance. Are you suggesting to provide 2 implementations for backwards compatibility? I guess we can implement 2 callbacks one with the MsgID and the other with the whole Msg?

[michaelklishin]

Yes, that's the idea: have function heads that would cover both cases.

@lhoguin may have something different to suggest.

[lhoguin]

My idea is to just send MsgId to gm and make the discard function handle both. In that case older versions will not receive/support a full Msg which is probably fine. Newer versions will receive Msg first and then the gm messages will have MsgId which might be good enough since it's just used to confirm everyone got the event. The backing queue can still receive Msg from the CMQ master, and normal CQs will also get Msg. Just a theory though, I don't know how the plugin interacts with CMQs.