fix:When Codis deletes a key, deletion fails for the key in Pika slot…#3241
fix:When Codis deletes a key, deletion fails for the key in Pika slot…#3241chejinge wants to merge 1 commit intoOpenAtomFoundation:unstablefrom
Conversation
📝 WalkthroughWalkthroughThis change enhances the deletion mechanism by introducing type-aware slot cleanup. A pre-pass collects key types during deletion, enabling the use of Changes
Sequence DiagramsequenceDiagram
participant DelCmd as DelCmd::Do()
participant Storage as Storage Layer
participant DB as Database Instance
participant SlotCmd as SlotCommand
DelCmd->>DelCmd: Pre-pass: iterate keys_<br/>GetKeyType(key) for each key
DelCmd->>DelCmd: Build key_type_map<br/>only for keys with type > 0
DelCmd->>Storage: Del(keys_, &key_types)
Storage->>DB: For each key: GetType(key)
DB-->>Storage: Return type info
Storage->>Storage: Collect type info<br/>in key_types
Storage->>DB: Del(key) for each key
DB-->>Storage: Deletion status
Storage-->>DelCmd: Return deletion count
DelCmd->>SlotCmd: For each (key, type)<br/>in key_type_map
SlotCmd->>DB: RemSlotKeyByType(type, key)
DB->>DB: SRem from slot set<br/>and tag set
DB-->>SlotCmd: Status (ignore IsNotFound)
SlotCmd-->>DelCmd: Cleanup complete
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/pika_kv.cc (1)
246-254:⚠️ Potential issue | 🟡 Minor
DelCmd::Splitdoes not perform slot cleanup.The
Splitmethod at lines 246-254 callsdb_->storage()->Del(hint_keys.keys)and incrementssplit_res_, but does not remove keys from slot tracking. TheDo()method explicitly callsRemSlotKeyByType(type, key, db_)for each deleted key (lines 227-229), butSplit()lacks this cleanup entirely.While
Split/Mergeappear to be unused in current execution paths (no invocations found in the codebase), this inconsistency should still be addressed for correctness: either add the slot cleanup toSplit()to matchDo(), or document why it is intentionally omitted.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pika_kv.cc` around lines 246 - 254, DelCmd::Split currently calls db_->storage()->Del(hint_keys.keys) and updates split_res_ or res_ but does not remove deleted keys from slot tracking; update Split to iterate over hint_keys.keys (the same way Do() does) and call RemSlotKeyByType(type, key, db_) for each deleted key so slot metadata is cleaned up, ensuring behavior matches DelCmd::Do; preserve existing error handling (res_.SetRes) and update split_res_ only for successful deletions.
🧹 Nitpick comments (1)
src/storage/include/storage/storage.h (1)
1015-1017: Misleading comment for the newDeloverload.The comment "Return the key exists type count" appears to be a leftover or copy-paste error. This function returns the count of successfully deleted keys, not the count of existing key types. The
key_typesparameter collects type information as a side effect.📝 Suggested comment fix
- // Return the key exists type count - // Del keys and return deleted keys' types + // Removes the specified keys and optionally collects their types before deletion. + // `@param` key_types If non-null, populated with (key, type_tag) pairs for each key. + // `@return` The number of keys that were successfully removed, or -1 on error. int64_t Del(const std::vector<std::string>& keys, std::vector<std::pair<std::string, std::string>>* key_types);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/storage/include/storage/storage.h` around lines 1015 - 1017, Update the misleading comment for the Del overload (int64_t Del(const std::vector<std::string>& keys, std::vector<std::pair<std::string, std::string>>* key_types);) to accurately state that the function deletes the provided keys and returns the count of keys successfully deleted, and that the optional key_types output parameter is populated with the deleted keys' type information as a side effect (not that it returns "key exists type count").
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/pika_kv.cc`:
- Around line 208-229: DelCmd::Do currently does a separate GetKeyType loop then
calls db_->storage()->Del(keys_) which causes a TOCTOU race; replace that
two-step approach by calling the new Storage::Del(keys_, &key_types) overload
(collecting deletion count and types atomically) instead of GetKeyType and
db_->storage()->Del(keys_); then use the returned key_types map to call
RemSlotKeyByType(type, key, db_) for each entry, keep the existing
res_.AppendInteger(count) and s_ = rocksdb::Status::OK() semantics, and remove
the old GetKeyType loop and the separate Del call.
---
Outside diff comments:
In `@src/pika_kv.cc`:
- Around line 246-254: DelCmd::Split currently calls
db_->storage()->Del(hint_keys.keys) and updates split_res_ or res_ but does not
remove deleted keys from slot tracking; update Split to iterate over
hint_keys.keys (the same way Do() does) and call RemSlotKeyByType(type, key,
db_) for each deleted key so slot metadata is cleaned up, ensuring behavior
matches DelCmd::Do; preserve existing error handling (res_.SetRes) and update
split_res_ only for successful deletions.
---
Nitpick comments:
In `@src/storage/include/storage/storage.h`:
- Around line 1015-1017: Update the misleading comment for the Del overload
(int64_t Del(const std::vector<std::string>& keys,
std::vector<std::pair<std::string, std::string>>* key_types);) to accurately
state that the function deletes the provided keys and returns the count of keys
successfully deleted, and that the optional key_types output parameter is
populated with the deleted keys' type information as a side effect (not that it
returns "key exists type count").
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: d1ad9b27-0725-4915-bf65-eb207cd79d43
📒 Files selected for processing (5)
src/pika_kv.ccsrc/pika_slot_command.ccsrc/pstd/CMakeLists.txtsrc/storage/include/storage/storage.hsrc/storage/src/storage.cc
💤 Files with no reviewable changes (1)
- src/pstd/CMakeLists.txt
| void DelCmd::Do() { | ||
| // 先获取所有键的类型 | ||
| std::map<std::string, std::string> key_type_map; | ||
| for (const auto& key : keys_) { | ||
| std::string type; | ||
| int result = GetKeyType(key, type, db_); | ||
| if (result > 0) { | ||
| key_type_map[key] = type; | ||
| } | ||
| } | ||
|
|
||
| // 然后删除键 | ||
| int64_t count = db_->storage()->Del(keys_); | ||
|
|
||
| if (count >= 0) { | ||
| res_.AppendInteger(count); | ||
| s_ = rocksdb::Status::OK(); | ||
| std::vector<std::string>::const_iterator it; | ||
| for (it = keys_.begin(); it != keys_.end(); it++) { | ||
| RemSlotKey(*it, db_); | ||
|
|
||
| // 使用预先获取的类型信息从 slotKey 中移除 | ||
| for (const auto& [key, type] : key_type_map) { | ||
| RemSlotKeyByType(type, key, db_); | ||
| } |
There was a problem hiding this comment.
Race condition between type lookup and deletion; consider using the new Del overload.
There's a TOCTOU (time-of-check-to-time-of-use) race: between GetKeyType (lines 211-217) and Del (line 220), another thread could delete or modify the key, causing:
- A key to be deleted but not removed from the slot (if deleted between GetKeyType and Del)
- Incorrect type being used for slot removal (if key type changed)
Additionally, the new Storage::Del(keys, &key_types) overload (added in this PR) collects types atomically with deletion. Consider using it to eliminate the race and the redundant type lookups:
🔧 Suggested fix using the new Del overload
void DelCmd::Do() {
- // 先获取所有键的类型
- std::map<std::string, std::string> key_type_map;
- for (const auto& key : keys_) {
- std::string type;
- int result = GetKeyType(key, type, db_);
- if (result > 0) {
- key_type_map[key] = type;
- }
- }
-
- // 然后删除键
- int64_t count = db_->storage()->Del(keys_);
+ // Delete keys and collect their types atomically
+ std::vector<std::pair<std::string, std::string>> key_types;
+ int64_t count = db_->storage()->Del(keys_, &key_types);
if (count >= 0) {
res_.AppendInteger(count);
s_ = rocksdb::Status::OK();
- // 使用预先获取的类型信息从 slotKey 中移除
- for (const auto& [key, type] : key_type_map) {
- RemSlotKeyByType(type, key, db_);
+ // Remove from slotKey using the collected type information
+ for (const auto& [key, type] : key_types) {
+ if (!type.empty()) {
+ RemSlotKeyByType(type, key, db_);
+ }
}
} else {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/pika_kv.cc` around lines 208 - 229, DelCmd::Do currently does a separate
GetKeyType loop then calls db_->storage()->Del(keys_) which causes a TOCTOU
race; replace that two-step approach by calling the new Storage::Del(keys_,
&key_types) overload (collecting deletion count and types atomically) instead of
GetKeyType and db_->storage()->Del(keys_); then use the returned key_types map
to call RemSlotKeyByType(type, key, db_) for each entry, keep the existing
res_.AppendInteger(count) and s_ = rocksdb::Status::OK() semantics, and remove
the old GetKeyType loop and the separate Del call.
2 participants
… key.
Summary by CodeRabbit
Bug Fixes
Performance Improvements
Chores