SlotMap refactor: Added NodesMap, sharing shard addresses between shard nodes and slot map values. #185
Conversation
barshaul
force-pushed
the
fix_moved_directly2
branch
from
a200f39
to
6ce81db
Compare
| } else if moved_redirect.is_some() { | ||
| // Update relevant slots in the slots map based on the moved_redirect address, | ||
| // rather than refreshing all slots by querying the cluster nodes for their topology view. | ||
| Self::update_slots_for_redirect_change(inner.clone(), moved_redirect).await?; |
There was a problem hiding this comment.
Removed as per the design decision, updates based on MOVED errors will be done synchronously and not as part of refresh_slots.
barshaul
force-pushed
the
fix_moved_directly2
branch
from
6ce81db
to
820dbc6
Compare
…hard between shard nodes and slot map values
barshaul
force-pushed
the
fix_moved_directly2
branch
from
820dbc6
to
9165165
Compare
redis/src/cluster_slotmap.rs
Outdated
| let mut shard_id = 0; | ||
| for slot in slots { | ||
| let primary = Arc::new(slot.master); | ||
| let replicas: Vec<Arc<String>> = slot.replicas.into_iter().map(Arc::new).collect(); |
There was a problem hiding this comment.
nit: I would make here slot.replicas_vec() method (we might be needing this one-liner elsewhere, so it makes sense to make it a function)
There was a problem hiding this comment.
we don't use it elsewhere, and using this function partially moves slot so we can't consume other properties of this struct (e.g. master)
|
|
||
| use crate::cluster_routing::{Route, ShardAddrs, Slot, SlotAddr}; | ||
|
|
||
| pub(crate) type NodesMap = DashMap<Arc<String>, Arc<RwLock<ShardAddrs>>>; |
There was a problem hiding this comment.
Question:
I don't have the entire context (GitHub folds existing code etc). But from what I have seen, we don't use .write() I have only seen .read() calls. Do we need this lock?
There was a problem hiding this comment.
You're correct, it hasn't been used in this PR - the write lock will be used in the next PR where i'll add support in topology updates based on MOVED errors. Then we will need to do modifications on the ShardAddrs object - for example when a MOVED error suggests a failover, or in scenarios when a replica is removed from existing ShardAddr as it is promoted to a primary in a different shard.
| }, | ||
| ); | ||
| } | ||
|
|
||
| slot_map | ||
| } | ||
|
|
||
| #[allow(dead_code)] // used in tests |
There was a problem hiding this comment.
nit: "tests" (when placed on the same file) are allowed to access private members, so the test can access the property member directly
There was a problem hiding this comment.
It's being used in other file, however i'm going to use it in the next PR then i'll remove the dead_code
barshaul
merged commit 8d05984
into
amazon-contributing:update_slotmap_moved
…rd nodes and slot map values. (#185) * SlotMap refactor: Added new NodesMap, changed shard addresses to be shard between shard nodes and slot map values
This PR lays the foundation for supporting topology changes triggered by MOVED errors.
Slot Map Design
Currently, our slot map contains only a tree structure representing the slots and their associated ShardAddr, which holds the primary and replica addresses. For non-consecutive slots owned by the same shard, a new ShardAddr is created. The proposed design introduces the following enhancements:
This design allows for fast address retrieval for routing while enabling synchronized updates across multiple slots, even if they are non-consecutive. In the SlotMap’s binary tree, non-consecutive slots owned by the same shard are represented by different tree nodes for each range. In the proposed design, all these tree nodes will share the same ShardAddrs. As a result, when a ShardAddrs is updated due to a MOVED error for one slot, the change will automatically apply to all other slots owned by the same shard.
The diagram on the left represents the current design of SlotMap, while the diagram on the right illustrates the new design.
Full example:
