feat(auto_reconnect): allow runtime configuration of give_up_ms (v2) #5701
Conversation
|
sorry had to open a new PR because I seem to have lost the branch the original PR was opened on (most likely due to trying to solve issues with gitattributes! 😄) |
|
I'll take another look today. |
| node_reconnect_timeout_secs * 1000 > std::numeric_limits<int>::max()) { | ||
| throw std::runtime_error(strprintf( | ||
| "ERROR: cluster-reconnect-timeout is too large. Must be at most %d", | ||
| std::numeric_limits<int>::max())); |
There was a problem hiding this comment.
The maximum in the error message needs to take the * 1000 into account.
There was a problem hiding this comment.
I think the maximum in the error message is still consistently the actual maximum (numeric_limits::max === biggest supported integer right?). So the error message is correct, it was the checks that needed to ensure that:
a) the input number is indeed within that threshold
b) secondarily, since we intend to use it as ms the number multiplied by 1000 should still be within that range
Perhaps you think the error message should be more descriptive in the event that someone enters a number that is too big only once we are testing for conversion to ms?
There was a problem hiding this comment.
It should be:
throw std::runtime_error(strprintf(
"ERROR: cluster-reconnect-timeout is too large. Must be at most %d",
std::numeric_limits<int>::max() / 1000));
|
Left just two comments. The rest of it looks great! |
mbroadst
force-pushed
the
auto-reconnect-config
branch
2 times, most recently
from
9e140ca to
f677d3e
Compare
Currently the auto_reconnect instance used to reconnect lost nodes is hardcoded to give up reconnecting after 24 hours. This is not ideal in some scenarios where a user may want to remove a node from a cluster, without having to reset all other participating nodes in the cluster. This patch allows the user to change that value.
|
Looks good, thanks @mbroadst . |
|
I'm waiting for some small style improvements to go through review, and will cherry-pick this into |
|
@danielmewes fantastic, thanks! |
Currently the auto_reconnect instance used to reconnect lost nodes is hardcoded to give up reconnecting after 24 hours. This is not ideal in some scenarios where a user may want to remove a node from a cluster, without having to reset all other participating nodes in the cluster. This patch allows the user to change that value.
3 participants
Currently the auto_reconnect instance used to reconnect lost nodes
is hardcoded to give up reconnecting after 24 hours. This is not
ideal in some scenarios where a user may want to remove a node from
a cluster, without having to reset all other participating nodes in
the cluster. This patch allows the user to change that value.