-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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
Fix quorum queue status calculation to ignore non-voters #10394
Conversation
d93644a
to
0ba62bd
Compare
0ba62bd
to
3a52acc
Compare
Opening for review. This PR
|
@illotum can we use |
3a52acc
to
34202f1
Compare
Reverted to direct state_query (and pruned everything unrelated). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this change will make the /queues HTTP API slower when there are many queues. It will need testing but I can't approve as is.
@@ -1783,6 +1785,14 @@ get_nodes(Q) when ?is_amqqueue(Q) -> | |||
#{nodes := Nodes} = amqqueue:get_type_state(Q), | |||
Nodes. | |||
|
|||
get_voters(Q) when ?amqqueue_is_quorum(Q) -> | |||
Leader = amqqueue:get_pid(Q), | |||
case ra:voters(Leader) of |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
whitespace
@@ -1672,34 +1672,36 @@ online(Q) when ?is_amqqueue(Q) -> | |||
is_pid(Pid)]. | |||
|
|||
format(Q, Ctx) when ?is_amqqueue(Q) -> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this function is called for every queue when using the /queues HTTP API, depending on sort columns this could mean all queues in the system. Please compare HTTP API query times with and without this change with 5k quorum queues in a 3 node cluster.
I think we need to find another way of achieving this. Perhaps we need to persist the voting set in the queue record queue type state
Closing per discussion with @illotum @kjnilsson. Unlike #10304, this PR needs to be reworked from scratch. Since this change does not break anything and is arguably a bug fix, we can ship it in a 3.13.x patch in case a revised version does not make it in time for 3.13.0. |
Proposed Changes
Fix: calculating qq status will ignore non-voters in cluster size calculation.
Add: list voters over HTTP API (via rabbit_quorum_queue:format/1).
Types of Changes
Checklist
CONTRIBUTING.md
documentFurther Comments