Start mirrors synchronously when transferring CMQ leadership #2751
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
of classic mirrored queues. There are cases when asynchronously adding mirrors makes a lot of sense: e.g. when a new node joins the cluster. In this case, if we add mirrors asynchronously, this operation will race with the step that removes mirrors. As a result, we can end up with a queue that decided that it had no promotable replicas => data loss from the transfer. Closes #2749. Pairs: @dcorbacho, @mkuratczyk
Since we only consider nodes hosting in-sync replicas for transfer candidates, we can drop only one mirror instead of N, and reduce the load caused by this operation. This does not affect CMQ leadership transfer when performed in the context 'rabbitmq-queues rebalance' Pair: @dcorbacho, @mkuratczyk
michaelklishin
changed the title
michaelklishin
changed the title
not only it uses unfortunate terms to refer to secondaries, it is not particularly clear.
|
We are considering exclude CMQ leadership transfer from maintenance mode because it can take quite some time in clusters with a lot of queue, and this cannot really be optimized. Quorum queues transfer leadership a lot more efficiently, so that can stay. However, this change can be merged as is. |
NickGrantX
reviewed
Jan 27, 2021
| QPid = amqqueue:get_pid(Q), | ||
| SPids = amqqueue:get_slave_pids(Q), | ||
| case [Pid || Pid <- [QPid | SPids], node(Pid) =:= MirrorNode] of | ||
| PrimaryPid = amqqueue:get_pid(Q), |
There was a problem hiding this comment.
John O'Hara - inventor of AMQP - will be sad to see the demise of QPid - AMQP's first implementation - from the RabbitMQ codebase
There was a problem hiding this comment.
Hahaha. Definitely a good one 🤓
michaelklishin
added a commit
that referenced
this pull request
Jan 27, 2021
Start mirrors synchronously when transferring CMQ leadership (cherry picked from commit 50761cb)
mkuratczyk
added a commit
to rabbitmq/rabbitmq-website
that referenced
this pull request
Jan 28, 2021
Starting with 3.8.12, `rabbitmq-upgrade` drain will not transfer classic mirrored queue leaders. See rabbitmq/rabbitmq-server#2751 for more info
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Proposed Changes
This changes classic mirror queue leadership transfer to add mirrors synchronously and also delete
fewer mirrors where it's not warranted.
We also used this as an opportunity to rename a few things around the relevant code paths.
Closes #2749.
References #2737.
Types of Changes
Checklist
CONTRIBUTING.mddocumentFurther Comments
Pairs: @dcorbacho @mkuratczyk.