Provide ability for local cluster topology query

[ranshid]

trafficstars

Some applications might require frequent node cluster slots ownership query. For example lets take the case of cluster wide scan of keys. the application would need to perform the scan on each cluster node and after completion keep track of which slots were already covered. this will require to make an extra call to cluster SLOTS/SHARDS/NODES follow each scan. While we have some optimizations for some of these commands (eg cache cluster slots) It might not provide a good enough solution for clients who will still have to parse a potentially very long cluster slots response.

This PR suggests the following modifications:

1. CLUSTER NODES will have an optional extra parameter 'node-id'. User can also provide 'myself'. When node-id is provided reply will contain information ONLY for the specific node. Example: ```

   cluster nodes myself "fcc9d096ad7dfa9f365c917941201a5bf037eb20 127.0.0.1:7000@17000 myself,slave 7cd62de6bb802699dad0c33961b2542c88afbeca 0 0 7 connected" ```

2. CLUSTER SLOTS will have an optional extra parameter 'node-id'. User can also provide 'myself'. When node-id is provided reply will contain information ONLY for the specific node. In case the node is a replica (owns no slots) the information will be taken for the nodes current primary. Note that since small cluster might have few nodes but a very large slots response (eg fragmented slots range) We will also seperatly cache the local cluster slots response. Example:

   cluster slots myself

1. 1. (integer) 5461
2. (integer) 10922 3) 1) "127.0.0.1" 2) (integer) 7001 3) "7330c7461582a447520745518f2d3a53ea2a5b0c" 4) (empty array) 4) 1) "127.0.0.1" 2) (integer) 7005 3) "f8b7e3188b0ec5bc6ea1a52342733dbb3dde2680" 4) (empty array) ```

3. CLUSTER SHARDS will have an optional extra parameter 'node-id'. User can also provide 'myself'. When node-id is provided, only the node's shards data will be return as part of the reply. Example: ```

   cluster shards myself

1. 1. "slots"
2. (empty array) 3) "nodes" 4) 1) 1) "id" 2) "7330c7461582a447520745518f2d3a53ea2a5b0c" 3) "port" 4) (integer) >7001 5) "ip" 6) "127.0.0.1" 7) "endpoint" 8) "127.0.0.1" 9) "role" 10) "master" 11) "replication-offset" 12) (integer) 2716 >13) "health" 14) "online" 2) 1) "id" 2) "f8b7e3188b0ec5bc6ea1a52342733dbb3dde2680" 3) "port" 4) (integer) 7005 5) >"ip" 6) "127.0.0.1" 7) "endpoint" 8) "127.0.0.1" 9) "role" 10) "replica" 11) "replication-offset" 12) (integer) 2716 13) >"health" 14) "online"

Note about using node-id as parameter In this initial PR I suggested to use the node-id in order to select which cluster node the data is requested for. This is good in order to allow clients to randomly send the request to any of the cluster nodes, or to traget a specific node without the need to understand exactly which node is it (using myself). However clients usually do not use cluster node-id and most clients would probably like to have host+port flavor of these different subcommands. I think it would be fairly easy to add host port flavor eg:

cluster nodes 127.0.0.1 6397 or cluster nodes some-node-hostname.com 7000

but the implementation would need to search the node by ip/hostname which will require some extra work and I think we can add it as a followup.

TODO:

• [ ] Get TSC general approval
• [x] Create tests

[codecov[bot]]

Codecov Report

Attention: Patch coverage is 89.84375% with 13 lines in your changes missing coverage. Please review.

Project coverage is 70.65%. Comparing base (2673320) to head (3c8b81b). Report is 333 commits behind head on unstable.

Files with missing lines	Patch %	Lines
src/cluster.c	85.18%	8 Missing :warning:
src/cluster_legacy.c	94.52%	4 Missing :warning:
src/debug.c	0.00%	1 Missing :warning:

Additional details and impacted files

@@             Coverage Diff              @@
##           unstable     #808      +/-   ##
============================================
+ Coverage     70.41%   70.65%   +0.24%     
============================================
  Files           112      112              
  Lines         61509    61577      +68     
============================================
+ Hits          43309    43505     +196     
+ Misses        18200    18072     -128     

