Maintain deterministic order of CLUSTER SHARDS response #411

VoletiRam · 2024-05-01T08:11:34Z

Maintain deterministic order of CLUSTER SHARDS response. Currently we don't maintain the shards/masters in sorted fashion and hence we get the order of CLUSTER SHARDS response non-deterministic on different nodes. Maintain the sorted Masters list of pointers, similar to replicas, and replace the current <shards, list[nodes]> dict which is not suitable for sorting. Add the TOPOLOGY argument to get the deterministic response which would remove the replication offset and node health status from cluster shards response. Sort the masters based on the node Id. Include the new CLUSTER SHARDS TOPOLOGY command in the cluster_config_consistent procedure to ensure thorough test coverage and conduct a sanity check on cluster consistency.

Example response of CLUSTER SHARDS TOPOLOGY in a 2 primaries 2 replicas cluster.

Response from Primary 1:

127.0.0.1:6320> cluster shards topology
1) 1) "slots"
   2) 1) (integer) 501
      2) (integer) 16383
   3) "nodes"
   4) 1)  1) "id"
          2) "879df438dd67bd56e717d8400b718c743c4355d1"
          3) "port"
          4) (integer) 6321
          5) "ip"
          6) "127.0.0.1"
          7) "endpoint"
          8) "127.0.0.1"
          9) "role"
         10) "master"
      2)  1) "id"
          2) "4c04d2a090de3e7c9a375ad0ee2e25b52c31c310"
          3) "port"
          4) (integer) 6323
          5) "ip"
          6) "127.0.0.1"
          7) "endpoint"
          8) "127.0.0.1"
          9) "role"
         10) "replica"
2) 1) "slots"
   2) 1) (integer) 0
      2) (integer) 500
   3) "nodes"
   4) 1)  1) "id"
          2) "af1e6a659a85779864456efe6a323fa7c7eda187"
          3) "port"
          4) (integer) 6320
          5) "ip"
          6) "127.0.0.1"
          7) "endpoint"
          8) "127.0.0.1"
          9) "role"
         10) "master"
      2)  1) "id"
          2) "5f4de2e73e92b67c0d4adb0f5737776f5d14e137"
          3) "port"
          4) (integer) 6322
          5) "ip"
          6) "127.0.0.1"
          7) "endpoint"
          8) "127.0.0.1"
          9) "role"
         10) "replica"

Response from Primary 2:

127.0.0.1:6321> cluster shards topology
1) 1) "slots"
   2) 1) (integer) 501
      2) (integer) 16383
   3) "nodes"
   4) 1)  1) "id"
          2) "879df438dd67bd56e717d8400b718c743c4355d1"
          3) "port"
          4) (integer) 6321
          5) "ip"
          6) "127.0.0.1"
          7) "endpoint"
          8) "127.0.0.1"
          9) "role"
         10) "master"
      2)  1) "id"
          2) "4c04d2a090de3e7c9a375ad0ee2e25b52c31c310"
          3) "port"
          4) (integer) 6323
          5) "ip"
          6) "127.0.0.1"
          7) "endpoint"
          8) "127.0.0.1"
          9) "role"
         10) "replica"
2) 1) "slots"
   2) 1) (integer) 0
      2) (integer) 500
   3) "nodes"
   4) 1)  1) "id"
          2) "af1e6a659a85779864456efe6a323fa7c7eda187"
          3) "port"
          4) (integer) 6320
          5) "ip"
          6) "127.0.0.1"
          7) "endpoint"
          8) "127.0.0.1"
          9) "role"
         10) "master"
      2)  1) "id"
          2) "5f4de2e73e92b67c0d4adb0f5737776f5d14e137"
          3) "port"
          4) (integer) 6322
          5) "ip"
          6) "127.0.0.1"
          7) "endpoint"
          8) "127.0.0.1"
          9) "role"
         10) "replica"

Ref: #114

Maintain deterministic order of CLUSTER SHARDS response. Currently we don't maintain the shards/masters in sorted fashion and hence we get the order of CLUSTER SHARDS response non-deterministic on different nodes. Maintain the sorted Masters list of pointers, similar to replicas, and get rid of <shards, list<nodes>> dict which is not suitable for sorting. Add TOPOLOGY argument to get the deterministic response which would remove the replication offset and node health status from cluster shards response. Sort the nodes based on the node Id. Use it in proc `cluster_config_consistent` for the test coverage and sanity purpose. Signed-off-by: Ram Prasad Voleti <ramvolet@amazon.com>

