Cluster Map Interface #429
Conversation
Signed-off-by: Miles Song <[email protected]>
vmsdk/src/cluster_map.h
Outdated
| bool IsSlotOwned(uint16_t slot) const; | ||
|
|
||
| // shard lookups | ||
| const ShardInfo* GetShardById(const std::string& shard_id) const; |
There was a problem hiding this comment.
How is shard-not-found signalled? Also, use string_view to pass in string parameters.
There was a problem hiding this comment.
If shard not found then this function would return nullptr. Another choice is to return std::optional
vmsdk/src/cluster_map.h
Outdated
| primary_targets_; | ||
| std::vector<valkey_search::query::fanout::FanoutSearchTarget> | ||
| replica_targets_; | ||
| std::vector<valkey_search::query::fanout::FanoutSearchTarget> random_targets_; |
There was a problem hiding this comment.
does random_targets make sense here? How do we randomize from one call to the next call?
There was a problem hiding this comment.
For here I think kRandom would just return the same set of nodes in a certain period. When the CreateNewClusterMap being called and refresh the cluster map, another set of random nodes are get selected.
vmsdk/src/cluster_map.h
Outdated
| // Pre-computed target lists | ||
| std::vector<valkey_search::query::fanout::FanoutSearchTarget> | ||
| primary_targets_; | ||
| std::vector<valkey_search::query::fanout::FanoutSearchTarget> | ||
| replica_targets_; |
There was a problem hiding this comment.
I don't see any interfaces that get any of these. Also, in looking at FanoutSearchTarget, I'm wondering how that's different from some per-Node information structure which belongs to this class. I'd recommend moving that struct into this class and naming it something like NodeInfo (parallel to ShardInfo).
There was a problem hiding this comment.
I think we can create a NodeInfo Struct in this class. Also all the current FanoutSearchTarget code can be moved entirely into the cluster map object, including the selection of nodes (kPrimary/kReplica/kRandom/kAll)
Signed-off-by: Miles Song <[email protected]>
vmsdk/src/cluster_map.h
Outdated
| std::string shard_id; | ||
| std::string primary_address; | ||
| std::vector<std::string> replica_addresses; | ||
| std::vector<uint16_t> owned_slots; |
There was a problem hiding this comment.
I believe an ordered set would be more useful than a vector. When you fingerprint, it will eliminate any order dependency.
There was a problem hiding this comment.
Should it be an unordered_set or a set here, is the order matter here?
vmsdk/src/cluster_map.h
Outdated
| // Pre-computed target lists | ||
| std::vector<NodeInfo> primary_targets_; | ||
| std::vector<NodeInfo> replica_targets_; | ||
| std::vector<NodeInfo> random_targets_; |
There was a problem hiding this comment.
You're proposing to tie the update frequency of the random_targets_ to a particular time-rate (the rate at which the overall ClusterMap is regenerated). This suggests a relationship between the time-rate and the duration of the search operations. What time-rate were you targetting? What is the expected duration of the search operations?
There was a problem hiding this comment.
Let's discuss this in meeting
Signed-off-by: Miles Song <[email protected]>
boda26
force-pushed
the
cluster-map-api
branch
from
237f22f to
46197f6
Compare
Signed-off-by: Miles Song <[email protected]>
2 participants
Initial design of cluster map interface. This interface enables instant retrieval of cluster slot status, shard info and fingerprints, and primary/replica/random node targets.