Add CLUSTER FLUSHSLOT command #1384

wuranxx · 2024-12-03T13:51:17Z

Add cluster flushslot command. Closes #1133.

Syntax:

CLUSTER FLUSHSLOT <slot> [ASYNC|SYNC]

Background

We want to implement the ability to SYNC or ASYNC free all data from a single slot. This is useful in two cases:

When a primary loses ownership of a slot, it will temporarily freeze while it synchronously frees the slot.
When doing resharding, you may prefer to forcibly delete all the data from a slot and assign it to another node.

Internal changes

Modify the function signature of delKeysInSlot, adding parameters lazy, propagate_del and send_del_event to control the behavior.

src/cluster_legacy.c

codecov · 2024-12-04T22:06:08Z

Codecov Report

Attention: Patch coverage is 88.00000% with 3 lines in your changes missing coverage. Please review.

Project coverage is 71.11%. Comparing base (374718b) to head (9a2839e).
Report is 18 commits behind head on unstable.

Files with missing lines	Patch %	Lines
src/cluster.c	81.81%	2 Missing ⚠️
src/cluster_legacy.c	92.85%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##           unstable    #1384      +/-   ##
============================================
- Coverage     71.21%   71.11%   -0.11%     
============================================
  Files           122      122              
  Lines         66049    66067      +18     
============================================
- Hits          47038    46984      -54     
- Misses        19011    19083      +72

Files with missing lines	Coverage Δ
src/commands.def	`100.00% <ø> (ø)`
src/cluster_legacy.c	`86.51% <92.85%> (+0.17%)`	⬆️
src/cluster.c	`90.12% <81.81%> (-0.13%)`	⬇️

... and 12 files with indirect coverage changes

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

wuranxx · 2024-12-05T02:44:02Z

Please help review this PR. Thank you. @hwware @madolson @PingXie @zuiderkwast

hwware · 2024-12-05T19:36:15Z

There are too much information in issue #1133. To save some of the reviewer energy, could you pls update the top comment to describe the background why we want to this new command? You could reference PR #1392.

Thanks a lot

PingXie

LGTM overall. Thanks @wuranxx!

src/cluster_legacy.c

tests/unit/cluster/cluster-flush-slot.tcl

wuranxx · 2024-12-06T09:21:21Z

Before I rebased to the latest unstable (old commit is 32f7541) , the Tcl tests and manual command tests ran fine locally.

After I rebased to the latest unstable 6df376d, the command failed when executed locally.

It is possible that changes in the code during this period caused the inconsistent behavior. Until I confirm the cause, I have marked this PR as WIP (Work In Progress).

wuranxx · 2025-01-22T03:22:50Z

Before I rebased to the latest unstable (old commit is 32f7541) , the Tcl tests and manual command tests ran fine locally.

After I rebased to the latest unstable 6df376d, the command failed when executed locally.

It is possible that changes in the code during this period caused the inconsistent behavior. Until I confirm the cause, I have marked this PR as WIP (Work In Progress).

Fixed. For details, see #1399.

zuiderkwast · 2025-01-22T11:59:09Z

@enjoy-binbin @murphyjacob4 Do you want this command for atomic slot migration, for example when a slot migration is cancelled, the importing node can send CLUSTER FLUSHSLOT to its replicas to clean up the slot?

murphyjacob4 · 2025-01-22T20:04:55Z

I think we will have a protocol for cancelling the slot migration, and this will trigger the drop for all slot ranges being migrated locally. But we should be able to use the async delKeysInSlot functionality to implement this

zuiderkwast · 2025-01-23T07:58:32Z

I think we will have a protocol for cancelling the slot migration, and this will trigger the drop for all slot ranges being migrated locally. But we should be able to use the async delKeysInSlot functionality to implement this

@murphyjacob4 The replicas are not involved in the slot migration protocol IIUC. So when a primary cleans up a cancelled slot import, do you just replicate a lot of DEL commands to the replicas? Replicating a single FLUSHSLOT may save some work and bandwidth...

murphyjacob4 · 2025-01-23T18:21:38Z

do you just replicate a lot of DEL commands to the replicas? Replicating a single FLUSHSLOT may save some work and bandwidth...

Yeah that's a good idea. Currently, this is what delKeysInSlot does, and it could result in a lot of bandwidth for big slots. Disabling the replication of the individual keys in delKeysInSlot and replacing with a single FLUSHSLOT makes sense to me. This path is also executed in the dirty slot case, so it won't just be an improvement for atomic slot migration (although I anticipate it will be executed more frequently there).

zuiderkwast

Hi @wuranxx and sorry for the delay.

It looks strait-forward, but there are some details we should think about a bit more. I'm not actually sure what's best to do in all cases, but see my comments below.

src/cluster_legacy.c

zuiderkwast · 2025-01-28T10:18:47Z

src/cluster_legacy.c

+        if (lazy) {
+            dbAsyncDelete(&server.db[0], key);
+        } else {
+            dbSyncDelete(&server.db[0], key);


Here, the deleted keys are replicated just by multiple DEL commands. For comparison, FLUSHDB is replicated as a single FLUSHDB command, which is much less traffic than many DEL commands. If we want to replicate FLUSHSLOT as a single FLUSHSLOT command, then we should probably check that the replicas supports this though. We already have the replica's version stored in c->repl_data->replica_version (REPLCONF VERSION is sent by the replica since 8.0) so we could check that it's >= 8.1 for all connected replicas. It's also fine to skip this. We can do it later as a separate optimization.

Actually I think we don't have to worry about old replicas. This is a new feature and users should use new features only after all nodes have been upgraded.

For atomic slot migration - we will need send some FLUSHSLOT commands implicitly:

If an import doesn't succeed (is failed or cancelled) we need to flush the contents of the staged slots to replicas

If there is an import to a target primary and FLUSHDB is run on that target, the target will need to break this out into FLUSHSLOT commands to avoid the importing slots being flushed

If there is an export happening from some source primary and FLUSHDB is run, we need to break this out into FLUSHSLOT commands (so that the target only flushes the impacted importing slots)

Plus - in the case that a dirty slot is found - we currently delete it by sending individual key deletions. It would make sense to create the FLUSHSLOT command implicitly to improve replication traffic.

Due to the cases of implicit FLUSHSLOT propagation - I would suggest that the "delKeysInSlot" helper method include logic for deciding about if we should propagate as individual deletions or as FLUSHSLOT based on the capabilities of the replicas. I'm sure for many customers, they may be running in a cross-version deployment for some amount of time while they do rolling upgrades

@murphyjacob4

we will need send some FLUSHSLOT commands implicitly

Yes, so do you need any changes here? You can call this function and then you explicitly replicate a FLUSHSLOT command. Or do you want this function to do it?

I think you may want to skip keyspace events and skip replication using DEL commands. With the is_cmd argument you can't disable both. See other comment.

include logic for deciding about if we should propagate as individual deletions or as FLUSHSLOT based on the capabilities of the replicas. I'm sure for many customers, they may be running in a cross-version deployment for some amount of time while they do rolling upgrades

I agree that we should support cross-version deployments, but in this case I'm not worried and I don't think we need to decide based on replica capabilities.

We require that all nodes are upgraded before you start using new features. For example, an old replica wouldn't understand a new datatype, or information like hash field expiration, etc. The FLUSHSLOT command and the atomic slot migration are both new features. If a node has ASM, it also has FLUSHSLOT. If you have an 8.1 replica in your cluster, you shouldn't start using these features just yet. Makes sense?

wuranxx · 2025-04-12T09:02:41Z

I add command scene to delKeysInSlot. In command sence, delKeysInSlot will not propagate del command, but will notify del event.

flushslot now just propagate flushslot command to aof and replica.

please help review this PR, thank you! @zuiderkwast @PingXie

PingXie

Overall LGTM. We should also add official documentation for this new command.

src/cluster_legacy.c

src/commands/cluster-flushslot.json

PingXie · 2025-04-14T01:53:13Z

tests/unit/cluster/cluster-flush-slot.tcl

+        R 0 CLUSTER FLUSHSLOT $key_slot ASYNC
+        assert_equal [R 0 CLUSTER COUNTKEYSINSLOT $key_slot] 0
+    }
+}


Can you add a test case to validate the command is indeed replicated as-is, as opposed to the key deletions getting replicated?

Of course. Added.

madolson · 2025-05-12T15:31:29Z

Weekly meeting notes. We will keep the command as is, but we will document this as primarily an internal and debugging command.

PingXie · 2025-05-13T03:53:10Z

Weekly meeting notes. We will keep the command as is, but we will document this as primarily an internal and debugging command.

I think we discussed two decisions

whether this should be a top level command?

based on the use case described in the issue, this is clearly not a "data access" command meant for the application developers, hence the guidance of keeping it under cluster.

whether it is a good idea to introduce a new command whose name differs from that of an existing command by just one character? cluster flushshot vs cluster flushshots.

I don't remember if we reached a conclusion?

zuiderkwast · 2025-05-13T08:43:47Z

I don't remember if we reached a conclusion?

Thanks Ping for summarizing the discussion. We did reach the conclusion to stick to cluster flushslot, as Madelyn wrote.

Neither of these commands are frequently used so the similarity between the two commands (cluster flushshot vs cluster flushshots) is not a major concern.

hwware · 2025-05-13T13:55:47Z

Weekly meeting notes. We will keep the command as is, but we will document this as primarily an internal and debugging command.

I think we discussed two decisions

whether this should be a top level command?

based on the use case described in the issue, this is clearly not a "data access" command meant for the application developers, hence the guidance of keeping it under cluster.

whether it is a good idea to introduce a new command whose name differs from that of an existing command by just one character? cluster flushshot vs cluster flushshots.

I don't remember if we reached a conclusion?

I forgot if you drop off meeting earlier, we have one conclusion that the new command "cluster flushslot" as an internal command.

hwware · 2025-05-13T15:07:55Z

Weekly meeting notes. We will keep the command as is, but we will document this as primarily an internal and debugging command.

I think we discussed two decisions

whether this should be a top level command?

based on the use case described in the issue, this is clearly not a "data access" command meant for the application developers, hence the guidance of keeping it under cluster.

whether it is a good idea to introduce a new command whose name differs from that of an existing command by just one character? cluster flushshot vs cluster flushshots.

I don't remember if we reached a conclusion?

I forgot if you drop off meeting earlier, we have one conclusion that the new command "cluster flushslot" as an internal command, like cluster-SYNCSLOTS (a new internal command too)

Signed-off-by: wuranxx <wuranxx@foxmail.com>

zuiderkwast

LGTM.

@murphyjacob4 Flush one slot at a time, is that enough for cancelled atomic slot migration? If you have 1000 ongoing slot migrations that are cancelled, will you need to send 1000 flushslot commands to the replicas at the same time?

src/commands/cluster-flushslot.json

murphyjacob4 · 2025-05-20T18:41:09Z

@murphyjacob4 Flush one slot at a time, is that enough for cancelled atomic slot migration? If you have 1000 ongoing slot migrations that are cancelled, will you need to send 1000 flushslot commands to the replicas at the same time?

I think it is fine. ASM actually has a few scenarios for FLUSHSLOT:

