fix: PkPatternMatchDel inconsistent between rediscache and db #2777
Conversation
WalkthroughThe updates introduce enhanced functionality for pattern-matching deletions in the Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant PKPatternMatchDelCmd
participant Storage
participant Redis
participant Cache
Client->>PKPatternMatchDelCmd: Execute()
PKPatternMatchDelCmd->>Storage: PKPatternMatchDelWithRemoveKeys(pattern, max_count)
Storage->>Redis: PKPatternMatchDelWithRemoveKeys(pattern, max_count)
Redis-->>Storage: Return removed keys and count
Storage-->>PKPatternMatchDelCmd: Return removed keys and count
PKPatternMatchDelCmd->>Cache: DoUpdateCache(removed keys)
PKPatternMatchDelCmd-->>Client: Return result
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 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 as PR comments)
Additionally, you can add 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: 4
Outside diff range and nitpick comments (5)
src/storage/src/redis_strings.cc (3)
1768-1768: Add method documentation.Consider adding a docstring to describe the purpose, parameters, and return values of the method
PKPatternMatchDelWithRemoveKeys.
1782-1782: Consider using a more descriptive variable name.The variable name
itercould be more descriptive to improve readability.
1791-1791: Avoid using magic numbers.Replace the
0inStringMatchcall with a named constant or define its meaning.src/pika_admin.cc (2)
3135-3147: Consider adding comments for clarity.The logic in the
Domethod is correct, but adding comments would improve readability and maintainability.void PKPatternMatchDelCmd::Do() { int ret = 0; // Perform pattern match delete and get the keys to be removed int64_t count = db_->storage()->PKPatternMatchDelWithRemoveKeys(type_, pattern_, &remove_keys_); if (count >= 0) { res_.AppendInteger(count); s_ = rocksdb::Status::OK(); // Iterate over the keys to remove and perform slot key removal for (const auto& key : remove_keys_) { RemSlotKey(key, db_); } } else { res_.SetRes(CmdRes::kErrOther, "delete error"); s_ = rocksdb::Status::Corruption("delete error"); } }
3153-3155: Consider adding comments for clarity.The logic in the
DoUpdateCachemethod is correct, but adding comments would improve readability and maintainability.void PKPatternMatchDelCmd::DoUpdateCache() { // Check if the status is OK before updating the cache if(s_.ok()) { // Delete the keys from the cache db_->cache()->Del(remove_keys_); } }
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (6)
- include/pika_admin.h (1 hunks)
- src/pika_admin.cc (1 hunks)
- src/storage/include/storage/storage.h (1 hunks)
- src/storage/src/redis.h (1 hunks)
- src/storage/src/redis_strings.cc (1 hunks)
- src/storage/src/storage.cc (1 hunks)
Additional comments not posted (7)
include/pika_admin.h (3)
469-469: Ensure proper implementation ofDoThroughDB().The
DoThroughDB()method is added but its implementation is not shown in this file. Ensure this method is correctly implemented in the corresponding source file.
470-470: Ensure proper implementation ofDoUpdateCache().The
DoUpdateCache()method is added but its implementation is not shown in this file. Ensure this method is correctly implemented in the corresponding source file.
476-476: Review the usage ofremove_keys_.The
remove_keys_member variable is added to store keys that need to be removed. Ensure this variable is used consistently and correctly in the implementation.src/storage/src/redis.h (1)
193-193: Ensure the correctness ofPKPatternMatchDelWithRemoveKeysimplementation.The
PKPatternMatchDelWithRemoveKeysmethod is added to theRedisclass. Ensure this method is correctly implemented in the corresponding source file.src/storage/include/storage/storage.h (1)
990-991: LGTM! Verify the method usage in the codebase.The addition of the method
PKPatternMatchDelWithRemoveKeyslooks good. Ensure that all usages of this method are correctly implemented.Verification successful
Verify the method implementation and usage
The method
PKPatternMatchDelWithRemoveKeysis correctly defined and used in the codebase. Here are the relevant locations:
Definition:
src/storage/include/storage/storage.h: Line 990src/storage/src/storage.cc: Line 991Usage:
src/pika_admin.cc: Line 991src/storage/src/redis.h: Line 991src/storage/src/redis_strings.cc: Line 991src/storage/src/storage.cc: Line 991Please ensure that all usages of this method are correctly implemented.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all usages of the method `PKPatternMatchDelWithRemoveKeys` in the codebase. # Test: Search for the method usage. Expect: Correct usage in the codebase. rg --type cpp --type c++ $'PKPatternMatchDelWithRemoveKeys'Length of output: 86
Script:
#!/bin/bash # Description: Verify all usages of the method `PKPatternMatchDelWithRemoveKeys` in the codebase. # Test: Search for the method usage. Expect: Correct usage in the codebase. rg --type cpp $'PKPatternMatchDelWithRemoveKeys'Length of output: 895
src/storage/src/storage.cc (1)
1402-1412: Verify error handling and performance.The
PKPatternMatchDelWithRemoveKeysmethod accumulates counts and removes keys, but it should handle errors more explicitly. If an instance operation fails, it currently continues without reporting the error. Consider adding error handling to ensure robustness.Additionally, the method iterates through all instances, which may impact performance. Ensure that this method is optimized for large datasets.
int64_t Storage::PKPatternMatchDelWithRemoveKeys(const DataType& data_type, const std::string& pattern, std::vector<std::string>* remove_keys) { Status s; int64_t count = 0; for (const auto& inst : insts_) { int64_t tmp_ret = 0; s = inst->PKPatternMatchDelWithRemoveKeys(pattern, &tmp_ret, remove_keys); if (!s.ok()) { // Handle error appropriately, e.g., log the error, return early, etc. // LOG(ERROR) << "Error in PKPatternMatchDelWithRemoveKeys: " << s.ToString(); return count; } count += tmp_ret; } return count; }src/pika_admin.cc (1)
3149-3151: LGTM!The
DoThroughDBmethod is straightforward and correctly calls theDomethod.
src/storage/src/redis_strings.cc
Outdated
| rocksdb::Iterator* iter = db_->NewIterator(iterator_options, handles_[kMetaCF]); | ||
| iter->SeekToFirst(); | ||
| while (iter->Valid()) { | ||
| auto meta_type = static_cast<enum DataType>(static_cast<uint8_t>(iter->value()[0])); |
There was a problem hiding this comment.
Check for null pointer before dereferencing.
Ensure iter is not null before dereferencing it.
if (iter == nullptr) {
return rocksdb::Status::InvalidArgument("Iterator creation failed.");
}
src/storage/src/redis_strings.cc
Outdated
| if (static_cast<size_t>(batch.Count()) >= BATCH_DELETE_LIMIT) { | ||
| s = db_->Write(default_write_options_, &batch); | ||
| if (s.ok()) { | ||
| total_delete += static_cast<int32_t>(batch.Count()); | ||
| batch.Clear(); | ||
| } else { | ||
| remove_keys->erase(remove_keys->end() - batch.Count(), remove_keys->end()); | ||
| *ret = total_delete; | ||
| return s; | ||
| } | ||
| } |
There was a problem hiding this comment.
Handle potential write failure more gracefully.
If db_->Write fails, the method should handle the error more gracefully, such as logging the error and attempting a retry.
if (s.ok()) {
total_delete += static_cast<int32_t>(batch.Count());
batch.Clear();
} else {
LOG(ERROR) << "Batch write failed: " << s.ToString();
remove_keys->erase(remove_keys->end() - batch.Count(), remove_keys->end());
*ret = total_delete;
return s;
}
src/storage/src/redis_strings.cc
Outdated
| if (batch.Count() != 0U) { | ||
| s = db_->Write(default_write_options_, &batch); | ||
| if (s.ok()) { | ||
| total_delete += static_cast<int32_t>(batch.Count()); | ||
| batch.Clear(); | ||
| } | ||
| } |
There was a problem hiding this comment.
Handle potential write failure more gracefully.
Similar to the previous comment, handle the error more gracefully if db_->Write fails.
if (s.ok()) {
total_delete += static_cast<int32_t>(batch.Count());
batch.Clear();
} else {
LOG(ERROR) << "Batch write failed: " << s.ToString();
}
| } | ||
|
|
||
| *ret = total_delete; | ||
| return s; |
src/pika_admin.cc
Outdated
| rocksdb::Status s = db_->storage()->PKPatternMatchDel(type_, pattern_, &ret); | ||
| if (s.ok()) { | ||
| res_.AppendInteger(ret); | ||
| int64_t count = db_->storage()->PKPatternMatchDelWithRemoveKeys(type_, pattern_, &remove_keys_); |
There was a problem hiding this comment.
这个pr提了之后，上边的TODO注释就可以去掉啦。
src/pika_admin.cc
Outdated
| rocksdb::Status s = db_->storage()->PKPatternMatchDel(type_, pattern_, &ret); | ||
| if (s.ok()) { | ||
| res_.AppendInteger(ret); | ||
| int64_t count = db_->storage()->PKPatternMatchDelWithRemoveKeys(type_, pattern_, &remove_keys_); |
There was a problem hiding this comment.
这个count感觉还是传个指针作为形参传入到PKPatternMatchDelWithRemoveKeys中，函数返回值把status返回回来。
src/storage/src/redis_strings.cc
Outdated
| if (!parsed_strings_value.IsStale() && | ||
| (StringMatch(pattern.data(), pattern.size(), parsed_meta_key.Key().data(), parsed_meta_key.Key().size(), 0) != 0)) { | ||
| batch.Delete(key); | ||
| remove_keys->push_back(key); |
There was a problem hiding this comment.
这个地方的key其实是encode之后的key，所以push_back的应该是parsed_meta_key中的key。
| } | ||
| } | ||
|
|
||
| void PKPatternMatchDelCmd::DoThroughDB() { |
There was a problem hiding this comment.
既然记录了删除的key，那是不是binlog内容也可以修改下？直接删除指定的key即可？省去了slave遍历的开销。
|
另外还有一个问题，如果磁盘数据量比较大且匹配到的key比较多，内存占用比较多（记录了remove_key），处理耗时比较长。这个命令是pika自己的，所以是否需要定义一个每次操作可以删除的最大个数。 |
There is another problem. If the amount of disk data is relatively large and there are many matched keys, the memory usage will be large (remove_key is recorded), and the processing will take a long time. This command is pika's own, so do you need to define a maximum number that can be deleted in each operation? |
感谢，我尝试改一下 |
Thanks, I'll try to change it |
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (5)
- include/pika_admin.h (1 hunks)
- src/pika_admin.cc (2 hunks)
- src/storage/include/storage/storage.h (1 hunks)
- src/storage/src/redis_strings.cc (2 hunks)
- src/storage/src/storage.cc (1 hunks)
Files skipped from review as they are similar to previous changes (5)
- include/pika_admin.h
- src/pika_admin.cc
- src/storage/include/storage/storage.h
- src/storage/src/redis_strings.cc
- src/storage/src/storage.cc
问题已修改 |
Question has been modified |
|
|
||
| void PKPatternMatchDelCmd::DoUpdateCache() { | ||
| if(s_.ok()) { | ||
| db_->cache()->Del(remove_keys_); |
There was a problem hiding this comment.
如果remove_key特别多的话 这个地方 remove_keys_ 会不会有放不下的情况，如果redis有按照字符匹配去删除的命令是不是直接执行命令会好一点
There was a problem hiding this comment.
如果remove_key特别多的话 这个地方 remove_keys_ 会不会有放不下的情况，如果redis有按照字符匹配去删除的命令是不是直接执行命令会好一点
我感觉有几个问题
- 这样的话感觉需要 rediscache 中支持 scan命令。
- remove_keys_中感觉需要存删除失败的键，这样才能保证缓存和磁盘上数据的一致性（会有这样的问题么）
- PKPatternMatchDelCmd返回值，需要记录所有失败的信息么，还是和DEL命令一样。（得保证扫描完所有数据才行，现在是遇到错误就停止）
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (7)
- include/pika_admin.h (1 hunks)
- src/pika_admin.cc (1 hunks)
- src/pika_command.cc (1 hunks)
- src/storage/include/storage/storage.h (1 hunks)
- src/storage/src/redis.h (1 hunks)
- src/storage/src/redis_strings.cc (2 hunks)
- src/storage/src/storage.cc (1 hunks)
Files skipped from review as they are similar to previous changes (6)
- include/pika_admin.h
- src/pika_admin.cc
- src/storage/include/storage/storage.h
- src/storage/src/redis.h
- src/storage/src/redis_strings.cc
- src/storage/src/storage.cc
Additional comments not posted (1)
src/pika_command.cc (1)
133-135: Verify the correctness of the arity change forPKPatternMatchDelCmd.Changing the
arityparameter from a positive to a negative value implies that the command now accepts a variable number of arguments, with the absolute value indicating the minimum number of arguments required. Ensure that this change aligns with the command's expected behavior and does not introduce unintended side effects.
添加了一个count参数，默认的最大值是100（可以删除的最大个数） pkpatternmatch pattern [count] |
Added a count parameter, the default maximum value is 100 (the maximum number that can be deleted) pkpatternmatch pattern [count] |
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- src/storage/src/redis_strings.cc (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- src/storage/src/redis_strings.cc
| if (s.ok()) { | ||
| res_.AppendInteger(ret); | ||
| int64_t count = 0; | ||
| rocksdb::Status s = db_->storage()->PKPatternMatchDelWithRemoveKeys(pattern_, &count, &remove_keys_, max_count_); |
There was a problem hiding this comment.
看起来storage和redis.h里的PKPatternMatchDel函数用不到了，感觉可以删了。
src/pika_admin.cc
Outdated
| int64_t count = 0; | ||
| rocksdb::Status s = db_->storage()->PKPatternMatchDelWithRemoveKeys(pattern_, &count, &remove_keys_, max_count_); | ||
|
|
||
| if(s.ok()){ |
There was a problem hiding this comment.
Actionable comments posted: 4
Outside diff range, codebase verification and nitpick comments (1)
src/pika_admin.cc (1)
3176-3179: Ensure cache consistency.Ensure that the cache is updated consistently even if the deletion operation partially succeeds. Consider adding error handling for cache updates to ensure consistency.
src/pika_admin.cc:void PKPatternMatchDelCmd::DoUpdateCache() { if (s_.ok()) { auto status = db_->cache()->Del(remove_keys_); if (!status.ok()) { // Handle the error, possibly logging it and taking corrective actions } } }Analysis chain
Ensure cache consistency.
Ensure that the cache is updated consistently even if the deletion operation partially succeeds.
Consider adding error handling for cache updates to ensure consistency.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify cache consistency after deletion operation. # Test: Search for cache update logic. Expect: Proper error handling and consistency checks. rg --type cpp -A 5 'cache()->Del'Length of output: 35
Script:
#!/bin/bash # Description: Verify cache consistency after deletion operation. # Test: Search for cache update logic. Expect: Proper error handling and consistency checks. rg --type cpp -A 5 'cache()->Del' rg --type cpp -A 5 'cache()->Update' rg --type cpp -A 5 'cache()->Put'Length of output: 110
Script:
#!/bin/bash # Description: Verify cache consistency after deletion operation. # Test: Search for all cache-related methods. Expect: Proper error handling and consistency checks. rg --type cpp -A 10 'cache()->'Length of output: 1103
Script:
#!/bin/bash # Description: Verify cache consistency after deletion operation. # Test: Search for the PKPatternMatchDelCmd class and its methods. Expect: Proper error handling and consistency checks. rg --type cpp -A 10 'class PKPatternMatchDelCmd' rg --type cpp -A 10 'PKPatternMatchDelCmd::DoUpdateCache'Length of output: 1255
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (7)
- CMakeLists.txt (1 hunks)
- src/pika_admin.cc (1 hunks)
- src/storage/include/storage/storage.h (1 hunks)
- src/storage/src/redis.h (1 hunks)
- src/storage/src/redis_strings.cc (6 hunks)
- src/storage/src/storage.cc (1 hunks)
- src/storage/tests/keys_test.cc (30 hunks)
Files skipped from review due to trivial changes (1)
- CMakeLists.txt
Files skipped from review as they are similar to previous changes (4)
- src/storage/include/storage/storage.h
- src/storage/src/redis.h
- src/storage/src/redis_strings.cc
- src/storage/src/storage.cc
Additional comments not posted (22)
src/storage/tests/keys_test.cc (22)
2101-2104: LGTM!The changes to introduce
remove_keys,max_count, and changingdelete_counttoint64_tare appropriate for handling the new functionality.
2116-2121: LGTM!The function call to
PKPatternMatchDelWithRemoveKeysand the associated assertions ensure the correct number of keys are deleted and removed. Clearingremove_keysafter usage is a good practice.
2135-2141: LGTM!The function call to
PKPatternMatchDelWithRemoveKeysfor expired keys and the associated assertions ensure the correct number of expired keys are deleted and removed. Clearingremove_keysafter usage is a good practice.
2152-2157: LGTM!The function call to
PKPatternMatchDelWithRemoveKeyswith a specific pattern and the associated assertions ensure the correct number of keys matching the pattern are deleted and removed. Clearingremove_keysafter usage is a good practice.
2176-2181: LGTM!The function call to
PKPatternMatchDelWithRemoveKeysfor expired keys with a specific pattern and the associated assertions ensure the correct number of expired keys matching the pattern are deleted and removed. Clearingremove_keysafter usage is a good practice.
2190-2198: LGTM!The function call to
PKPatternMatchDelWithRemoveKeysfor a large number of keys and the associated assertions ensure the maximum number of keys are deleted and removed. Clearingremove_keysafter usage is a good practice.
2209-2214: LGTM!The function call to
PKPatternMatchDelWithRemoveKeysfor set data type and the associated assertions ensure the correct number of set keys are deleted and removed. Clearingremove_keysafter usage is a good practice.
2228-2233: LGTM!The function call to
PKPatternMatchDelWithRemoveKeysfor expired set keys and the associated assertions ensure the correct number of expired set keys are deleted and removed. Clearingremove_keysafter usage is a good practice.
2244-2249: LGTM!The function call to
PKPatternMatchDelWithRemoveKeysfor set keys with a specific pattern and the associated assertions ensure the correct number of set keys matching the pattern are deleted and removed. Clearingremove_keysafter usage is a good practice.
2268-2273: LGTM!The function call to
PKPatternMatchDelWithRemoveKeysfor removed set keys and the associated assertions ensure the correct number of removed set keys are deleted and removed. Clearingremove_keysafter usage is a good practice.
2290-2295: LGTM!The function call to
PKPatternMatchDelWithRemoveKeysfor expired set keys with a specific pattern and the associated assertions ensure the correct number of expired set keys matching the pattern are deleted and removed. Clearingremove_keysafter usage is a good practice.
2308-2316: LGTM!The function call to
PKPatternMatchDelWithRemoveKeysfor a large number of set keys and the associated assertions ensure the maximum number of set keys are deleted and removed. Clearingremove_keysafter usage is a good practice.
2327-2332: LGTM!The function call to
PKPatternMatchDelWithRemoveKeysfor hash data type and the associated assertions ensure the correct number of hash keys are deleted and removed. Clearingremove_keysafter usage is a good practice.
2346-2351: LGTM!The function call to
PKPatternMatchDelWithRemoveKeysfor expired hash keys and the associated assertions ensure the correct number of expired hash keys are deleted and removed. Clearingremove_keysafter usage is a good practice.
2362-2367: LGTM!The function call to
PKPatternMatchDelWithRemoveKeysfor hash keys with a specific pattern and the associated assertions ensure the correct number of hash keys matching the pattern are deleted and removed. Clearingremove_keysafter usage is a good practice.
2386-2391: LGTM!The function call to
PKPatternMatchDelWithRemoveKeysfor removed hash keys and the associated assertions ensure the correct number of removed hash keys are deleted and removed. Clearingremove_keysafter usage is a good practice.
2408-2413: LGTM!The function call to
PKPatternMatchDelWithRemoveKeysfor expired hash keys with a specific pattern and the associated assertions ensure the correct number of expired hash keys matching the pattern are deleted and removed. Clearingremove_keysafter usage is a good practice.
2426-2434: LGTM!The function call to
PKPatternMatchDelWithRemoveKeysfor a large number of hash keys and the associated assertions ensure the maximum number of hash keys are deleted and removed. Clearingremove_keysafter usage is a good practice.
2445-2450: LGTM!The function call to
PKPatternMatchDelWithRemoveKeysfor zset data type and the associated assertions ensure the correct number of zset keys are deleted and removed. Clearingremove_keysafter usage is a good practice.
2464-2469: LGTM!The function call to
PKPatternMatchDelWithRemoveKeysfor expired zset keys and the associated assertions ensure the correct number of expired zset keys are deleted and removed. Clearingremove_keysafter usage is a good practice.
2480-2485: LGTM!The function call to
PKPatternMatchDelWithRemoveKeysfor zset keys with a specific pattern and the associated assertions ensure the correct number of zset keys matching the pattern are deleted and removed. Clearingremove_keysafter usage is a good practice.
2504-2509: LGTM!The function call to
PKPatternMatchDelWithRemoveKeysfor removed zset keys and the associated assertions ensure the correct number of removed zset keys are deleted and removed. Clearingremove_keysafter usage is a good practice.
src/pika_admin.cc
Outdated
| if (count >= 0) { | ||
| s_ = rocksdb::Status::OK(); | ||
| std::vector<std::string>::const_iterator it; | ||
| for (it = remove_keys_.begin(); it != remove_keys_.end(); it++) { | ||
| RemSlotKey(*it, db_); | ||
| } | ||
| } |
There was a problem hiding this comment.
Avoid duplicate code for removing slot keys.
The code for removing slot keys is duplicated. Consider refactoring to avoid duplication.
- } else {
- res_.SetRes(CmdRes::kErrOther, s.ToString());
- if (count >= 0) {
- s_ = rocksdb::Status::OK();
- std::vector<std::string>::const_iterator it;
- for (it = remove_keys_.begin(); it != remove_keys_.end(); it++) {
- RemSlotKey(*it, db_);
- }
- }
- }
+ } else if (count >= 0) {
+ s_ = rocksdb::Status::OK();
+ } else {
+ res_.SetRes(CmdRes::kErrOther, s.ToString());
+ }
+ for (const auto& key : remove_keys_) {
+ RemSlotKey(key, db_);
+ }Committable suggestion was skipped due to low confidence.
src/pika_admin.cc
Outdated
| max_count_ = storage::BATCH_DELETE_LIMIT; | ||
| if (argv_.size() > 2) { | ||
| if (pstd::string2int(argv_[2].data(), argv_[2].size(), &max_count_) == 0 || max_count_ > storage::BATCH_DELETE_LIMIT) { | ||
| res_.SetRes(CmdRes::kInvalidInt); | ||
| return; | ||
| } | ||
| } |
There was a problem hiding this comment.
Ensure proper bounds for max_count_.
The max_count_ parameter should be validated to ensure it is within acceptable bounds. Consider adding a lower bound check.
- if (argv_.size() > 2) {
- if (pstd::string2int(argv_[2].data(), argv_[2].size(), &max_count_) == 0 || max_count_ > storage::BATCH_DELETE_LIMIT) {
- res_.SetRes(CmdRes::kInvalidInt);
- return;
- }
- }
+ if (argv_.size() > 2) {
+ if (pstd::string2int(argv_[2].data(), argv_[2].size(), &max_count_) == 0 || max_count_ < 1 || max_count_ > storage::BATCH_DELETE_LIMIT) {
+ res_.SetRes(CmdRes::kInvalidInt);
+ return;
+ }
+ }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.
| max_count_ = storage::BATCH_DELETE_LIMIT; | |
| if (argv_.size() > 2) { | |
| if (pstd::string2int(argv_[2].data(), argv_[2].size(), &max_count_) == 0 || max_count_ > storage::BATCH_DELETE_LIMIT) { | |
| res_.SetRes(CmdRes::kInvalidInt); | |
| return; | |
| } | |
| } | |
| max_count_ = storage::BATCH_DELETE_LIMIT; | |
| if (argv_.size() > 2) { | |
| if (pstd::string2int(argv_[2].data(), argv_[2].size(), &max_count_) == 0 || max_count_ < 1 || max_count_ > storage::BATCH_DELETE_LIMIT) { | |
| res_.SetRes(CmdRes::kInvalidInt); | |
| return; | |
| } | |
| } |
| void PKPatternMatchDelCmd::DoBinlog() { | ||
| std::string opt = "del"; | ||
| for(auto& key: remove_keys_) { | ||
| argv_.clear(); | ||
| argv_.emplace_back(opt); | ||
| argv_.emplace_back(key); | ||
| Cmd::DoBinlog(); |
There was a problem hiding this comment.
Optimize binlog operations.
The binlog operations can be optimized by reducing the number of argv_ modifications.
- std::string opt = "del";
- for(auto& key: remove_keys_) {
- argv_.clear();
- argv_.emplace_back(opt);
- argv_.emplace_back(key);
- Cmd::DoBinlog();
- }
+ argv_.clear();
+ argv_.emplace_back("del");
+ for (const auto& key : remove_keys_) {
+ argv_.resize(2);
+ argv_[1] = key;
+ Cmd::DoBinlog();
+ }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.
| void PKPatternMatchDelCmd::DoBinlog() { | |
| std::string opt = "del"; | |
| for(auto& key: remove_keys_) { | |
| argv_.clear(); | |
| argv_.emplace_back(opt); | |
| argv_.emplace_back(key); | |
| Cmd::DoBinlog(); | |
| void PKPatternMatchDelCmd::DoBinlog() { | |
| argv_.clear(); | |
| argv_.emplace_back("del"); | |
| for (const auto& key : remove_keys_) { | |
| argv_.resize(2); | |
| argv_[1] = key; | |
| Cmd::DoBinlog(); |
src/pika_admin.cc
Outdated
| int64_t count = 0; | ||
| rocksdb::Status s = db_->storage()->PKPatternMatchDelWithRemoveKeys(pattern_, &count, &remove_keys_, max_count_); | ||
|
|
||
| if(s.ok()) { | ||
| res_.AppendInteger(count); | ||
| s_ = rocksdb::Status::OK(); | ||
| std::vector<std::string>::const_iterator it; | ||
| for (it = remove_keys_.begin(); it != remove_keys_.end(); it++) { | ||
| RemSlotKey(*it, db_); | ||
| } | ||
| } else { | ||
| res_.SetRes(CmdRes::kErrOther, s.ToString()); | ||
| if (count >= 0) { | ||
| s_ = rocksdb::Status::OK(); | ||
| std::vector<std::string>::const_iterator it; | ||
| for (it = remove_keys_.begin(); it != remove_keys_.end(); it++) { | ||
| RemSlotKey(*it, db_); | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Optimize the loop for removing slot keys.
The loop for removing slot keys can be optimized by using a range-based for loop.
- std::vector<std::string>::const_iterator it;
- for (it = remove_keys_.begin(); it != remove_keys_.end(); it++) {
- RemSlotKey(*it, db_);
- }
+ for (const auto& key : remove_keys_) {
+ RemSlotKey(key, db_);
+ }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.
| int64_t count = 0; | |
| rocksdb::Status s = db_->storage()->PKPatternMatchDelWithRemoveKeys(pattern_, &count, &remove_keys_, max_count_); | |
| if(s.ok()) { | |
| res_.AppendInteger(count); | |
| s_ = rocksdb::Status::OK(); | |
| std::vector<std::string>::const_iterator it; | |
| for (it = remove_keys_.begin(); it != remove_keys_.end(); it++) { | |
| RemSlotKey(*it, db_); | |
| } | |
| } else { | |
| res_.SetRes(CmdRes::kErrOther, s.ToString()); | |
| if (count >= 0) { | |
| s_ = rocksdb::Status::OK(); | |
| std::vector<std::string>::const_iterator it; | |
| for (it = remove_keys_.begin(); it != remove_keys_.end(); it++) { | |
| RemSlotKey(*it, db_); | |
| } | |
| } | |
| } | |
| } | |
| int64_t count = 0; | |
| rocksdb::Status s = db_->storage()->PKPatternMatchDelWithRemoveKeys(pattern_, &count, &remove_keys_, max_count_); | |
| if(s.ok()) { | |
| res_.AppendInteger(count); | |
| s_ = rocksdb::Status::OK(); | |
| for (const auto& key : remove_keys_) { | |
| RemSlotKey(key, db_); | |
| } | |
| } else { | |
| res_.SetRes(CmdRes::kErrOther, s.ToString()); | |
| if (count >= 0) { | |
| s_ = rocksdb::Status::OK(); | |
| for (const auto& key : remove_keys_) { | |
| RemSlotKey(key, db_); | |
| } | |
| } | |
| } | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- include/pika_admin.h (1 hunks)
- src/pika_admin.cc (1 hunks)
Files skipped from review as they are similar to previous changes (2)
- include/pika_admin.h
- src/pika_admin.cc
|
辛苦在加点相关的测试吧 |
Please try to add some related tests. |
…omFoundation#2777) * update PkPatternMatchDel with rediscache * fix bug * fix bug * feat:add count argument with pkpatternmatchdel * fix bug * remove unused function * fix bug
issue: #2715
Summary by CodeRabbit
New Features
Improved Performance
Bug Fixes