search

rabbitmq / ra

A Raft implementation for Erlang and Elixir that strives to be efficient and make it easier to use multiple Raft clusters in a single system.
Other
813 stars 96 forks source link

Add `ra:key_metrics/2` #461

Closed dumbbell closed 2 months ago

dumbbell commented 2 months ago

Why

This allows the caller to pass a timeout to erpc:call/5.

ra:key_metrics(ServerId) is the same as:

ra:key_metrics(ServerId, ?DEFAULT_TIMEOUT).

This is a change of behavior because the default timeout was infinity, however this is now consistent with other public functions.

dumbbell commented 2 months ago

I set the default timeout to infinity to keep the existing behavior. Should I set it to ?DEFAULT_TIMEOUT so that the behavior is consistent with other functions?

dumbbell commented 2 months ago

Replying to my own question: I changed the patch to use the same default timeout than other functions.

This is a breaking change but I believe the timeout was rarely a problem for the callers of that API. Also, the new behavior is saner and consistent with other functions.

the-mikedavis commented 2 months ago

It looks like the places that call key_metrics/1 in RabbitMQ already use erpc calls to the proper node so that only the first function clause of key_metrics/1 is ever used, and the callsites provide their own timeout: https://github.com/rabbitmq/rabbitmq-server/blob/963c5d6c8fdfd1affced492c68eaa4107e1d15d3/deps/rabbit/src/rabbit_quorum_queue.erl#L1118-L1119 for example. So for RabbitMQ I don't think we need to make any changes to address the breaking change.