Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 1 commit into from
Apr 9, 2024

Conversation

hpatro
Copy link
Contributor

@hpatro hpatro commented Apr 8, 2024

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: redis/redis#13149

Signed-off-by: Harkrishn Patro <harkrisp@amazon.com>
Copy link
Contributor

@zuiderkwast zuiderkwast left a comment

Choose a reason for hiding this comment

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

Thanks for migrating this one!

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

@hpatro
Copy link
Contributor Author

hpatro commented Apr 8, 2024

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.

@madolson madolson added the release-notes This issue should get a line item in the release notes label Apr 9, 2024
@madolson madolson merged commit e59dd41 into valkey-io:unstable Apr 9, 2024
14 checks passed
@madolson
Copy link
Member

madolson commented Apr 9, 2024

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 madolson added the needs-doc-pr This change needs to update a documentation page. Remove label once doc PR is open. label Apr 9, 2024
PatrickJS pushed a commit to PatrickJS/placeholderkv that referenced this pull request Apr 24, 2024
valkey-io#265)

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)
```

Signed-off-by: Harkrishn Patro <harkrisp@amazon.com>
roshkhatri added a commit to roshkhatri/valkey-doc that referenced this pull request Jul 25, 2024
ref: valkey-io/valkey#265

Signed-off-by: Roshan Khatri <rvkhatri@amazon.com>
roshkhatri added a commit to roshkhatri/valkey-doc that referenced this pull request Jul 25, 2024
ref: valkey-io/valkey#265

Signed-off-by: Roshan Khatri <rvkhatri@amazon.com>
zuiderkwast pushed a commit to valkey-io/valkey-doc that referenced this pull request Aug 28, 2024
Update cluster-slots docs
ref: valkey-io/valkey#265

---------

Signed-off-by: Roshan Khatri <rvkhatri@amazon.com>
Signed-off-by: Madelyn Olson <madelyneolson@gmail.com>
Co-authored-by: Madelyn Olson <madelyneolson@gmail.com>
asafpamzn pushed a commit to asafpamzn/valkey-doc that referenced this pull request Sep 26, 2024
…#159)

Update cluster-slots docs
ref: valkey-io/valkey#265

---------

Signed-off-by: Roshan Khatri <rvkhatri@amazon.com>
Signed-off-by: Madelyn Olson <madelyneolson@gmail.com>
Co-authored-by: Madelyn Olson <madelyneolson@gmail.com>
Signed-off-by: asafpamzn <97948347+asafpamzn@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-doc-pr This change needs to update a documentation page. Remove label once doc PR is open. release-notes This issue should get a line item in the release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants