add cluster flushslot command. #1384
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## unstable #1384 +/- ##
=========================================
Coverage 71.05% 71.06%
=========================================
Files 123 123
Lines 66106 66134 +28
=========================================
+ Hits 46971 46997 +26
- Misses 19135 19137 +2
🚀 New features to boost your workflow:
|
|
Please help review this PR. Thank you. @hwware @madolson @PingXie @zuiderkwast |
src/cluster_legacy.c
Outdated
| addReplyErrorObject(c, shared.syntaxerr); | ||
| return; | ||
| } | ||
| } |
There was a problem hiding this comment.
we should fail the c->argc > 4 case I think
There was a problem hiding this comment.
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);
}There was a problem hiding this comment.
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().
c66cb6a to
ab74efb
Compare
|
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). |
ab74efb to
4db74af
Compare
Fixed. For details, see #1399. |
|
@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? |
|
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 |
@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... |
Yeah that's a good idea. Currently, this is what |
There was a problem hiding this comment.
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
Outdated
| addReplyErrorObject(c, shared.syntaxerr); | ||
| return; | ||
| } | ||
| } |
There was a problem hiding this comment.
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().
src/cluster_legacy.c
Outdated
| 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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
- For FLUSHDB, what are the keyspace notifications sent? A lot of DEL events or one FLUSHDB event?
- In my test, flushdb not trigger any event.
192.168.0.1:2774> set k2 1
OK
192.168.0.1:2774> flushdb
OK
192.168.0.1:2774> PSUBSCRIBE __key*@0__:*
Reading messages... (press Ctrl-C to quit)
1) "psubscribe"
2) "__key*@0__:*"
3) (integer) 1
1) "pmessage"
2) "__key*@0__:*"
3) "__keyspace@0__:k2"
4) "set"
1) "pmessage"
2) "__key*@0__:*"
3) "__keyevent@0__:set"
4) "k2"
- 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.
- yes, we should add some flag to mark is migration or del.
If we want to like flushdb, we should write a new method to implement flushslot logic.
There was a problem hiding this comment.
In my test, flushdb not trigger any event.
That's surprising, but OK. I guess flushslot should behave the same as flushdb. I'm open to other suggestions though. If this was a bad design for flushdb, then we can do something else for flushslot.
If we want to like flushdb, we should write a new method to implement flushslot logic.
Let's to it?
There was a problem hiding this comment.
Okay, I'll try to rewrite the flushslot as per the flushdb implementation.
| if (lazy) { | ||
| dbAsyncDelete(&server.db[0], key); | ||
| } else { | ||
| dbSyncDelete(&server.db[0], key); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
4db74af to
0d751a2
Compare
|
I add command scene to delKeysInSlot. In command sence, delKeysInSlot will not propagate del command, but will notify del event. flushslot now just propagate please help review this PR, thank you! @zuiderkwast @PingXie |
| R 0 CLUSTER FLUSHSLOT $key_slot ASYNC | ||
| assert_equal [R 0 CLUSTER COUNTKEYSINSLOT $key_slot] 0 | ||
| } | ||
| } |
There was a problem hiding this comment.
Can you add a test case to validate the command is indeed replicated as-is, as opposed to the key deletions getting replicated?
12e3d55 to
1744f75
Compare
0b478f3 to
eab7f7c
Compare
Signed-off-by: wuranxx <wuranxx@foxmail.com> Signed-off-by: hwware <wen.hui.ware@gmail.com>
Signed-off-by: wuranxx <wuranxx@foxmail.com> Signed-off-by: hwware <wen.hui.ware@gmail.com>
Signed-off-by: wuranxx <wuranxx@foxmail.com> Signed-off-by: hwware <wen.hui.ware@gmail.com>
Signed-off-by: wuranxx <wuranxx@foxmail.com> Signed-off-by: hwware <wen.hui.ware@gmail.com>
eab7f7c to
cf527a6
Compare
2. add command scene to delKeysInSlot. In command sence, delKeysInSlot will not propagate del command, and notify del event. Signed-off-by: wuranxx <wuranxx@foxmail.com>
Signed-off-by: hwware <wen.hui.ware@gmail.com>
Signed-off-by: hwware <wen.hui.ware@gmail.com>
Signed-off-by: hwware <wen.hui.ware@gmail.com>
Signed-off-by: hwware <wen.hui.ware@gmail.com>
Co-authored-by: Ping Xie <pingxie@outlook.com> Signed-off-by: wuranxx <155042264+wuranxx@users.noreply.github.com> Signed-off-by: hwware <wen.hui.ware@gmail.com>
Co-authored-by: Ping Xie <pingxie@outlook.com> Signed-off-by: wuranxx <155042264+wuranxx@users.noreply.github.com> Signed-off-by: hwware <wen.hui.ware@gmail.com>
Co-authored-by: Ping Xie <pingxie@outlook.com> Signed-off-by: wuranxx <155042264+wuranxx@users.noreply.github.com> Signed-off-by: hwware <wen.hui.ware@gmail.com>
Co-authored-by: Ping Xie <pingxie@outlook.com> Signed-off-by: wuranxx <155042264+wuranxx@users.noreply.github.com> Signed-off-by: hwware <wen.hui.ware@gmail.com>
Co-authored-by: Ping Xie <pingxie@outlook.com> Signed-off-by: wuranxx <155042264+wuranxx@users.noreply.github.com> Signed-off-by: hwware <wen.hui.ware@gmail.com>
Co-authored-by: Ping Xie <pingxie@outlook.com> Signed-off-by: wuranxx <155042264+wuranxx@users.noreply.github.com> Signed-off-by: hwware <wen.hui.ware@gmail.com>
fix `commands.def`. Signed-off-by: wuranxx <wuranxx@foxmail.com>
Signed-off-by: wuranxx <wuranxx@foxmail.com>
cf527a6 to
ddfa7ff
Compare
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:
behavior
Add
CLUSTER FLUSHSLOT <slot> [ASYNC|SYNC]command.Modify the function signature of
delKeysInSlot, adding alazyparameter to decide whether to delete keys SYNC or ASYNC.