codecov · 2024-05-01T08:23:17Z

Codecov Report

Attention: Patch coverage is 94.87179% with 4 lines in your changes are missing coverage. Please review.

Project coverage is 68.45%. Comparing base (f4e10ee) to head (715816a).
Report is 41 commits behind head on unstable.

Additional details and impacted files

@@             Coverage Diff              @@
##           unstable     #411      +/-   ##
============================================
- Coverage     68.47%   68.45%   -0.02%     
============================================
  Files           109      109              
  Lines         61669    61670       +1     
============================================
- Hits          42225    42216       -9     
- Misses        19444    19454      +10

Files	Coverage Δ
src/commands.def	`100.00% <ø> (ø)`
src/cluster.c	`83.64% <77.77%> (-0.13%)`	⬇️
src/cluster_legacy.c	`74.71% <97.10%> (+0.11%)`	⬆️

... and 12 files with indirect coverage changes

hpatro · 2024-05-01T17:49:53Z

Thanks @VoletiRam for the PR.

There were discussion around using CANONICAL / DETERMINISTIC terminology for the filter. I think both of those are confusing from user perspective who is not bothered about the order. Rather I feel TOPOLOGY is more easy to understand where we provide a subset of information regarding the cluster topology. We also initially wanted the command to be named as CLUSTER TOPOLOGY redis/redis#10168.

@valkey-io/core-team Please take a look.

hpatro · 2024-05-01T17:53:09Z

src/cluster_legacy.c

 list *clusterGetNodesInMyShard(clusterNode *node) {
-    sds s = sdsnewlen(node->shard_id, CLUSTER_NAMELEN);
-    dictEntry *de = dictFind(server.cluster->shards,s);
-    sdsfree(s);
-    return (de != NULL) ? dictGetVal(de) : NULL;
+    clusterNode *master = clusterNodeGetMaster(node);
+
+    list *l = listCreate();
+    listAddNodeTail(l, master);
+    for (int i = 0; i < master->numslaves; i++) {
+        listAddNodeTail(l, master->slaves[i]);
+    }
+    return l;
 }


This operation becomes O(N) and some additional memory allocation. However, I'm not bothered a lot about it since the code flow is not on the hot path.

hwware · 2024-05-01T19:35:09Z

Just want to confirm with you @VoletiRam If addling the new parameter "topology" for cluster shards goal is that from every client's view, the output of the 2 masters and 2 replicas nodes is always:

127.0.0.1:6321> cluster shards topology

1. "slots"
2. 1. (integer) 501
  2. (integer) 16383
3. "nodes"
4. 1. 1. "id"
    2. "879df438dd67bd56e717d8400b718c743c4355d1"
    3. "port"
    4. (integer) 6321
      .....
  2. 1. "id"
    2. "4c04d2a090de3e7c9a375ad0ee2e25b52c31c310"
    3. "port"
    4. (integer) 6323
      ......
1. "slots"
2. 1. (integer) 0
  2. (integer) 500
  3. "nodes"
  4. 1. 1. "id"
    2. "af1e6a659a85779864456efe6a323fa7c7eda187"
    3. "port"
    4. (integer) 6320
  5. 1. "id"
    2. "5f4de2e73e92b67c0d4adb0f5737776f5d14e137"
    3. "port"
    4. (integer) 6322

BWT, I am reviewing this PR codes， Thanks

madolson · 2024-05-01T20:03:56Z

src/cluster_legacy.c

@@ -63,6 +63,8 @@ void clusterUpdateState(void);
 int clusterNodeCoversSlot(clusterNode *n, int slot);
 list *clusterGetNodesInMyShard(clusterNode *node);
 int clusterNodeAddSlave(clusterNode *master, clusterNode *slave);
+int clusterNodeAddMaster(clusterNode *master);


I actually don't like this abstraction. I would like it to be conceptually possible for a shard to exist without a master. @PingXie Thoughts?

Could you explain a bit more on how such situation can arise?

I prefer updating this function name as int clusterNodeAddToMasters(clusterNode *node);

I would like it to be conceptually possible for a shard to exist without a master.

This abstraction seems fine from a quick look. Basically, the contract is to keep track of all the primaries in the cluster in a deterministic order. Even if a primary-less shard could exist, the contract would be upheld regardless I think.

This also reminds me that I will need to create a PR to remove all references in code to "m/s".

I will take a closer look at this PR after merging #245

Thank you for taking a look. Will wait until you review the PR.

hpatro · 2024-05-01T20:28:31Z

@hwware The ordering is based on primary node id lexicographically irrespective of the topology filtering. topology filtering helps with removing the state of replication offset/health which can vary across node(s).

VoletiRam · 2024-05-02T01:20:41Z

Thank you @hwware. As @hpatro pointed, the view will be same for both CLUSTER SHARDS and CLUSTER SHARDS TOPOLOGY from every client. The TOPOLOGY option will exclude replication offset and node health status from the shards response. This request aims to eliminate differing fields, facilitating straightforward comparison of the shards response across cluster nodes via simple string matching, thus avoiding the need for parsing.

zuiderkwast · 2024-05-02T08:02:06Z

@PingXie This PR has large changes in cluster_legacy.c. I think we should merge your PR #245 before this one. Please check if you think everything will be different after merging your PR.

barshaul · 2024-05-02T10:39:31Z

src/cluster_legacy.c

