valkey icon indicating copy to clipboard operation
valkey copied to clipboard

Maintain determinstic ordering of replica(s) in CLUSTER SLOTS response

Open hpatro opened this issue 10 months ago • 1 comments

Sort clusterNode.slaves while adding a new replica to the cluster on basis of name. This will enable deterministic ordering of replica(s) information in CLUSTER SLOTS response.

Before this change:

127.0.0.1:6380> CLUSTER SLOTS
1) 1) (integer) 0
   2) (integer) 16383
   3) 1) "127.0.0.1"
      2) (integer) 6379
      3) "fc72609a620c62d073a31eed9ddde5104c1fa302"
      4) (empty array)
   4) 1) "127.0.0.1"
      2) (integer) 6381
      3) "fac84bbf2ffc5cfcdebc92c06b8ead9c3cba4051"
      4) (empty array)
   5) 1) "127.0.0.1"
      2) (integer) 6380
      3) "b1249d394326f1485df0b895f2fd38e141aa5056"
      4) (empty array)

After this change:

127.0.0.1:6380> CLUSTER SLOTS
1) 1) (integer) 0
   2) (integer) 16383
   3) 1) "127.0.0.1"
      2) (integer) 6379
      3) "fc72609a620c62d073a31eed9ddde5104c1fa302"
      4) (empty array)
   4) 1) "127.0.0.1"
      2) (integer) 6380
      3) "b1249d394326f1485df0b895f2fd38e141aa5056"
      4) (empty array)
   5) 1) "127.0.0.1"
      2) (integer) 6381
      3) "fac84bbf2ffc5cfcdebc92c06b8ead9c3cba4051"
      4) (empty array)

ref: https://github.com/redis/redis/pull/13149

hpatro avatar Apr 08 '24 18:04 hpatro

It'd be nice to add a test case that verifies that the responses are identical from all nodes in the cluster.

@zuiderkwast Actually we do compare CLUSTER SLOTS response across nodes in lot's of TCL tests via cluster_config_consistent. However, the issue which exists in the TCL tests is we never wait enough for the replica(s) to be online and surface in CLUSTER SLOTS output.

@VoletiRam is fixing the cluster_config_consistent to wait until replica(s) are online. And that should cover your request.

hpatro avatar Apr 08 '24 21:04 hpatro

This was broadly approved before the switch over, so I think it's safe to move forward with it now. We should update the docs page though for it.

madolson avatar Apr 09 '24 02:04 madolson