Files with missing lines	Coverage Δ
src/commands.def	100.00% <ø> (ø)
src/server.h	100.00% <ø> (ø)
src/debug.c	54.32% <0.00%> (ø)
src/cluster_legacy.c	85.43% <94.52%> (+0.08%)	:arrow_up:
src/cluster.c	87.56% <85.18%> (-0.79%)	:arrow_down:

... and 9 files with indirect coverage changes

[madolson]

@asafpamzn @barshaul Can you give feedback on what you guys think about the implementation.

[barshaul]

As you mentioned Ran, most clients don’t store the ‘node-id’ and instead use ‘host:port’ as the identifier. So, adding the ‘node-id’ alone wouldn’t be particularly useful, as it would require client-side code changes. However, including the ‘myself’ flag makes it compatible without requiring changes to existing clients. So overall LGTM, and I support the followup host:ip option, as it is more client- and user-friendly.

Aside from executing a cluster-wide SCAN, are there other cases where you found this addition could be beneficial?

@avifenesh worked on the cluster scan in GLIDE—maybe you have some insights to add?

[avifenesh]

As you mentioned Ran, most clients don’t store the ‘node-id’ and instead use ‘host:port’ as the identifier. So, adding the ‘node-id’ alone wouldn’t be particularly useful, as it would require client-side code changes. However, including the ‘myself’ flag makes it compatible without requiring changes to existing clients. So overall LGTM, and I support the followup host:ip option, as it is more client- and user-friendly.

Aside from executing a cluster-wide SCAN, are there other cases where you found this addition could be beneficial?

@avifenesh worked on the cluster scan in GLIDE—maybe you have some insights to add?

Speaking from the perspective of wide cluster scan only, that will be very beneficial, and potentially decrease the cost to almost "regular" Scan. Im pretty sure that's there's many cases were it would be beneficial, the cost of pulling al slots and parsing for single node needed is way higher than it should.

[zuiderkwast]

Personally, I think we should instead actually do #33.

[avifenesh]

Beyond the feature itself, I'm concerned about the current state of cluster topology API.

I feel like maintaining all the 3 API(s) is going to be a pain in the longer term. I think we should figure out which API slots, shards, nodes has the right interface and flexibility to keep improving in the future. Then, we should make changes into that particular API. I felt like we all were aligning towards CLUSTER SLOTS.

Overall, this should make the client developers life simpler (don't need to figure out which one is better) and also force them to pick the one which has all the latest improvements.

I feel like the three have different roles and returning different amount of information. But actually change it to one command return the subsets of data requested base on commands parameters can be much more convenient in most cases. I can decide whether i want id's, ip, slots, role, state etc. without the need to get the whole data set over the net and perform parsing with many optional cases to take into consideration (slots are under migration for example and the data set returned is full of -> ). A bit like graphQL vs REST.

[PingXie]

@ranshid, I’m not entirely sure how common this specific use case will be, but I’m open to the idea. That said, if we’re going to introduce it, I think we should avoid assuming the position of new arguments. Using key-value pairs for filters, like this:

CLUSTER SLOTS FILTER shard_id=<id> node_id=<id> ip=<ip> port=<port>

would allow us to add more filtering options later without worrying about argument order or compatibility.

[madolson]

I feel like we learned a very painful lesson from CLUSTER SHARDS, which was although something might seem convenient for clients, they might not adopt it and we end up with a more complex server side API as a result. I want the client folks begging us to implement this feature before we move forward with it. I want them to have a concrete use case that needs this.

CLUSTER SLOTS FILTER shard_id= node_id= ip= port=

The typical way to add these types of arguments would be like:

CLUSTER SLOTS FILTER SHARD-ID <id> NODE-ID <id> IP PORT <port>

Typically all arguments are space delimited.

[madolson]

@valkey-io/core-team So reading through this again, right now I don't think we should move forward with the proposal since the use case isn't all that clear. For the SCAN use case, I think we have the alternative proposal with https://github.com/valkey-io/valkey/issues/33 which I think is a bit more practical of a proposal for solving cluster scan.

[hwware]

Let's move to https://github.com/valkey-io/valkey/issues/33 and can we close this pr now?