If a slot migration is cancelled, each slot within this migration will be removed by the target primary and FLUSHSLOT will be propagated to the target replica(s)
If FLUSHSLOT is run on the source primary, it will be sent to the target primary (via slot migration link) foreach ongoing slot export on that node (if any). Target primary will forward this to the target replicas.
If FLUSHSLOT is run on the target primary (by user), it will be denied if the slot is being imported (should be covered by this CL I think, will return -MOVED)
If FLUSHDB is run on the source primary, if there is a slot export occurring, it will replicate as a FLUSHSLOT command for each owned slot. For all target primaries and their replicas, the FLUSHSLOT commands that match the ongoing slot migration will be propagated via the path in 2
If FLUSHDB is run on the target primary, if there is a slot import occurring, it will replicate as a FLUSHSLOT command for each owned slot. Locally, we will leave the importing data. We will not replicate a FLUSHSLOT for the ongoing slot import (since we want to keep that data

In the worst case, it would be something like ~700KiB of replication data to send 16384 FLUSHSLOT commands (each would be <= 43 bytes, *3\r\n$7\r\nCLUSTER\r\n$9\r\nFLUSHSLOT\r\n$5\r\n16383). But this seems like a very rare case. We have a path to optimizing with something like CLUSTER FLUSHSLOTSRANGE a la CLUSTER ADDSLOTSRANGE, but I don't think it is required, and just FLUSHSLOT should better than propagating DEL for each key (at least for 99% of use cases)

zuiderkwast · 2025-05-21T08:22:09Z

If FLUSHDB is run on the source primary, if there is a slot export occurring, it will replicate as a FLUSHSLOT command for each owned slot. For all target primaries and their replicas, the FLUSHSLOT commands that match the ongoing slot migration will be propagated via the path in 2

If FLUSHDB is run on the target primary, if there is a slot import occurring, it will replicate as a FLUSHSLOT command for each owned slot. Locally, we will leave the importing data. We will not replicate a FLUSHSLOT for the ongoing slot import (since we want to keep that data

@murphyjacob4 Now that we have multi-db in cluster mode, FLUSHSLOT will affects all databases. FLUSHDB should only flush the currently SELECT'ed database.

Signed-off-by: wuranxx <wuranxx@foxmail.com>

hwware · 2025-05-23T18:24:43Z

@murphyjacob4 Flush one slot at a time, is that enough for cancelled atomic slot migration? If you have 1000 ongoing slot migrations that are cancelled, will you need to send 1000 flushslot commands to the replicas at the same time?

I think it is fine. ASM actually has a few scenarios for FLUSHSLOT:

If a slot migration is cancelled, each slot within this migration will be removed by the target primary and FLUSHSLOT will be propagated to the target replica(s)

If FLUSHSLOT is run on the source primary, it will be sent to the target primary (via slot migration link) foreach ongoing slot export on that node (if any). Target primary will forward this to the target replicas.

If FLUSHSLOT is run on the target primary (by user), it will be denied if the slot is being imported (should be covered by this CL I think, will return -MOVED)

If FLUSHDB is run on the source primary, if there is a slot export occurring, it will replicate as a FLUSHSLOT command for each owned slot. For all target primaries and their replicas, the FLUSHSLOT commands that match the ongoing slot migration will be propagated via the path in 2

If FLUSHDB is run on the target primary, if there is a slot import occurring, it will replicate as a FLUSHSLOT command for each owned slot. Locally, we will leave the importing data. We will not replicate a FLUSHSLOT for the ongoing slot import (since we want to keep that data

In the worst case, it would be something like ~700KiB of replication data to send 16384 FLUSHSLOT commands (each would be <= 43 bytes, *3\r\n$7\r\nCLUSTER\r\n$9\r\nFLUSHSLOT\r\n$5\r\n16383). But this seems like a very rare case. We have a path to optimizing with something like CLUSTER FLUSHSLOTSRANGE a la CLUSTER ADDSLOTSRANGE, but I don't think it is required, and just FLUSHSLOT should better than propagating DEL for each key (at least for 99% of use cases)

We have consensus that flushslot is an internal command only, I think case 2 and 3 will not happen. How do you think?

zuiderkwast · 2025-05-23T18:42:55Z

We have consensus that flushslot is an internal command only, I think case 2 and 3 will not happen. How do you think?

Agree, but I'm not sure about case 4 and 5. Maybe we need CLUSTER FLUSHSLOT 2333 DB 2 to handle FLUSHDB on one db during atomic slot migration. It can be extended later if we need it. Otherwise, we could change flushdb so it only affects owned slots?

Signed-off-by: wuranxx <wuranxx@foxmail.com>

hwware · 2025-05-26T16:06:00Z

We have consensus that flushslot is an internal command only, I think case 2 and 3 will not happen. How do you think?

Agree, but I'm not sure about case 4 and 5. Maybe we need CLUSTER FLUSHSLOT 2333 DB 2 to handle FLUSHDB on one db during atomic slot migration. It can be extended later if we need it. Otherwise, we could change flushdb so it only affects owned slots?

I think case 4 is a valid scenario. However, for case 5, it depends on how the operator does. One worst case is causing data inconsistent. (source primary is exporting data, and FLUSHDB command runs target primary, then some data will lost)

soloestoy · 2025-05-28T01:57:13Z

For the rollback of atomic slot migration, using FLUSHSLOT to affect all databases is more appropriate. However, considering the replication of FLUSHDB during migration and the possibility that users might need to clear a specific slot in a particular database, I think CLUSTER FLUSHSLOT <slotid> [DB id] would be a good option.

hwware · 2025-05-28T15:37:23Z

For the rollback of atomic slot migration, using FLUSHSLOT to affect all databases is more appropriate. However, considering the replication of FLUSHDB during migration and the possibility that users might need to clear a specific slot in a particular database, I think CLUSTER FLUSHSLOT <slotid> [DB id] would be a good option.

I think the proper command format as below:

CLUSTER FLUSHSLOT [[DB id] | all] [ASYNC|SYNC]

if there is no any argument after , this command will only apply current db, it is aligned with FLUSHDB
if the command as CLUSTER FLUSHSLOT [DB id], it will only remove specific db keys
if command as CLUSTER FLUSHSLOT all, it will remove keys from all dbs

I add this PR in our next Monday meeting agenda.

madolson · 2025-06-04T20:45:04Z

@valkey-io/core-team Shouldn't this have had a major decision tag? I remember we decided this was an internal command, but it doesn't look like it was really restricted to just be internal.

zuiderkwast · 2025-06-05T00:35:15Z

The issue has the major decision tag. Seemed enough...(?)

it doesn't look like it was really restricted to just be internal.

If we don't add a markdown file for it in the docs, it will not show up in the docs, but maybe it will show up somewhere else? How can we make it completely internal?

Add cluster flushslot command. Closes valkey-io#1133. Syntax: CLUSTER FLUSHSLOT <slot> [ASYNC|SYNC] ## Background We want to implement the ability to SYNC or ASYNC free all data from a single slot. This is useful in two cases: 1. When a primary loses ownership of a slot, it will temporarily freeze while it synchronously frees the slot. 2. When doing resharding, you may prefer to forcibly delete all the data from a slot and assign it to another node. ## Internal changes Modify the function signature of `delKeysInSlot`, adding parameters `lazy`, `propagate_del` and `send_del_event` to control the behavior. --------- Signed-off-by: wuranxx <wuranxx@foxmail.com> Signed-off-by: hwware <wen.hui.ware@gmail.com> Signed-off-by: wuranxx <155042264+wuranxx@users.noreply.github.com> Co-authored-by: hwware <wen.hui.ware@gmail.com> Co-authored-by: Ping Xie <pingxie@outlook.com> Signed-off-by: chzhoo <czawyx@163.com>

Add cluster flushslot command. Closes valkey-io#1133. Syntax: CLUSTER FLUSHSLOT <slot> [ASYNC|SYNC] ## Background We want to implement the ability to SYNC or ASYNC free all data from a single slot. This is useful in two cases: 1. When a primary loses ownership of a slot, it will temporarily freeze while it synchronously frees the slot. 2. When doing resharding, you may prefer to forcibly delete all the data from a slot and assign it to another node. ## Internal changes Modify the function signature of `delKeysInSlot`, adding parameters `lazy`, `propagate_del` and `send_del_event` to control the behavior. --------- Signed-off-by: wuranxx <wuranxx@foxmail.com> Signed-off-by: hwware <wen.hui.ware@gmail.com> Signed-off-by: wuranxx <155042264+wuranxx@users.noreply.github.com> Co-authored-by: hwware <wen.hui.ware@gmail.com> Co-authored-by: Ping Xie <pingxie@outlook.com> Signed-off-by: shanwan1 <shanwan1@intel.com>

zuiderkwast reviewed Dec 3, 2024

View reviewed changes

src/cluster_legacy.c Outdated Show resolved Hide resolved

wuranxx changed the title ~~WIP: add cluster flushslot command.~~ add cluster flushslot command. Dec 5, 2024

PingXie reviewed Dec 5, 2024

View reviewed changes

src/cluster_legacy.c Outdated Show resolved Hide resolved

tests/unit/cluster/cluster-flush-slot.tcl Show resolved Hide resolved

wuranxx force-pushed the cluster-flush-slot branch from c66cb6a to ab74efb Compare December 6, 2024 08:24

wuranxx changed the title ~~add cluster flushslot command.~~ WIP: add cluster flushslot command. Dec 6, 2024

wuranxx mentioned this pull request Dec 6, 2024

[BUG]bio lazy free object crash in unstable branch. #1399

Closed

wuranxx force-pushed the cluster-flush-slot branch from ab74efb to 4db74af Compare January 22, 2025 03:20

wuranxx changed the title ~~WIP: add cluster flushslot command.~~ add cluster flushslot command. Jan 22, 2025

wuranxx requested review from PingXie and zuiderkwast January 23, 2025 07:53

zuiderkwast reviewed Jan 28, 2025

View reviewed changes

madolson moved this from Todo to Optional for next release candidate in Valkey 9.0 Mar 31, 2025

wuranxx force-pushed the cluster-flush-slot branch from 4db74af to 0d751a2 Compare April 12, 2025 08:52

wuranxx requested a review from zuiderkwast April 12, 2025 09:02

PingXie added the documentation label Apr 14, 2025

PingXie reviewed Apr 14, 2025

View reviewed changes

zuiderkwast added needs-doc-pr This change needs to update a documentation page. Remove label once doc PR is open. and removed documentation labels Apr 14, 2025

hwware force-pushed the cluster-flush-slot branch 2 times, most recently from c76db56 to 39d045b Compare April 14, 2025 16:02

Merge branch 'unstable' into cluster-flush-slot

245f1d2

wuranxx added 2 commits May 20, 2025 19:59

Merge branch 'unstable' into cluster-flush-slot

4742800

add propagate_del and send_del_event parameters to delKeysInSlot.

e37a8c2

Signed-off-by: wuranxx <wuranxx@foxmail.com>

zuiderkwast approved these changes May 20, 2025

View reviewed changes

src/commands/cluster-flushslot.json Outdated Show resolved Hide resolved

fix code format.

e7c9be7

Signed-off-by: wuranxx <wuranxx@foxmail.com>

zuiderkwast added release-notes This issue should get a line item in the release notes to-be-merged Almost ready to merge labels May 25, 2025

wuranxx and others added 2 commits May 26, 2025 20:37

Merge branch 'valkey-io:unstable' into cluster-flush-slot

62593e4

Remove since from cluster-flushslot options definition.

9a2839e

Signed-off-by: wuranxx <wuranxx@foxmail.com>

hwware approved these changes May 26, 2025

View reviewed changes

zuiderkwast changed the title ~~add cluster flushslot command.~~ Add CLUSTER FLUSHSLOT command May 27, 2025

hwware removed the to-be-merged Almost ready to merge label Jun 2, 2025

hwware merged commit 992c5b9 into valkey-io:unstable Jun 2, 2025
52 checks passed

github-project-automation bot moved this from Optional for next release candidate to Done in Valkey 9.0 Jun 2, 2025

Add CLUSTER FLUSHSLOT command #1384

Add CLUSTER FLUSHSLOT command #1384

Uh oh!

Conversation

Uh oh!

Background

Internal changes

Uh oh!

Uh oh!

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!