-void addShardReplyForClusterShards(client *c, list *nodes) {
-    serverAssert(listLength(nodes) > 0);
-    clusterNode *n = listNodeValue(listFirst(nodes));
+void addShardReplyForClusterShards(client *c, clusterNode* n, int topology) {


minor: clusterNode* n -> clusterNode* primary to clarify that this function expects to receive a primary node

barshaul · 2024-05-02T10:48:59Z

@hpatro @VoletiRam

Does it filter out failed and loading nodes?

It would be ideal if the client's topology map could be solely based on the results of this command, eliminating the need for subsequent checks on the nodes' status.

hwware · 2024-05-02T14:02:02Z

@hwware The ordering is based on primary node id lexicographically irrespective of the topology filtering. topology filtering helps with removing the state of replication offset/health which can vary across node(s).

Thanks for your words. Then according to this rule, sorted by primary node id lexicographically, all clients should get the same view from any node. (At least primary output is same.)

VoletiRam · 2024-05-02T14:28:45Z

@barshaul We are only filtering out fields that contribute to non-deterministic output but not the node's information based on their health status. I think the ask was to eliminate volatile fields that can vary across the clients, at least not clear from discussion in #114. We can help filter out node's information if everyone agrees.

hwware · 2024-05-02T14:19:10Z

src/cluster.c

+                (c->argc == 2 || c->argc == 3))
+    {
+        /* CLUSTER SHARDS [TOPOLOGY] */
+        int topology = 1;


Usually, I set this kind of bool variable default value as 0 (here the variable more close to a bool variable).
But it is not a big issue I think

hwware · 2024-05-02T14:21:30Z

src/commands/cluster-shards.json

@@ -4,15 +4,20 @@
        "complexity": "O(N) where N is the total number of cluster nodes",
        "group": "cluster",
        "since": "7.0.0",
-        "arity": 2,
+        "arity": -2,
        "container": "CLUSTER",
        "function": "clusterCommand",
        "command_flags": [


Because you add one more argument here, you need add one item "history" here, for example

"history": [
[
"2.8.0",
"Added the -2 reply."
]
],

hwware · 2024-05-02T14:48:03Z

src/cluster_legacy.c

-void clusterCommandShards(client *c) {
-    addReplyArrayLen(c, dictSize(server.cluster->shards));
+void clusterCommandShards(client *c, int topology) {
+    serverAssert(server.cluster->nummasters > 0);


I do not think server.cluster->nummasters > 0 checking is must. 0 should work, how do you think?

Thank you very much for reviewing the changes. Will address the suggestions once everyone finishes reviewing the PR.

hpatro · 2024-05-08T22:12:51Z

@PingXie @madolson @barshaul

Few questions which we need to reach consensus on:

With this filtering option introduced, does CLUSTER SHARDS command have any significance in itself?
@barshaul Regarding removing replica(s) from response which are not ready to serve any data, would it be fine to keep the health data? I presume health information doesn't change that often compared to replication-offset.
Should we filter out primaries not serving any slot?

VoletiRam · 2024-05-09T05:32:45Z

Thank you @hpatro for raising the questions that need consensus. I want to add couple of questions as well.

I am checking few scenarios with 2 primaries - 2 replicas in a 4 node cluster with slot coverage on primaries.

If a primary is failed, the replica will never failover as there are not enough votes to win the election (need atleast 2 votes and there are not enough primaries with slot coverage in the cluster that are responsive). The quorum would never reach to mark the primary as failed. Except CLUSTER NODES, nothing indicates about failed/disconnected primary. How do we want the topology response in this scenario?
Let's assume replica failed over a primary. If old primary is still part of cluster but not responsive/failed (which means the role is still primary and marked as fail with no slot coverage), currently in CLUSTER SHARDS response, we show two primaries (old is marked fail and new one is online) but slots coverage will show as empty, which is inaccurate. How do we want the topology response in this scenario?

With my implementation, the slot coverage empty issue in ##5 can be solved as we go over each master from masters list and print corresponding slots, but it will still show old master in the response with empty slots and fail health status unless we decide to filter out either master node if marked fail or master with no slot coverage. Please share your opinion.

PingXie · 2024-05-09T08:15:02Z

With this filtering option introduced, does CLUSTER SHARDS command have any significance in itself?

Not sure if I understand your question. Can you elaborate?

@barshaul Regarding removing replica(s) from response which are not ready to serve any data, would it be fine to keep the health data? I presume health information doesn't change that often compared to replication-offset.

Based on the use case as described in #114 , including health makes sense to me as it is an important factor to consider for the "routing" decision.

Should we filter out primaries not serving any slot?
I would say yes since the uber use case is to facilitate routing decisions in the client.

I am checking few scenarios with 2 primaries - 2 replicas in a 4 node cluster with slot coverage on primaries.

I don't think 2-shard deployments are legit given the current design/implementation. We need to officially support 2-shard clusters first and then it makes sense to discuss the output of cluster shards in this case.

@zuiderkwast should we resurrect the "voting replicas" discussion? redis/redis#12390

hpatro · 2024-05-09T17:07:38Z

With this filtering option introduced, does CLUSTER SHARDS command have any significance in itself?

Not sure if I understand your question. Can you elaborate?

If all clients would prefer using CLUSTER SHARDS TOPOLOGY command, what will be the utility of CLUSTER SHARDS command ? Few alternatives I can think of apart from the above solution:

Deprecate CLUSTER SHARDS command and introduce CLUSTER TOPOLOGY command with the trimmed down version.
Make a breaking change in Valkey 8 and update CLUSTER SHARDS command itself and remove replication-offset from it (I'm not aware of the client adoption and version handling).

hwware · 2024-05-09T19:22:12Z

With this filtering option introduced, does CLUSTER SHARDS command have any significance in itself?

Not sure if I understand your question. Can you elaborate?

If all clients would prefer using CLUSTER SHARDS TOPOLOGY command, what will be the utility of CLUSTER SHARDS command ? Few alternatives I can think of apart from the above solution:

Deprecate CLUSTER SHARDS command and introduce CLUSTER TOPOLOGY command with the trimmed down version.

Make a breaking change in Valkey 8 and update CLUSTER SHARDS command itself and remove replication-offset from it (I'm not aware of the client adoption and version handling).

I do not think we should deprecate CLUSTER SHARDS command. Client need to remember one more command and
the worse case is to update the client side code as well.

Thus my suggestion is:
If client just call CLUSTER SHARDS, the response should be the same behavior as now.
If client call CLUSTER SHARDS TOPOLOGY, the result should be from this PR.

hwware · 2024-05-09T19:31:36Z

Thank you @hpatro for raising the questions that need consensus. I want to add couple of questions as well.

I am checking few scenarios with 2 primaries - 2 replicas in a 4 node cluster with slot coverage on primaries.

If a primary is failed, the replica will never failover as there are not enough votes to win the election (need atleast 2 votes and there are not enough primaries with slot coverage in the cluster that are responsive). The quorum would never reach to mark the primary as failed. Except CLUSTER NODES, nothing indicates about failed/disconnected primary. How do we want the topology response in this scenario?

Let's assume replica failed over a primary. If old primary is still part of cluster but not responsive/failed (which means the role is still primary and marked as fail with no slot coverage), currently in CLUSTER SHARDS response, we show two primaries (old is marked fail and new one is online) but slots coverage will show as empty, which is inaccurate. How do we want the topology response in this scenario?

With my implementation, the slot coverage empty issue in ##5 can be solved as we go over each master from masters list and print corresponding slots, but it will still show old master in the response with empty slots and fail health status unless we decide to filter out either master node if marked fail or master with no slot coverage. Please share your opinion.

In fact. 2 primaries - 2 replicas cluster, 2 primaries - 0 replicas cluster, 2 primaries - 4 replicas cluster are totally different.

Case 1: 2 primaries - 0 replicas cluster. If client set cluster-require-full-coverage as no in conf file, cluster still work even one primary node fail.

Case 2: 2 primaries - 2 replicas cluster: If any primary fails, no vote happens, and replica can failover immediately

Case 3: 2 primaries - 4 replicas cluster: vote happen if any primary fails

So i agree with Ping, let us first support 2-shard clusters first then discuss the output of cluster shards [topology]

zuiderkwast · 2024-05-10T10:26:57Z

I don't think 2-shard deployments are legit given the current design/implementation. We need to officially support 2-shard clusters first and then it makes sense to discuss the output of cluster shards in this case.

@zuiderkwast should we resurrect the "voting replicas" discussion?

@PingXie Whether replicas can vote or whether the cluster has quorum to perform failovers, or even what kind of consensus algorithm is used, should be irrelevant to the clients. (It's even possible to have some external watchdog that performs manual failover.) So let's decouple those discussions from this PR?

Based on the use case as described in #114 , including health makes sense to me as it is an important factor to consider for the "routing" decision.

I don't think clients should make their own decisions about the health of nodes. That's something the cluster does for them. The clients should only be concerned with routing according to what the cluster tells them. For this, there's no need to include shards without slots. Maybe it's better to exclude them, because such nodes are usually going to be taken down or are just being set up and not really ready to be used for pubsub and other stuff clients may want to send to them.

To summarize: I think CLUSTER SHARDS TOPOLOGY should return no more info that what's included in CLUSTER SLOTS. (Just on a different format.)

I do not think we should deprecate CLUSTER SHARDS command. Client need to remember one more command and the worse case is to update the client side code as well.

Thus my suggestion is:
If client just call CLUSTER SHARDS, the response should be the same behavior as now.
If client call CLUSTER SHARDS TOPOLOGY, the result should be from this PR.

I agree with @hwware about this. If clients have started using CLUSTER SHARDS, we can let them do that. Let's not break it.

madolson · 2024-05-10T17:38:41Z

To summarize: I think CLUSTER SHARDS TOPOLOGY should return no more info that what's included in CLUSTER SLOTS. (Just on a different format.)

If we accept this premise, I think we should consider that maybe we are trying to force CLUSTER SHARDS to be something it isn't. CLUSTER SHARDS was supposed to solve three things:

Be more densely encoded for large highly fragmented clusters. I.e., where slots are distributed in such a way that they don't form continuous ranges. This causes cluster slots to print a lot of duplicate information.
Be a more readable and extensible version of cluster nodes for clients that want more information (like health and offset) but out of the text based format of CLUSTER NODES. It was also supposed to be for humans to see the status of a cluster.
Unwind some poor choices we made historically about "preferred" endpoints and make it easier for clients to see the "whole" picture about networking. I don't think SHARDS has really been that successful here.

It seems like we are saying that clients just shouldn't care about all the extra data provided by CLUSTER NODES. In that world, why wouldn't we do something like CLUSTER SLOTS COMPACT that attempts to help CLUSTER SLOTS solve problem 1. We could change the return type from a start and end, to just an array of start/end ranges. Then clients can more easily adopt the new code? In respect to 3, I guess clients already have that baked in, so do we need to fix it?

The asks from @barshaul are basically, "I don't want any more information, I just want to know what slots are healthy and able to be served from". That is what CLUSTER SLOTS does.

So, we can make CLUSTER SHARDS more deterministic (in ordering), it's still a nice property for doing diffs. But I'm not sure I'm convinced anymore that CLUSTER SHARDS TOPOLOGY is a good idea.

zuiderkwast · 2024-05-10T19:43:53Z

Yes (1) was what I meant, but I wasn't completely aware of the background and details.

It seems like the main point of this new CLUSTER SHARDS variant is that it's deterministic, so a you (or a test case) can check that the nodes' views of the cluster is consistent. This isn't the use case for client slot routing. It's rather a use case for test cases and for admins, to check that the cluster converges after adding/removing nodes, slot migrations, etc. If it's deterministic for a healthy cluster even with health info included, then I'm not going to argue against it.

It can be used by clients too, just to save some bytes, but if some clients feels they want more info, they'll just use the full version of the command, or CLUSTER NODES. That can't be helped.

So I guess the question should be: How common or important is it for cluster admins to check that a cluster converges in this way?

(In our own test framework we can solve it in some other way if it's just for us.)

madolson · 2024-05-12T23:48:11Z

So I guess the question should be: How common or important is it for cluster admins to check that a cluster converges in this way?

I've done a fair amount of "diff" between various cluster outputs, and usually have to do some pre-processing to make sure they agree. It would be nice if the node ordering was the same in that case. You could then trivially ignore the fields that are known to be slightly different (replication offset).

madolson · 2024-05-15T15:05:45Z

To make my suggestion about cluster slots more concrete, I'm proposing a change so that the response of cluster slots becomes:

> CLUSTER SLOTS
1) 1) 1) (integer) 0  -- Start of range 1
      2) (integer) 10000 -- Start of range 2
   2) 1) (integer) 5460 -- End of range 1
      2) (integer) 12000 -- End of range 2
   3) 1) "127.0.0.1"
      2) (integer) 30001
      3) "09dbe9720cda62f7865eabc5fd8857c5d2678366"
      4) 1) hostname
         2) "host-1.valkey.example.com"
   4) 1) "127.0.0.1"
      2) (integer) 30004
      3) "821d8ca00d7ccf931ed3ffc7e3db0599d2271abf"
      4) 1) hostname
         2) "host-2.valkey.example.com"
