fix: hash rename cmd #196
fix: hash rename cmd #196
Conversation
WalkthroughThis pull request modifies the renaming functionality in the Redis storage layer. The Changes
Sequence Diagram(s)HashesRename FlowsequenceDiagram
participant Client
participant Redis
participant BatchOp
Client->>Redis: Request HashesRename(old_key, new_key)
Redis-->>Redis: Retrieve all field-value pairs for old_key
Redis->>BatchOp: Add operations to create new_key with retrieved pairs
Redis->>BatchOp: Add operation to delete old_key
BatchOp-->>Redis: Commit batch operations
Redis-->>Client: Return success status
HashesRenamenx FlowsequenceDiagram
participant Client
participant Redis
participant BatchOp
Client->>Redis: Request HashesRenamenx(old_key, new_key)
Redis-->>Redis: Check if new_key exists and is stale
alt new_key is valid for renaming
Redis-->>Redis: Retrieve field-value pairs for old_key
Redis->>BatchOp: Add operations to copy data to new_key
Redis->>BatchOp: Add operation to delete old_key
BatchOp-->>Redis: Commit batch operations
Redis-->>Client: Return success status
else new_key exists and is not stale
Redis-->>Client: Return error status
end
Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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
🧹 Nitpick comments (1)
src/storage/src/redis_hashes.cc (1)
1164-1192: Check cross-database rename consistency and memory usage.This logic copies all field-value pairs from the old key to the new key using a single batch, then deletes the old key. While it ensures atomicity, there are two points of concern:
- Cross-DB usage: The code references
new_instas a possible different instance, yet all the read/write operations are performed onthis->db_instead ofnew_inst->GetDB(). If the intent is truly cross-database renaming, consider moving the new key writes tonew_inst->db_for consistency.- Potentially large memory footprint: Storing all field-value pairs in the
fvsvector can be expensive for huge hashes. A more efficient, streaming-based approach might reduce memory usage.Would you like a shell script to scan other rename usage throughout the codebase to ensure consistent cross-database operations?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/storage/src/redis_hashes.cc(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build_on_macos
- GitHub Check: build_on_ubuntu
|
拉 unstable 分支最新代码试下，CI 应该没问题 |
Pull the latest code of the unstable branch and try it, CI should be fine |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/storage/src/redis_hashes.cc (1)
1225-1253:⚠️ Potential issueValidate new key logic in cross-database conditions.
The method takes a
new_instparameter but all operations are performed onthisrather than the provided new instance. If cross-database rename operations are intended, the batch operations should be performed on the appropriate database instance.Consider modifying the implementation to support cross-database operations:
- auto batch = Batch::CreateBatch(this); + auto batch = Batch::CreateBatch(new_inst); // When writing to the new key - batch->Put(kHashesDataCF, new_hashes_data_key.Encode(), internal_value.Encode()); + batch->Put(new_inst->GetHandle(kHashesDataCF), new_hashes_data_key.Encode(), internal_value.Encode()); // When writing metadata for the new key - batch->Put(kMetaCF, base_meta_newkey.Encode(), meta_value); + batch->Put(new_inst->GetHandle(kMetaCF), base_meta_newkey.Encode(), meta_value);
🧹 Nitpick comments (2)
src/storage/src/redis_hashes.cc (2)
1177-1186: Consider optimizing the iteration pattern for better performance.For larger hash tables, the current implementation that loads all field-value pairs into memory before performing operations might be memory-intensive. Consider a streaming approach where you process each field-value pair immediately:
- std::vector<FieldValue> fvs; - // ... code that populates fvs - for (const auto& fv : fvs) { - // process each field-value - } + // Process each field-value during iteration + for (iter->Seek(prefix); iter->Valid() && iter->key().starts_with(prefix); iter->Next()) { + ParsedHashesDataKey parsed_hashes_data_key(iter->key()); + ParsedBaseDataValue parsed_internal_value(iter->value()); + + // Create new entry + HashesDataKey new_hashes_data_key(newkey, version, parsed_hashes_data_key.field()); + batch->Put(kHashesDataCF, new_hashes_data_key.Encode(), iter->value()); + + // Delete old entry + batch->Delete(kHashesDataCF, iter->key()); + }
1190-1190: Consider transaction safety with explicit error handling.The deletion of the original key's metadata should be contingent on the successful writing of all field-value pairs to the new key. While batch operations provide atomicity, it would be good to add explicit error handling around the batch commit.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/storage/src/redis_hashes.cc(3 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
src/storage/src/redis_hashes.cc (4)
src/storage/src/batch.h (2)
CreateBatch(105-110)CreateBatch(105-105)src/storage/src/redis.h (4)
meta_value(341-344)meta_value(341-341)meta_value(354-375)meta_value(354-354)src/storage/src/redis_strings.cc (8)
base_meta_key(1210-1210)base_meta_key(1247-1247)base_meta_key(1294-1294)base_meta_key(1351-1351)base_meta_key(1408-1408)base_meta_key(1471-1471)base_meta_key(1608-1608)base_meta_key(1619-1619)src/storage/src/redis_zsets.cc (2)
base_meta_newkey(1576-1576)base_meta_newkey(1611-1611)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build_on_ubuntu
- GitHub Check: build_on_macos
🔇 Additional comments (2)
src/storage/src/redis_hashes.cc (2)
1164-1190: Implementation of atomic hash rename operation.The new implementation of
HashesRenameproperly implements batch operations to atomically copy all field-value pairs and delete the old key. This is a significant improvement as it ensures consistency throughout the rename operation.
1164-1193:❓ Verification inconclusive
Verify performance impact on large hash tables.
The implementation loads all field-value pairs into memory before performing batch operations. For very large hash tables, this could lead to significant memory usage and potential performance issues.
🏁 Script executed:
#!/bin/bash # Check existing benchmark tests for hash operations rg -A 3 "HashesRename" --include "*.cc" --include "*.h" # Look for memory usage or performance tests rg -A 5 "benchmark" --include "*test*.cc" | grep -i "hash"Length of output: 996
The previous commands encountered errors due to unsupported flags. Please execute the above script to verify if there are any performance or memory benchmarks related to hash operations that could help assess the impact on very large hash tables. Once we have the correct output from this search, we can provide an updated review comment regarding the potential performance impact.
🏁 Script executed:
#!/bin/bash # Re-run benchmark and test searches with corrected ripgrep flags # Search for "HashesRename" occurrences in C/C++ source and header files rg -A 3 "HashesRename" -g "*.cc" -g "*.h" # Search for any benchmark tests related to hash operations in test files rg -A 5 "benchmark" -g "*test*.cc" | grep -i "hash"Length of output: 2080
Action Required: Manually verify performance under heavy-hash workloads.
The current implementation collects all hash fields into an in-memory vector before applying batch operations. Although we located the
HashesRenamecalls, no benchmark tests or automated performance validations were found for hash operations. This raises concerns about memory usage and performance degradation when operating on very large hash tables. Please ensure manual profiling is performed under simulated large-hash workloads to verify that the implementation meets performance requirements. Consider evaluating incremental processing strategies if significant memory overhead is observed.
support hash rename cmd
Summary by CodeRabbit