-
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
Add force checkpoint functions for quorum queues and command line tool #13175
base: main
Are you sure you want to change the base?
Conversation
checking failed test case (edit: some issue with |
{error, classic_queue_not_supported}; | ||
{ok, Q} when ?amqqueue_is_quorum(Q) -> | ||
{RaName, _} = amqqueue:get_pid(Q), | ||
rpc:call(Node, ra, cast_aux_command, [{RaName, Node}, force_checkpoint]), |
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 is a call with an implicit timeout (most likely of 5s). Timeouts lower than 15s are very likely to cause false positives sooner or later.
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.
Will look to see how to invoke the aux command with a specified timeout of 15s
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 was not able to find a way to increase the implicit timeout, would appreciate any pointers here :)
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.
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.
There's also rabbit_misc:rpc_call/5
but I doubt it is very relevant on Erlang 26+.
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.
Ah, by "implicit timeout" I misunderstood and thought you were mentioning a timeout to do within ra
and not rpc:call/5
.
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.
No, I was referring to the typical OTP timeout when it comes to rpc
calls.
{ok, Q} when ?amqqueue_is_quorum(Q) -> | ||
{RaName, _} = amqqueue:get_pid(Q), | ||
rpc:call(Node, ra, cast_aux_command, [{RaName, Node}, force_checkpoint]), | ||
rabbit_log:debug("Sent command to force checkpoint ~ts", [QNameFmt]); |
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.
"to force ~ts to take a checkpoint "
use RabbitMQ.CLI.Core.RequiresRabbitAppRunning | ||
use RabbitMQ.CLI.Core.AcceptsNoPositionalArguments | ||
|
||
def run([], %{ |
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.
The returned value can easily be formatted as JSON but formatter: "json"
is not supported in this PR.
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.
It seems formatter: "json"
is supported (presumably via RabbitMQ.CLI.DefaultOutput
?)
aaronseo: ~/workplace/rabbitMq/rabbitmq-server/sbin $ ./rabbitmq-queues -n force_checkpoint2 force_checkpoint --formatter json
[
{"vhost":"/","name":"qq2","result":"ok"}
,{"vhost":"/","name":"qwe","result":"ok"}
,{"vhost":"/","name":"testqq","result":"ok"}
...
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.
For tabular-looking data (so, lists of maps), yes, you can at least try DefaultOutput
and see if there may be any reasons to override what it defines.
# Implementation | ||
# | ||
|
||
defp format_result({:ok}) do |
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 reinvent the output interface used extensively by all existing commands.
:ok
, {:error, :timeout}
, {:error, _}
are all handled by RabbitMQ.CLI.DefaultOutput
, including JSON formatting of certain common returned values.
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.
The custom format_result
does have a cosmetic effect, adopted from a couple other commands (namely grow
and shrink
). For example
With just RabbitMQ.CLI.DefaultOutput
:
aaronseo: ~/workplace/rabbitMq/rabbitmq-server/sbin $ ./rabbitmq-queues -n force_checkpoint2 force_checkpoint
Forcing checkpoint for all matching quorum queues...
vhost name result
/ qq2 {ok}
/ qwe {ok}
/ testqq {ok}
/ qq1 {ok}
test qwe {ok}
test qq1 {ok}
test qq2 {ok}
...
aaronseo: ~/workplace/rabbitMq/rabbitmq-server/sbin $ ./rabbitmq-queues -n force_checkpoint2 force_checkpoint --formatter json
[
{"vhost":"/","name":"qq2","result":["ok"]}
,{"vhost":"/","name":"qwe","result":["ok"]}
,{"vhost":"/","name":"testqq","result":["ok"]}
...
With the custom format_result
:
Forcing checkpoint for all matching quorum queues...
vhost name result
/ qq2 ok
/ qwe ok
/ testqq ok
/ qq1 ok
test qwe ok
test qq1 ok
test qq2 ok
...
aaronseo: ~/workplace/rabbitMq/rabbitmq-server/sbin $ ./rabbitmq-queues -n force_checkpoint2 force_checkpoint --formatter json
[
{"vhost":"/","name":"qq2","result":"ok"}
,{"vhost":"/","name":"qwe","result":"ok"}
,{"vhost":"/","name":"testqq","result":"ok"}
...
The cosmetic changes are, I think, more beneficial with the error messages, too.
However, I'm also open to just going to the RabbitMQ.CLI.DefaultOutput
formatting, if still found to be preferred.
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.
You can override output/2
before the line where RabbitMQ.CLI.DefaultOutput
is included, and rely on RabbitMQ.CLI.DefaultOutput
as a catch-all for, say, rpc:call/4
error reporting.
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.
Decided to forego with the custom format_result
, I think the default output is a fine alternative, and allows better structuring of the error
messages. Please lmk what you think
Proposed Changes
Addresses #13137.
Adds functions to force checkpoint for a quorum queue or for all quorum queues matching a
VhostSpec
andQueueSpec
.Also adds a CLI tool to invoke that function.
Hand-in-hand with doc change PR: rabbitmq/rabbitmq-website#2175
Types of Changes
What types of changes does your code introduce to this project?
Put an
x
in the boxes that applyChecklist
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.
CONTRIBUTING.md
documentFurther Comments
The test case for
force_checkpoint_on_queue
is a bit sensitive to changes inrabbit_fifo:aux...
andrabbit_fifo:checkpoint
records. Please feel free to suggest a different testing approach.Some local testing for CLI tool
gmake run-broker RABBITMQ_NODENAME="force_checkpoint2"
Create these queues and vhost:
Some local testing for checkpoint value
gmake run-broker RABBITMQ_NODENAME="force_checkpoint2"
testqq
.observer:start()
-->Applications
-->ra
-->%2F_testqq
-->State
-->testqq
-- Observe change in
aux => checkpoint