2) 1) 1) (integer) 5461 -- Start of range 1
      2) (integer) 12001 -- Start of range 2
   2) 1) (integer) 9999 -- End of range 1
      2) (integer) 16383 -- End of range 2
   3) 1) "127.0.0.1"
      2) (integer) 30002
      3) "c9d93d9f2c0c524ff34cc11838c2003d8c29e013"
      4) 1) hostname
         2) "host-3.valkey.example.com"
   4) 1) "127.0.0.1"
      2) (integer) 30005
      3) "faadb3eb99009de4ab72ad6b6ed87634c7ee410f"
      4) 1) hostname
         2) "host-4.valkey.example.com"

Besides that, it behaves the exact same cluster CLUSTER SLOTS. Most likely by adding an argument like CLUSTER SLOTS PACKED or something.

zuiderkwast · 2024-05-15T16:33:41Z

@madolson I don't think the reason clients haven't adopted CLUSTER SHARDS (added in 7.0) is that it's hard to parse. The reason is rather that clients want to be backward compatible and support old Redis versions. If we add CLUSTER SLOTS PACKED, it will have the same problem: Clients can only use it if they know the server supports it, and then they still need a fallback for versions that don't.

Once Redis 6.2 and all Redis 6-compatible services are EOL (or about the time of Valkey 9 is released), then all deployments support CLUSTER SHARDS and then we can start expecting clients to switch to CLUSTER SHARDS.

