DO NOT MERGE (needs signed CLA) Add a configurable limit for number of exchanges #14304
Conversation
| case rabbit_db_exchange:exists(XName) of | ||
| true -> | ||
| %% Allow re-declares of existing exchanges when at the | ||
| %% exchange limit. | ||
| ok; |
There was a problem hiding this comment.
Note: allowing redeclares is important because if you reach the exchange limit and restart a Rabbit node, it will fail to boot due to the exchange declarations done in boot steps
deps/rabbit/src/rabbit_exchange.erl
Outdated
| @@ -105,6 +105,7 @@ serial(X) -> | |||
| Ret :: {ok, rabbit_types:exchange()} | {error, timeout}. | |||
There was a problem hiding this comment.
Nice! small comment, can we update the rabbit_exchange:declare/7 spec to reflect that this can now also return a channel exit / protocol error?
There was a problem hiding this comment.
I added no_return() here and a comment 👍
Note that this exit was possible before this change too if the exchange's validations failed:
rabbitmq-server/deps/rabbit/src/rabbit_exchange_type.erl
Lines 35 to 36 in 5c26be6
(And this already logs a warning, so there's no need to also log in addition to exiting like we do for the vhost limit)
There was a problem hiding this comment.
Cool. I was thinking rabbit_types:channel_exit() gives a bit more context here for the type? Can we use that instead?
36bc319 to
acfd6b6
Compare
There was a problem hiding this comment.
exchange_max is not consistent with the queue equivalent, cluster_queue_limit. Perhaps this new setting should be named cluster_exchange_limit. This is a global limit.
Speaking of which, I guess we should also introduce a per-vhost limit as well? Possibly as a follow-up change.
|
Yeah adding per-vhost limits as a follow up sounds good to me. The naming is a bit tricky: config options in this section tend to follow the |
|
|
acfd6b6 to
3bd151e
Compare
|
@the-mikedavis thank you for renaming the setting. I'm afraid we have to wait for a signed CLA to be returned to us by one of your colleagues, which is a work in progress and hopefully won't take too long. |
| "exchange limit of ~b is reached", | ||
| [OverLimitXName, ?EXCHANGE_LIMIT])), | ||
| ?assertExit( | ||
| {{shutdown, {server_initiated_close, 406, ExpectedError}}, _}, |
There was a problem hiding this comment.
It's not a big deal but I would not depend on a specific message in the test. The message can change, arguably the 406 PRECONDITION_FAILED response is all we care about.
However, I don't feel strongly about this.
These functions will be used in the child commit for a check on the number of exchanges. We can use the projection to avoid bothering the Khepri process with a query.
3bd151e to
d4e06ad
Compare
This is like @SimonUnge's past work for vhosts and queues but for limiting the total number of exchanges.
Vhosts and queues are more expensive since they run processes while exchanges are just metadata. A limit would be good for exchanges too though, because if you create a really large number of them you can increase the memory footprint by quite a bit - through taking more space in the metadata store and making more work for monitoring. By default the limit is unset and it can be enabled with
cluster_exchange_limit = 100or explicitly disabled withcluster_exchange_limit = infinity, like the vhost limit.I've also added some small changes to the Khepri impls for
rabbit_db_exchange'scount/0andexists/1so that we use the projection ETS table rather than a query, to make this as cheap as possible.