Added changes to propogate FLUSHSLOT instead of UNLINK in replication stream in case of ASM slot migration#4191
Added changes to propogate FLUSHSLOT instead of UNLINK in replication stream in case of ASM slot migration#4191omanges wants to merge 2 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughChangesCluster flush-slot propagation
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant ClusterCleanup
participant delKeysInSlot
participant alsoPropagate
participant Keyspace
ClusterCleanup->>delKeysInSlot: delete keys with flush-slot propagation enabled
delKeysInSlot->>alsoPropagate: propagate CLUSTER FLUSHSLOT
delKeysInSlot->>Keyspace: snapshot and delete slot keys
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
… stream in case of ASM slot migration Signed-off-by: Omkar Mestry <om.m.mestry@gmail.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/cluster.h (1)
152-152: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUnify parameter naming between declaration and definition.
The parameter is named
propagate_as_flushslothere in the declaration, but insrc/cluster_legacy.c, it is defined asis_asm_path. Consider aligning the names for consistency.♻️ Proposed refactor
-unsigned int delKeysInSlot(unsigned int hashslot, int lazy, bool propagate_del, bool send_del_event, bool propagate_as_flushslot); +unsigned int delKeysInSlot(unsigned int hashslot, int lazy, bool propagate_del, bool send_del_event, bool is_asm_path);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/cluster.h` at line 152, Rename the mismatched parameter in the definition of delKeysInSlot to propagate_as_flushslot so it matches the declaration in cluster.h; preserve the existing behavior and call-site semantics.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/cluster.h`:
- Line 152: Rename the mismatched parameter in the definition of delKeysInSlot
to propagate_as_flushslot so it matches the declaration in cluster.h; preserve
the existing behavior and call-site semantics.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: c08e2de6-f4ae-4088-90bb-9026d3f6dbd0
📒 Files selected for processing (4)
src/cluster.csrc/cluster.hsrc/cluster_legacy.csrc/cluster_migrateslots.c
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/cluster_legacy.c (2)
7533-7533: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAlign parameter name with header declaration.
The parameter is named
is_asm_pathhere, but it is declared aspropagate_as_flushslotinsrc/cluster.h(as shown in the provided context).Consider renaming it to
propagate_as_flushslotfor consistency. This name is also semantically stronger because it describes the desired propagation behavior rather than leaking the caller's context into the function.♻️ Proposed refactor
-unsigned int delKeysInSlot(unsigned int hashslot, int lazy, bool propagate_del, bool send_del_event, bool is_asm_path) { +unsigned int delKeysInSlot(unsigned int hashslot, int lazy, bool propagate_del, bool send_del_event, bool propagate_as_flushslot) {(Note: If you apply this change, remember to also update the
is_asm_pathreferences on lines 7559 and 7579 within this function).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/cluster_legacy.c` at line 7533, Rename the delKeysInSlot parameter is_asm_path to propagate_as_flushslot to match the cluster.h declaration, and update all references to that parameter within delKeysInSlot.
7511-7512: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMark helper function as
static.Since
propagateAsmFlushSlotis only used internally within this file and isn't exported in the headers, it should be markedstaticto prevent namespace pollution and potentially allow the compiler to inline it.♻️ Proposed refactor
/* Helper function to propagate the CLUSTER FLUSHSLOT command to replicas and AOF during ASM. */ -void propagateAsmFlushSlot(unsigned int hashslot, int lazy) { +static void propagateAsmFlushSlot(unsigned int hashslot, int lazy) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/cluster_legacy.c` around lines 7511 - 7512, Mark the file-local helper function propagateAsmFlushSlot as static in its definition, preserving its existing parameters and behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/cluster_legacy.c`:
- Line 7533: Rename the delKeysInSlot parameter is_asm_path to
propagate_as_flushslot to match the cluster.h declaration, and update all
references to that parameter within delKeysInSlot.
- Around line 7511-7512: Mark the file-local helper function
propagateAsmFlushSlot as static in its definition, preserving its existing
parameters and behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 65bc8f08-b81b-42aa-8d2c-d809751809ff
📒 Files selected for processing (4)
src/cluster.csrc/cluster.hsrc/cluster_legacy.csrc/cluster_migrateslots.c
🚧 Files skipped from review as they are similar to previous changes (3)
- src/cluster.h
- src/cluster_migrateslots.c
- src/cluster.c
|
I was trying to add test but facing issue, so raised changes for review by the time I fix the test, not sure am I using This is an example test I am trying to add :- |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## unstable #4191 +/- ##
============================================
+ Coverage 76.83% 76.85% +0.02%
============================================
Files 162 162
Lines 81433 81499 +66
============================================
+ Hits 62565 62639 +74
+ Misses 18868 18860 -8
🚀 New features to boost your workflow:
|
![valkey-review-bot[bot]](https://avatars.githubusercontent.com/in/3633497?s=60&v=4){kind=small}
| } | ||
| // if is command, skip del propagate | ||
| if (propagate_del) propagateDeletion(db, key, lazy, hashslot); | ||
| if (propagate_del && !is_asm_path) propagateDeletion(db, key, lazy, hashslot); |
There was a problem hiding this comment.
Skipping propagateDeletion() here means the per-key execution units at src/cluster_legacy.c:7560-7571 still flush any replicated module/post-notification writes before the later slot-wide FLUSHSLOT is appended. Notification callbacks are allowed to issue writes (tests/modules/keyspace_events.c:84-98), so an ASM-aware module that writes back into the migrating slot from its del callback will now be replayed on replicas/AOF before the flush and then deleted again by the later FLUSHSLOT, even though it survived on the primary. The old per-key propagation kept the delete and those callback side effects ordered together; tests/unit/moduleapi/cluster.tcl:235-266 covers that invariant for slot-change deletions.
Keep the delete propagation ordered with the notification side effects in this path, or fall back to per-key propagation for ASM cleanup.
There was a problem hiding this comment.
Yeah we need to think this through a bit more:
- If we send CLUSTER FLUSHSLOT after, it will flush everything, even items that were recreated by modules -> desync
- If we send CLUSTER FLUSHSLOT before, we will only flush the items that existed at the start. But if the module recreates a key, that recreated key may also be iterated a second time (since kvstore iterator does not guarantee we don't iterate over newly added items). If the module doesn't recreate on the second call -> desync
I think the best solution is:
- At the beginning, swap in a fresh new hashtable for the keys, expires, etc for that slot in the DB
- At the same time, propagate the CLUSTER FLUSHSLOT
- Now, iterate over each key in the OLD hashtable and trigger the keyspace notifications
- Any recreated keys will land in the NEW hashtable and not be reiterated (don't hit the second issue above)
WDYT?
Another option is to force modules to handle FLUSHSLOT as an event like what we do for FLUSHDB, and not trigger any key-level notifications. But that would require module opt-in, and it may be hard for modules to handle this (e.g. search would need to somehow figure out which keys in the index belong to the slot).
In any case - I think the first option is easier for now.
There was a problem hiding this comment.
Yes, I agree with the suggested solution. Propagating FLUSHSLOT before deleting the keys ensures that any data or metadata created by callback events during key deletion is preserved on replicas rather than being wiped out. I have updated the PR accordingly.
| } | ||
|
|
||
| /* Helper function to propagate the CLUSTER FLUSHSLOT command to replicas and AOF during ASM. */ | ||
| void propagateAsmFlushSlot(unsigned int hashslot, int lazy) { |
There was a problem hiding this comment.
nit: naming
| void propagateAsmFlushSlot(unsigned int hashslot, int lazy) { | |
| void propagateFlushSlot(unsigned int hashslot, int lazy) { |
There was a problem hiding this comment.
Done updated to propagateFlushSlot
| /* Remove all the keys in the specified hash slot. | ||
| * The number of removed items is returned. */ | ||
| unsigned int delKeysInSlot(unsigned int hashslot, int lazy, bool propagate_del, bool send_del_event) { | ||
| unsigned int delKeysInSlot(unsigned int hashslot, int lazy, bool propagate_del, bool send_del_event, bool is_asm_path) { |
There was a problem hiding this comment.
nit: match the naming in cluster.h
| unsigned int delKeysInSlot(unsigned int hashslot, int lazy, bool propagate_del, bool send_del_event, bool is_asm_path) { | |
| unsigned int delKeysInSlot(unsigned int hashslot, int lazy, bool propagate_del, bool send_del_event, bool propagate_as_flushslot) { |
There was a problem hiding this comment.
Done update to propagate_as_flushslot
| if (cleanup_needed) { | ||
| /* Defer cleanup until beforeSleep. */ | ||
| job->post_cleanup_state = state; | ||
| state = SLOT_IMPORT_FINISHED_CLEANING_UP; |
There was a problem hiding this comment.
Note that delKeysNotOwnedByMyself is only triggered on the importing node for cleanup. It is half of the required propagation. The other half will need to be in cluster_legacy.c when we clean up unowned slots on the exporting node.
You will need to check in the dirty slot deletion logic whether or not the dirty slot was exporting or not. If it was exporting, you will want to use FLUSHSLOT.
There was a problem hiding this comment.
Done updated propagate_as_flushslot to true in :-
if (delete_dirty_slots) {
for (int j = 0; j < dirty_slots_count; j++) {
serverLog(LL_NOTICE, "Deleting keys in dirty slot %d on node %.40s (%s) in shard %.40s", dirty_slots[j],
myself->name, humanNodename(myself), myself->shard_id);
delKeysInSlot(dirty_slots[j], server.lazyfree_lazy_server_del, true, false, true);
}
}```
| } | ||
| // if is command, skip del propagate | ||
| if (propagate_del) propagateDeletion(db, key, lazy, hashslot); | ||
| if (propagate_del && !is_asm_path) propagateDeletion(db, key, lazy, hashslot); |
There was a problem hiding this comment.
Yeah we need to think this through a bit more:
- If we send CLUSTER FLUSHSLOT after, it will flush everything, even items that were recreated by modules -> desync
- If we send CLUSTER FLUSHSLOT before, we will only flush the items that existed at the start. But if the module recreates a key, that recreated key may also be iterated a second time (since kvstore iterator does not guarantee we don't iterate over newly added items). If the module doesn't recreate on the second call -> desync
I think the best solution is:
- At the beginning, swap in a fresh new hashtable for the keys, expires, etc for that slot in the DB
- At the same time, propagate the CLUSTER FLUSHSLOT
- Now, iterate over each key in the OLD hashtable and trigger the keyspace notifications
- Any recreated keys will land in the NEW hashtable and not be reiterated (don't hit the second issue above)
WDYT?
Another option is to force modules to handle FLUSHSLOT as an event like what we do for FLUSHDB, and not trigger any key-level notifications. But that would require module opt-in, and it may be hard for modules to handle this (e.g. search would need to somehow figure out which keys in the index belong to the slot).
In any case - I think the first option is easier for now.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/cluster_legacy.c (1)
7571-7596: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winCheck deletion success to prevent TOCTOU state corruption.
Because keys are collected into a snapshot, a key in the snapshot might already be deleted by the time the loop reaches it (e.g., deleted by a module callback triggered by a prior key's deletion in the same loop). Blindly processing the key when
dbSyncDelete/dbAsyncDeletefails to find it will cause duplicate keyspace events, double propagation, and corruptedserver.dirtystatistics.Check the return value of the delete function and only proceed if the key was actually removed.
🐛 Proposed fix
/* 3. Delete the snapshot of collected keys. */ for (unsigned int k = 0; k < keys_count; k++) { robj *key = keys_to_del[k]; enterExecutionUnit(1, 0); - if (lazy) { - dbAsyncDelete(db, key); - } else { - dbSyncDelete(db, key); - } - // if is command, skip del propagate - if (propagate_del && !propagate_as_flushslot) propagateDeletion(db, key, lazy, hashslot); - signalModifiedKey(NULL, db, key); - if (send_del_event) { - /* In the `cluster flushslot` scenario, the keys are actually deleted so notify everyone. */ - notifyKeyspaceEvent(NOTIFY_GENERIC, "del", key, db->id); - } else { - /* 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. */ - moduleNotifyKeyspaceEvent(NOTIFY_GENERIC, "del", key, db->id); - } - exitExecutionUnit(); - postExecutionUnitOperations(); - decrRefCount(key); - j++; - server.dirty++; + int deleted = lazy ? dbAsyncDelete(db, key) : dbSyncDelete(db, key); + if (deleted) { + // if is command, skip del propagate + if (propagate_del && !propagate_as_flushslot) propagateDeletion(db, key, lazy, hashslot); + signalModifiedKey(NULL, db, key); + if (send_del_event) { + /* In the `cluster flushslot` scenario, the keys are actually deleted so notify everyone. */ + notifyKeyspaceEvent(NOTIFY_GENERIC, "del", key, db->id); + } else { + /* 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. */ + moduleNotifyKeyspaceEvent(NOTIFY_GENERIC, "del", key, db->id); + } + j++; + server.dirty++; + } + exitExecutionUnit(); + postExecutionUnitOperations(); + decrRefCount(key); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/cluster_legacy.c` around lines 7571 - 7596, Check the return value of dbSyncDelete or dbAsyncDelete in the deletion loop before performing propagation, key signaling, notifications, reference-count cleanup, incrementing j, or updating server.dirty. Continue processing only when the deletion actually succeeds, while preserving the existing lazy/non-lazy delete selection and execution-unit lifecycle.
🧹 Nitpick comments (1)
src/cluster_legacy.c (1)
7524-7524: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueConsider passing
-1as thedbidforCLUSTER FLUSHSLOT.
CLUSTER FLUSHSLOTis a database-neutral cluster command that flushes the slot across the entire node. Passingdbid = 0toalsoPropagatewill unnecessarily emit aSELECT 0to the replication stream (if the replica is not already on DB 0). Passing-1avoids this.♻️ Proposed refactor
- alsoPropagate(0, argv, argc, PROPAGATE_REPL | PROPAGATE_AOF, hashslot); + alsoPropagate(-1, argv, argc, PROPAGATE_REPL | PROPAGATE_AOF, hashslot);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/cluster_legacy.c` at line 7524, Update the alsoPropagate call in the CLUSTER FLUSHSLOT handling to pass dbid -1 instead of 0, preserving the existing arguments and propagation flags so the database-neutral command does not emit SELECT 0.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/cluster_legacy.c`:
- Around line 7559-7568: Update the keys_to_del snapshot construction in the
kvstoreHashtableIterator loop to grow its allocation dynamically as keys_count
approaches capacity, reallocating before writing each new object. Preserve the
existing key-copying and iterator-release behavior while ensuring insertions
during multi-DB processing cannot write beyond the buffer.
In `@tests/unit/cluster/cluster-migrateslots.tcl`:
- Around line 619-637: Remove the early break from the replication-stream loop
that checks found_flushslot and found_individual_dels. After detecting a
FLUSHSLOT command, continue reading until empty_count reaches 50 so subsequent
DEL or UNLINK commands are still detected.
---
Outside diff comments:
In `@src/cluster_legacy.c`:
- Around line 7571-7596: Check the return value of dbSyncDelete or dbAsyncDelete
in the deletion loop before performing propagation, key signaling,
notifications, reference-count cleanup, incrementing j, or updating
server.dirty. Continue processing only when the deletion actually succeeds,
while preserving the existing lazy/non-lazy delete selection and execution-unit
lifecycle.
---
Nitpick comments:
In `@src/cluster_legacy.c`:
- Line 7524: Update the alsoPropagate call in the CLUSTER FLUSHSLOT handling to
pass dbid -1 instead of 0, preserving the existing arguments and propagation
flags so the database-neutral command does not emit SELECT 0.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 4c54e31f-0bdb-403a-87e1-d9761f677cf7
📒 Files selected for processing (2)
src/cluster_legacy.ctests/unit/cluster/cluster-migrateslots.tcl
| robj **keys_to_del = zmalloc(sizeof(robj*) * num_keys); | ||
| unsigned int keys_count = 0; | ||
|
|
||
| kvstoreHashtableIterator *kvs_di = kvstoreGetHashtableIterator(db->keys, hashslot, HASHTABLE_ITER_SAFE); | ||
| void *next; | ||
| while (kvstoreHashtableIteratorNext(kvs_di, &next)) { | ||
| robj *valkey = next; | ||
| sds sdskey = objectGetKey((robj*)next); | ||
| keys_to_del[keys_count++] = createStringObject(sdskey, sdslen(sdskey)); | ||
| } | ||
| kvstoreReleaseHashtableIterator(kvs_di); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
Prevent buffer overflow when allocating keys_to_del.
The size of keys_to_del is based on num_keys, which is the total key count captured before the multi-DB loop. If a module handles the deletion of a key in db 0 and reacts by creating new keys in db 1, the number of keys collected when iterating db 1 could exceed the original global num_keys, causing a buffer overflow at keys_to_del[keys_count++].
Use dynamic reallocation to safely build the snapshot regardless of mid-flight insertions.
🐛 Proposed fix
- robj **keys_to_del = zmalloc(sizeof(robj*) * num_keys);
+ unsigned int capacity = num_keys > 0 ? num_keys : 16;
+ robj **keys_to_del = zmalloc(sizeof(robj*) * capacity);
unsigned int keys_count = 0;
kvstoreHashtableIterator *kvs_di = kvstoreGetHashtableIterator(db->keys, hashslot, HASHTABLE_ITER_SAFE);
void *next;
while (kvstoreHashtableIteratorNext(kvs_di, &next)) {
+ if (keys_count == capacity) {
+ capacity *= 2;
+ keys_to_del = zrealloc(keys_to_del, sizeof(robj*) * capacity);
+ }
sds sdskey = objectGetKey((robj*)next);
keys_to_del[keys_count++] = createStringObject(sdskey, sdslen(sdskey));
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| robj **keys_to_del = zmalloc(sizeof(robj*) * num_keys); | |
| unsigned int keys_count = 0; | |
| kvstoreHashtableIterator *kvs_di = kvstoreGetHashtableIterator(db->keys, hashslot, HASHTABLE_ITER_SAFE); | |
| void *next; | |
| while (kvstoreHashtableIteratorNext(kvs_di, &next)) { | |
| robj *valkey = next; | |
| sds sdskey = objectGetKey((robj*)next); | |
| keys_to_del[keys_count++] = createStringObject(sdskey, sdslen(sdskey)); | |
| } | |
| kvstoreReleaseHashtableIterator(kvs_di); | |
| unsigned int capacity = num_keys > 0 ? num_keys : 16; | |
| robj **keys_to_del = zmalloc(sizeof(robj*) * capacity); | |
| unsigned int keys_count = 0; | |
| kvstoreHashtableIterator *kvs_di = kvstoreGetHashtableIterator(db->keys, hashslot, HASHTABLE_ITER_SAFE); | |
| void *next; | |
| while (kvstoreHashtableIteratorNext(kvs_di, &next)) { | |
| if (keys_count == capacity) { | |
| capacity *= 2; | |
| keys_to_del = zrealloc(keys_to_del, sizeof(robj*) * capacity); | |
| } | |
| sds sdskey = objectGetKey((robj*)next); | |
| keys_to_del[keys_count++] = createStringObject(sdskey, sdslen(sdskey)); | |
| } | |
| kvstoreReleaseHashtableIterator(kvs_di); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/cluster_legacy.c` around lines 7559 - 7568, Update the keys_to_del
snapshot construction in the kvstoreHashtableIterator loop to grow its
allocation dynamically as keys_count approaches capacity, reallocating before
writing each new object. Preserve the existing key-copying and iterator-release
behavior while ensuring insertions during multi-DB processing cannot write
beyond the buffer.
| set found_flushslot 0 | ||
| set found_individual_dels 0 | ||
| set empty_count 0 | ||
| while {$empty_count < 50} { | ||
| set cmd [read_from_replication_stream $repl] | ||
| if {$cmd eq ""} { | ||
| incr empty_count | ||
| after 100 | ||
| continue | ||
| } | ||
| set empty_count 0 | ||
| if {[string match -nocase "*flushslot*" $cmd]} { | ||
| set found_flushslot 1 | ||
| break | ||
| } | ||
| if {[string match -nocase "*unlink*" $cmd] || [string match -nocase "*del*" $cmd]} { | ||
| set found_individual_dels 1 | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Remove the early break to correctly verify the absence of DEL/UNLINK commands.
Since CLUSTER FLUSHSLOT is propagated before the key deletion loop, breaking the loop immediately upon finding flushslot skips reading the remainder of the replication stream. This renders the test ineffective because found_individual_dels will always remain 0, even if the suppression logic fails and DEL/UNLINK commands are erroneously emitted.
Let the loop consume the entire stream until the timeout is reached (empty_count reaches 50) to guarantee no subsequent deletes are present.
🐛 Proposed fix
while {$empty_count < 50} {
set cmd [read_from_replication_stream $repl]
if {$cmd eq ""} {
incr empty_count
after 100
continue
}
set empty_count 0
if {[string match -nocase "*flushslot*" $cmd]} {
set found_flushslot 1
- break
}
if {[string match -nocase "*unlink*" $cmd] || [string match -nocase "*del*" $cmd]} {
set found_individual_dels 1
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| set found_flushslot 0 | |
| set found_individual_dels 0 | |
| set empty_count 0 | |
| while {$empty_count < 50} { | |
| set cmd [read_from_replication_stream $repl] | |
| if {$cmd eq ""} { | |
| incr empty_count | |
| after 100 | |
| continue | |
| } | |
| set empty_count 0 | |
| if {[string match -nocase "*flushslot*" $cmd]} { | |
| set found_flushslot 1 | |
| break | |
| } | |
| if {[string match -nocase "*unlink*" $cmd] || [string match -nocase "*del*" $cmd]} { | |
| set found_individual_dels 1 | |
| } | |
| } | |
| set found_flushslot 0 | |
| set found_individual_dels 0 | |
| set empty_count 0 | |
| while {$empty_count < 50} { | |
| set cmd [read_from_replication_stream $repl] | |
| if {$cmd eq ""} { | |
| incr empty_count | |
| after 100 | |
| continue | |
| } | |
| set empty_count 0 | |
| if {[string match -nocase "*flushslot*" $cmd]} { | |
| set found_flushslot 1 | |
| } | |
| if {[string match -nocase "*unlink*" $cmd] || [string match -nocase "*del*" $cmd]} { | |
| set found_individual_dels 1 | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/unit/cluster/cluster-migrateslots.tcl` around lines 619 - 637, Remove
the early break from the replication-stream loop that checks found_flushslot and
found_individual_dels. After detecting a FLUSHSLOT command, continue reading
until empty_count reaches 50 so subsequent DEL or UNLINK commands are still
detected.
6ca3060 to
e56c34a
Compare
Signed-off-by: Omkar Mestry <om.m.mestry@gmail.com>
2 participants
Added changes to propogate FLUSHSLOT instead of UNLINK in replication stream in case of ASM slot migration
#3405