madolson · 2024-05-15T17:35:54Z

I don't think the reason clients haven't adopted CLUSTER SHARDS (added in 7.0) is that it's hard to parse. The reason is rather that clients want to be backward compatible and support old Redis versions. If we add CLUSTER SLOTS PACKED, it will have the same problem: Clients can only use it if they know the server supports it, and then they still need a fallback for versions that don't.

I agree! People don't want to use the CLUSTER SHARDS we implemented, for reasons that Bar mentioned, so if we were to introduce a net new variant CLUSTER SHARDS TOPOLOGY, we are in the same situation of asking clients to move again. I'm saying that maybe CLUSTER SHARDS as a client command was a mistake, the way it was structured was almost entirely to make it more likely to be adopted by a single client, lettuce, since they wanted weird extra information. What I got from this thread though, is we think clients shouldn't be collecting more information, and should just rely on the flat response from CLUSTER SLOTS.

I suppose there is another option. If we implement a client capability functionality, we could make a change that allows clients to "opt-in" to the new format I proposed. If at startup, they send CLIENT CAPA PACKED-SLOTS, then we can respond with packed response type. Clients can check during parse if it's the "integer" or "array" encoded responses, and handle it accordingly.

Once Redis 6.2 and all Redis 6-compatible services are EOL (or about the time of Valkey 9 is released), then all deployments support CLUSTER SHARDS and then we can start expecting clients to switch to CLUSTER SHARDS.

