Skip to content

Conversation

@boda26
Copy link
Collaborator

@boda26 boda26 commented Oct 27, 2025

  1. Cluster Map Object Implementation
  2. Refresh ClusterMap at the start of each fanout operation; fanout operations use targets from ClusterMap object

…nout and use targets from cluster map object

Signed-off-by: Miles Song <[email protected]>
@boda26 boda26 requested a review from allenss-amazon October 27, 2025 21:00
@yairgott
Copy link
Collaborator

I think that it would be much more efficient if the engine allow subscription to a notification mechanism which is triggered once the slot assignment is changed. @murphyjacob4 , wdyt?

@boda26
Copy link
Collaborator Author

boda26 commented Oct 29, 2025

Updated ClusterMap Implementation (significant change)

  1. added a configurable ClusterMap expiration timestamp, only refresh when it expires
  2. created a slot-to-shard map (map start slot to end slot and ShardInfo), enable efficient slot to shard retrieval
  3. shard-level and cluster-level fingerprint
  4. complete redesign of CreateNewClusteMap, entirely eliminated GetTargets, building shard map and target vectors altogether with only one CLUSTER SLOTS call to core
  5. redesign of GetRandomTargets, randomly picking one node from each shard in pre-computed shard map

@boda26
Copy link
Collaborator Author

boda26 commented Oct 29, 2025

Todo: create a SocketAddress struct for ip and port

@boda26 boda26 requested a review from allenss-amazon October 29, 2025 04:57
@boda26
Copy link
Collaborator Author

boda26 commented Oct 29, 2025

Separately stored ip and port in ClusterMap

@boda26
Copy link
Collaborator Author

boda26 commented Oct 29, 2025

Added unit tests for ClusterMap


auto operator<=>(const NodeInfo&) const = default;

friend std::ostream& operator<<(std::ostream& os, const NodeInfo& target) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it's freestanding, you need to add inline. If you make it a member function then you don't need that.

// Coordinator port offset - same as in src/coordinator/util.h
// This offset results in 26673 for Valkey default port 6379 - which is COORD
// on a telephone keypad.
static constexpr int kCoordinatorPortOffset = 20294;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't belong in the coordinator namespace. That's an application thing, for vmsdk, I'd just put this into it's own namespace: clustermap remove the nested namespace, i.e., just put it into vmsdk.

Comment on lines 29 to 33
inline int GetCoordinatorPort(int valkey_port) {
// TODO Make handling of TLS more robust
if (valkey_port == 6378) {
return valkey_port + kCoordinatorPortOffset + 1;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As we discussed before, the coordinator port number stuff belongs elsewhere.

Signed-off-by: Miles Song <[email protected]>
@boda26
Copy link
Collaborator Author

boda26 commented Oct 31, 2025

Update 10.30

  1. fix unit test compile error on macos
  2. change to member functions to avoid large number of parameters
  3. use a map to store (start slot, end slot) for each shard owned slots
  4. use highway hash for fingerprints
  5. SocketAddress struct for ip and port
  6. boolean isPrimary, isLocal
  7. simpler CHECK error messages

TODO:

  1. node info list (4th entry of node array)
  2. possible issue when 2 slot range point to same shard, but the ShardInfo differs

Signed-off-by: Miles Song <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants