add cluster flushslot command. #1384

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

add cluster flushslot command. #1133

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.

behavior

Add CLUSTER FLUSHSLOT <slot> [ASYNC|SYNC] command.
Modify the function signature of delKeysInSlot, adding a lazy parameter to decide whether to delete keys SYNC or ASYNC.

src/cluster_legacy.c

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

Codecov Report

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

Project coverage is 70.98%. Comparing base (7fc958d) to head (4db74af).

Files with missing lines	Patch %	Lines
src/cluster_legacy.c	86.36%	3 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##           unstable    #1384      +/-   ##
============================================
+ Coverage     70.80%   70.98%   +0.17%     
============================================
  Files           121      121              
  Lines         65132    65149      +17     
============================================
+ Hits          46118    46243     +125     
+ Misses        19014    18906     -108

Files with missing lines	Coverage Δ
src/commands.def	`100.00% <ø> (ø)`
src/cluster_legacy.c	`86.93% <86.36%> (-0.42%)`	⬇️

... and 11 files with indirect coverage changes

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!

PingXie · 2024-12-05T20:35:31Z

src/cluster_legacy.c

+            addReplyErrorObject(c, shared.syntaxerr);
+            return;
+        }
+    }


we should fail the c->argc > 4 case I think

In the call site of clusterCommandFlushslot, I have already performed a check on the number of parameters. Is it still necessary to validate the number of parameters again within the function?

else if (!strcasecmp(c->argv[1]->ptr, "flushslot") && (c->argc == 3 || c->argc == 4)) { /* CLUSTER FLUSHSLOT <slot> [ASYNC|SYNC] */ clusterCommandFlushslot(c); }

It's a bit weird that the arity check is split into multiple places. But I don't mind too much because it's like that for many commands already.

But I think this should be in cluster.c instead of cluster_legacy.c. Cluster_legacy is about the cluster bus, and we would replace this file if we replace the cluster bus for cluster V2. At least, that's the idea. This command doesn't use the cluster bus at all, so I think it should be in cluster.c and be called directly from clusterCommand().

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

Signed-off-by: wuranxx <[email protected]>

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.

zuiderkwast · 2025-01-28T09:56:59Z

src/cluster_legacy.c

+            addReplyErrorObject(c, shared.syntaxerr);
+            return;
+        }
+    }


It's a bit weird that the arity check is split into multiple places. But I don't mind too much because it's like that for many commands already.

But I think this should be in cluster.c instead of cluster_legacy.c. Cluster_legacy is about the cluster bus, and we would replace this file if we replace the cluster bus for cluster V2. At least, that's the idea. This command doesn't use the cluster bus at all, so I think it should be in cluster.c and be called directly from clusterCommand().

zuiderkwast · 2025-01-28T10:11:20Z

src/cluster_legacy.c

        dbDelete(&server.db[0], key);
-        propagateDeletion(&server.db[0], key, server.lazyfree_lazy_server_del);
+        propagateDeletion(&server.db[0], key, lazy);
        signalModifiedKey(NULL, &server.db[0], key);
        /* The keys are not actually logically deleted from the database, just moved to another node.
         * The modules needs to know that these keys are no longer available locally, so just send the


This code comment about keyspace notifications:

/* The keys are not actually logically deleted from the database, just moved to another node. * The modules needs to know that these keys are no longer available locally, so just send the * keyspace notification to the modules, but not to clients. */

delKeysInSlot is used for slot migrations, which are considered not deleting the keys but just migrating them to another node.

This is not necessarily the case for CLUSTER FLUSHSLOT. I think we should treat CLUSTER FLUSHSLOT more like FLUSHDB. For FLUSHDB, what are the keyspace notifications sent? A lot of DEL events or one FLUSHDB event?

Maybe we shouldn't use delKeysInSlot here, or use some extra argument or flag to indicate if it's a migration or if we're just deleting the keys.

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.

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

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 added 4 commits January 22, 2025 10:44

add cluster flushslot command.

d9978a2

Signed-off-by: wuranxx <[email protected]>

Add TCL tests and optimize code structure.

e138b7e

Signed-off-by: wuranxx <[email protected]>

Fix compile warning.

da2ab1a

Signed-off-by: wuranxx <[email protected]>

Fix delKeysInSlot does not consider command invoke.

4db74af

Signed-off-by: wuranxx <[email protected]>

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add cluster flushslot command. #1384

add cluster flushslot command. #1384

wuranxx commented Dec 3, 2024 •

edited

Loading

codecov bot commented Dec 4, 2024 •

edited

Loading

wuranxx commented Dec 5, 2024

hwware commented Dec 5, 2024 •

edited

Loading

PingXie left a comment

PingXie Dec 5, 2024

wuranxx Dec 6, 2024

zuiderkwast Jan 28, 2025

wuranxx commented Dec 6, 2024

wuranxx commented Jan 22, 2025

zuiderkwast commented Jan 22, 2025

murphyjacob4 commented Jan 22, 2025

zuiderkwast commented Jan 23, 2025

murphyjacob4 commented Jan 23, 2025

zuiderkwast left a comment

zuiderkwast Jan 28, 2025

zuiderkwast Jan 28, 2025

zuiderkwast Jan 28, 2025

add cluster flushslot command. #1384

Are you sure you want to change the base?

add cluster flushslot command. #1384

Conversation

wuranxx commented Dec 3, 2024 • edited Loading

background

behavior

codecov bot commented Dec 4, 2024 • edited Loading

Codecov Report

wuranxx commented Dec 5, 2024

hwware commented Dec 5, 2024 • edited Loading

PingXie left a comment

Choose a reason for hiding this comment

PingXie Dec 5, 2024

Choose a reason for hiding this comment

wuranxx Dec 6, 2024

Choose a reason for hiding this comment

zuiderkwast Jan 28, 2025

Choose a reason for hiding this comment

wuranxx commented Dec 6, 2024

wuranxx commented Jan 22, 2025

zuiderkwast commented Jan 22, 2025

murphyjacob4 commented Jan 22, 2025

zuiderkwast commented Jan 23, 2025

murphyjacob4 commented Jan 23, 2025

zuiderkwast left a comment

Choose a reason for hiding this comment

zuiderkwast Jan 28, 2025

Choose a reason for hiding this comment

zuiderkwast Jan 28, 2025

Choose a reason for hiding this comment

zuiderkwast Jan 28, 2025

Choose a reason for hiding this comment

wuranxx commented Dec 3, 2024 •

edited

Loading

codecov bot commented Dec 4, 2024 •

edited

Loading

hwware commented Dec 5, 2024 •

edited

Loading