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

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

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

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 <[email protected]>

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 <[email protected]>

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 <[email protected]>

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 <[email protected]> Signed-off-by: hwware <[email protected]> Signed-off-by: wuranxx <[email protected]> Co-authored-by: hwware <[email protected]> Co-authored-by: Ping Xie <[email protected]> Signed-off-by: chzhoo <[email protected]>

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 <[email protected]> Signed-off-by: hwware <[email protected]> Signed-off-by: wuranxx <[email protected]> Co-authored-by: hwware <[email protected]> Co-authored-by: Ping Xie <[email protected]> Signed-off-by: shanwan1 <[email protected]>

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

src/cluster_legacy.c Outdated Show resolved Hide resolved

src/cluster_legacy.c Outdated Show resolved Hide resolved

src/cluster_legacy.c Outdated Show resolved Hide resolved

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 <[email protected]>

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 <[email protected]>

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 <[email protected]>

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

wuranxx commented Dec 3, 2024 • edited by zuiderkwast Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Background

Internal changes

Uh oh!

Uh oh!

codecov bot commented Dec 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

wuranxx commented Dec 5, 2024

Uh oh!

hwware commented Dec 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

PingXie left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

wuranxx commented Dec 6, 2024

Uh oh!

wuranxx commented Jan 22, 2025

Uh oh!

zuiderkwast commented Jan 22, 2025

Uh oh!

murphyjacob4 commented Jan 22, 2025

Uh oh!

zuiderkwast commented Jan 23, 2025

Uh oh!

murphyjacob4 commented Jan 23, 2025

Uh oh!

zuiderkwast left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

wuranxx commented Apr 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

PingXie left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

madolson commented May 12, 2025

Uh oh!

PingXie commented May 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zuiderkwast commented May 13, 2025

Uh oh!

hwware commented May 13, 2025

Uh oh!

hwware commented May 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zuiderkwast left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

murphyjacob4 commented May 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zuiderkwast commented May 21, 2025

Uh oh!

hwware commented May 23, 2025

Uh oh!

zuiderkwast commented May 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

wuranxx commented Dec 3, 2024 •

edited by zuiderkwast

Loading

codecov bot commented Dec 4, 2024 •

edited

Loading

hwware commented Dec 5, 2024 •

edited

Loading

wuranxx commented Apr 12, 2025 •

edited

Loading

PingXie commented May 13, 2025 •

edited

Loading

hwware commented May 13, 2025 •

edited

Loading

murphyjacob4 commented May 20, 2025 •

edited

Loading

zuiderkwast commented May 23, 2025 •

edited

Loading

madolson commented Jun 4, 2025 •

edited

Loading