rabbit_db: Eliminate the delete_queue Khepri transaction#14902
rabbit_db: Eliminate the delete_queue Khepri transaction#14902
delete_queue Khepri transaction#14902Conversation
dumbbell
force-pushed
the
remove-delete-queue-transaction
branch
from
082e24d to
3a75b6c
Compare
|
I was looking into eliminating the transaction for queue deletion a while back but I didn't get the changes over the finish line. I just pushed up the branch I was working on: https://github.com/rabbitmq/rabbitmq-server/tree/md/khepri-q-delete-command. It's quite old but maybe it has some interesting leads for this branch. I also looked into using stored procedures where we use transactions now - wouldn't help us with the BEAM file changes in OTP 28 but it avoided some performance impacts. The code-server work for transactions is expensive but also the size of transaction commands can be quite large (the compiled module binary + env) and we could do a lot less work with commands + keep-while conditions. But, if I am remembering correctly, we would need some sort of migration for existing Khepri DBs to transition from transactions to keep-while and regular commands so that they could add the keep-while conditions. |
Yes, exactly. This patch is incomplete on purpose (the needed feature flag is a simple hard-coded Thank you very much for sharing your work! I will take a look at it :-) |
dumbbell
force-pushed
the
remove-delete-queue-transaction
branch
7 times, most recently
from
c49b2d4 to
a3fa372
Compare
dumbbell
changed the title
delete_queue Khepri transaction
|
This pull request is ready for review as it now takes care of the upgrade of cluster as well with the @the-mikedavis: I looked at your branch and incorporated your changes in |
dumbbell
requested review from
mkuratczyk and
the-mikedavis
and removed request for
the-mikedavis
dumbbell
force-pushed
the
remove-delete-queue-transaction
branch
2 times, most recently
from
d53fb6c to
e4e4f78
Compare
[Why] This wrapper was calling `khepri_adv:delete_many()`, not `khepri_adv:delete()` as the name was suggesting. They are not the same function and have not the same behaviour. This led to a performance issue with the branch that replaces the "delete queue" transaction by a simple delete (#14902) that was related to this confusion. [How] The wrapper is rename to reflect the Khepri API being called. While here, add a `rabbit_khepri:adv_delete()` wrapper that calls the similarily named Khepri API.
[Why] This wrapper was calling `khepri_adv:delete_many()`, not `khepri_adv:delete()` as the name was suggesting. They are not the same function and have not the same behaviour. This led to a performance issue with the branch that replaces the "delete queue" transaction by a simple delete (#14902) that was related to this confusion. [How] The wrapper is renamed to reflect the Khepri API being called. While here, add a `rabbit_khepri:adv_delete()` wrapper that calls the similarily named Khepri API.
There was a problem hiding this comment.
This looks like what I was thinking of 👍, use keep-while conditions to tie together the bindings and the queue and then examine the deleted nodes in with the delete function from the adv API. I don't think I looked very far into how to make the migration work. The feature flag approach here makes sense to me.
[Why] This wrapper was calling `khepri_adv:delete_many()`, not `khepri_adv:delete()` as the name was suggesting. They are not the same function and have not the same behaviour. This led to a performance issue with the branch that replaces the "delete queue" transaction by a simple delete (#14902) that was related to this confusion. [How] The wrapper is renamed to reflect the Khepri API being called. While here, add a `rabbit_khepri:adv_delete()` wrapper that calls the similarily named Khepri API.
[Why] This wrapper was calling `khepri_adv:delete_many()`, not `khepri_adv:delete()` as the name was suggesting. They are not the same function and have not the same behaviour. This led to a performance issue with the branch that replaces the "delete queue" transaction by a simple delete (#14902) that was related to this confusion. [How] The wrapper is renamed to reflect the Khepri API being called. While here, add a `rabbit_khepri:adv_delete()` wrapper that calls the similarily named Khepri API. (cherry picked from commit 0d3edce)
[Why] This wrapper was calling `khepri_adv:delete_many()`, not `khepri_adv:delete()` as the name was suggesting. They are not the same function and have not the same behaviour. This led to a performance issue with the branch that replaces the "delete queue" transaction by a simple delete (#14902) that was related to this confusion. [How] The wrapper is renamed to reflect the Khepri API being called. While here, add a `rabbit_khepri:adv_delete()` wrapper that calls the similarily named Khepri API. (cherry picked from commit 0d3edce) (cherry picked from commit 77cdf87)
dumbbell
force-pushed
the
remove-delete-queue-transaction
branch
2 times, most recently
from
da0bf28 to
f0059fa
Compare
|
@the-mikedavis: If you don’t have any more comments, are you ok if I mark this branch as ready for review and merge it? |
the-mikedavis
left a comment
the-mikedavis
left a comment
There was a problem hiding this comment.
This looks great to me, thanks for bringing this work over the finish line! I expect we would will see really nice improvements in scenarios where we delete very many queues. If I remember correctly there is a setup with MQTT where this happens fairly often?
|
Thank you! This is the case when many MQTT clients are disconnected simultaneously for instance. This might be what you are referring to. |
... by using `keep_while` conditions on bindings and auto-delete exchanges. [Why] The `delete_queue` transaction's anonymous function has to be extracted by Horus, like any Khepri transaction. This is an expensive operation, but Horus uses caching to avoid most work after the first extraction. Unfortunately, even with this caching, this transaction is still very expensive when there are massive simultaneous deletions of queues. For instance when the queues' lifetime is linked to that of many clients, and all these clients get disconnected at once. [How] This patch removes the transaction entirely. Instead, it uses `keep_while` conditions on bindings and auto-delete exchanges to let Khepri handle the deletion of semantically related tree nodes. RabbitMQ just has to make a simle "delete this queue" command. To maintain backward compatibility, we introduce the `tie_binding_to_dest_with_keep_while_cond` feature flag. Its `enable` callback will take care of rewriting all bindings and auto-delete exchanges record in the store to add the new `keep_while` conditions. The binding deletion transaction is also simplified because it benefits from the `keep_while` conditions. We may be able to replace this transaction is the future as well in the same manner.
dumbbell
force-pushed
the
remove-delete-queue-transaction
branch
from
f0059fa to
de43879
Compare
rabbit_db: Eliminate the `delete_queue` Khepri transaction (backport #14902)
2 participants
… by using
keep_whileconditions on bindings and auto-delete exchanges.Why
The
delete_queuetransaction's anonymous function has to be extracted by Horus, like any Khepri transaction. This is an expensive operation, but Horus uses caching to avoid most work after the first extraction.Unfortunately, even with this caching, this transaction is still very expensive when there are massive simultaneous deletions of queues. For instance when the queues' lifetime is linked to that of many clients, and all these clients get disconnected at once.
How
This patch removes the transaction entirely. Instead, it uses
keep_whileconditions on bindings and auto-delete exchanges to let Khepri handle the deletion of semantically related tree nodes. RabbitMQ just has to make a simle "delete this queue" command.To maintain backward compatibility, we introduce the
replaced_delete_queue_transactionfeature flag. Itsenablecallback will take care of rewriting all bindings and auto-delete exchanges record in the store to add the newkeep_whileconditions.The binding deletion transaction is also simplified because it benefits from the
keep_whileconditions. We may be able to replace this transaction is the future as well in the same manner.