I don't agree this will happen. Lots of people will continue to use old versions because they will be supported.

zuiderkwast · 2024-05-15T20:29:12Z

Regarding this PR: Can we just settle with sorting what can be sorted in CLUSTER SHARDS? No new argument. That's my vote. Then we document what needs to be ignored when comparing the result from two different nodes. That means a doc PR.

madolson · 2024-05-15T21:16:07Z

@VoletiRam @hpatro Can you review what Victor posted in the previous message. Instead of adding a new new command, let's just make the existing version deterministically ordered but not make any changes to arguments.

hpatro · 2024-05-16T23:11:24Z

@VoletiRam @hpatro Can you review what Victor posted in the previous message. Instead of adding a new new command, let's just make the existing version deterministically ordered but not make any changes to arguments.

@VoletiRam As discussed, could we limit the code change(s) to ordering and cleanup the subcommand.

The data can be compared as string for topology update easily if it's primary only cluster(s) or node(s) with load once/use data where replication offset doesn't change often. However, client would like to be generic. We should move the conversation to a separate thread regarding improving CLUSTER SHARDS or CLUSTER SLOTS command.

hpatro approved these changes May 1, 2024

View reviewed changes

madolson reviewed May 1, 2024

View reviewed changes

zuiderkwast requested a review from PingXie May 2, 2024 08:02

barshaul reviewed May 2, 2024

View reviewed changes

hwware reviewed May 2, 2024

View reviewed changes

madolson added the major-decision-pending Needs decision by core team label May 15, 2024

Maintain deterministic order of CLUSTER SHARDS response #411

Are you sure you want to change the base?

Maintain deterministic order of CLUSTER SHARDS response #411

Conversation

VoletiRam commented May 1, 2024

codecov bot commented May 1, 2024 • edited

Codecov Report

hpatro commented May 1, 2024

Choose a reason for hiding this comment

hwware commented May 1, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hpatro commented May 1, 2024

VoletiRam commented May 2, 2024

zuiderkwast commented May 2, 2024

barshaul May 2, 2024 • edited

Choose a reason for hiding this comment

barshaul commented May 2, 2024

hwware commented May 2, 2024 • edited

VoletiRam commented May 2, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hpatro commented May 8, 2024

VoletiRam commented May 9, 2024 • edited

PingXie commented May 9, 2024

hpatro commented May 9, 2024

hwware commented May 9, 2024

hwware commented May 9, 2024

zuiderkwast commented May 10, 2024 • edited

madolson commented May 10, 2024 • edited

zuiderkwast commented May 10, 2024

madolson commented May 12, 2024 • edited

madolson commented May 15, 2024 • edited

zuiderkwast commented May 15, 2024

madolson commented May 15, 2024

zuiderkwast commented May 15, 2024

madolson commented May 15, 2024

hpatro commented May 16, 2024

codecov bot commented May 1, 2024 •

edited

barshaul May 2, 2024 •

edited

hwware commented May 2, 2024 •

edited

VoletiRam commented May 2, 2024 •

edited

VoletiRam commented May 9, 2024 •

edited

zuiderkwast commented May 10, 2024 •

edited

madolson commented May 10, 2024 •

edited

madolson commented May 12, 2024 •

edited

madolson commented May 15, 2024 •

edited