Added periodic topology checks #51
Conversation
barshaul
force-pushed
the
periodic_check
branch
2 times, most recently
from
b5e0304
to
3b4bbdd
Compare
barshaul
force-pushed
the
periodic_check
branch
2 times, most recently
from
f18ce76
to
8ab226b
Compare
barshaul
force-pushed
the
periodic_check
branch
3 times, most recently
from
b4b07f9
to
5b27f1f
Compare
|
@nihohit ready for review |
barshaul
force-pushed
the
periodic_check
branch
9 times, most recently
from
3aabf2d
to
e00e610
Compare
|
@nihohit this time for real, ready for review:) |
|
@barshaul benchmarks? |
redis/src/cluster_async/mod.rs
Outdated
| #[cfg(all(not(feature = "tokio-comp"), feature = "async-std-comp"))] | ||
| async_std::task::sleep(interval_duration).await; | ||
| let read_guard = inner.conn_lock.read().await; | ||
| if read_guard.get_current_topology_hash().is_none() { |
There was a problem hiding this comment.
why do that? sounds like this is exactly the situation where we want a topology refresh.
There was a problem hiding this comment.
Actually, it cannot be None. When a new ClusterConnInner is being created, before we start the periodic checks we do refresh slots. If refresh slots failed it will fail the ClusterConnInner creation, otherwise it has to set the topology hash with Some(val).
Do you think it's better if i'll remove the Option and initialize it with 0?
| Ok(()) | ||
| .await; | ||
| if topology_check_res.is_ok() && topology_check_res.unwrap() { | ||
| let _ = Self::refresh_slots_with_retries(inner.clone()).await; |
There was a problem hiding this comment.
why trigger a new process and querying more nodes, instead of using the new topology result?
There was a problem hiding this comment.
The new topology result is based on log2n nodes. We cannot know at this stage if their view is accurate - if this small number of nodes have the most common view across the cluster nodes.
I can optimize it that if it's a single node cluster we'll use this view, but i'm not sure if it's really necessary. WDYT
There was a problem hiding this comment.
IMO even using a smaller sample is safe enough here, but if you're ok with delaying the slot refresh by another network round trip, it's fine.
barshaul
force-pushed
the
periodic_check
branch
3 times, most recently
from
48308b1
to
2652921
Compare
|
@nihohit round |
redis/src/cluster_async/mod.rs
Outdated
| .await?; | ||
| Ok(()) | ||
| .await; | ||
| if topology_check_res.is_ok() && topology_check_res.unwrap() { |
barshaul
force-pushed
the
periodic_check
branch
from
2652921
to
40564f0
Compare
This PR adds an optional periodic topology checks to the async cluster client.
<style> </style>The current periodic check use the user's connections. On the next PR I will add management connections that will be used for this checks.