From 5f22ac21151b821357a9854a3b01e53701735832 Mon Sep 17 00:00:00 2001 From: chejinge Date: Tue, 6 Aug 2024 11:51:06 +0800 Subject: [PATCH] fix review change --- src/pika_admin.cc | 46 +++++++++++++++----------------- src/storage/src/redis_hashes.cc | 14 +++------- src/storage/src/redis_lists.cc | 14 +++------- src/storage/src/redis_sets.cc | 14 +++------- src/storage/src/redis_streams.cc | 14 +++------- src/storage/src/redis_strings.cc | 17 +++--------- src/storage/src/redis_zsets.cc | 14 +++------- src/storage/src/storage.cc | 13 +++++---- src/storage/tests/keys_test.cc | 2 -- 9 files changed, 46 insertions(+), 102 deletions(-) diff --git a/src/pika_admin.cc b/src/pika_admin.cc index d564e9193a..deba49f5c5 100644 --- a/src/pika_admin.cc +++ b/src/pika_admin.cc @@ -3099,8 +3099,8 @@ void PKPatternMatchDelCmd::DoInitial() { 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) { + if (argv_.size() > 3) { + if (pstd::string2int(argv_[3].data(), argv_[3].size(), &max_count_) == 0 || max_count_ < 1 || max_count_ > storage::BATCH_DELETE_LIMIT) { res_.SetRes(CmdRes::kInvalidInt); return; } @@ -3133,30 +3133,28 @@ void PKPatternMatchDelCmd::DoThroughDB() { } void PKPatternMatchDelCmd::DoUpdateCache() { - if(s_.ok()) { + if (s_.ok()) { std::vector v; for (auto key : remove_keys_) { - if (argv_.size() > 2) { - //only delete the corresponding prefix - switch (type_) { - case storage::kSets: - v.emplace_back(PCacheKeyPrefixS + key); - break; - case storage::kLists: - v.emplace_back(PCacheKeyPrefixL + key); - break; - case storage::kStrings: - v.emplace_back(PCacheKeyPrefixK + key); - break; - case storage::kZSets: - v.emplace_back(PCacheKeyPrefixZ + key); - break; - case storage::kHashes: - v.emplace_back(PCacheKeyPrefixH + key); - break; - default: - break; - } + // only delete the corresponding prefix + switch (type_) { + case storage::kSets: + v.emplace_back(PCacheKeyPrefixS + key); + break; + case storage::kLists: + v.emplace_back(PCacheKeyPrefixL + key); + break; + case storage::kStrings: + v.emplace_back(PCacheKeyPrefixK + key); + break; + case storage::kZSets: + v.emplace_back(PCacheKeyPrefixZ + key); + break; + case storage::kHashes: + v.emplace_back(PCacheKeyPrefixH + key); + break; + default: + break; } } db_->cache()->Del(v); diff --git a/src/storage/src/redis_hashes.cc b/src/storage/src/redis_hashes.cc index 57f1b0d71d..4708a6e860 100644 --- a/src/storage/src/redis_hashes.cc +++ b/src/storage/src/redis_hashes.cc @@ -170,17 +170,7 @@ Status RedisHashes::PKPatternMatchDelWithRemoveKeys(const DataType& data_type, c (StringMatch(pattern.data(), pattern.size(), key.data(), key.size(), 0) != 0)) { parsed_hashes_meta_value.InitialMetaValue(); batch.Put(handles_[0], key, meta_value); - remove_keys->push_back(key.data()); - } - if (static_cast(batch.Count()) >= BATCH_DELETE_LIMIT) { - s = db_->Write(default_write_options_, &batch); - if (s.ok()) { - total_delete += static_cast( batch.Count()); - batch.Clear(); - } else { - *ret = total_delete; - return s; - } + remove_keys->push_back(key); } iter->Next(); } @@ -189,6 +179,8 @@ Status RedisHashes::PKPatternMatchDelWithRemoveKeys(const DataType& data_type, c if (s.ok()) { total_delete += static_cast(batch.Count()); batch.Clear(); + } else { + remove_keys->clear(); } } diff --git a/src/storage/src/redis_lists.cc b/src/storage/src/redis_lists.cc index 140046e2f9..4c83ffecad 100644 --- a/src/storage/src/redis_lists.cc +++ b/src/storage/src/redis_lists.cc @@ -177,17 +177,7 @@ Status RedisLists::PKPatternMatchDelWithRemoveKeys(const DataType& data_type, co (StringMatch(pattern.data(), pattern.size(), key.data(), key.size(), 0) != 0)) { parsed_lists_meta_value.InitialMetaValue(); batch.Put(handles_[0], key, meta_value); - remove_keys->push_back(key.data()); - } - if (static_cast(batch.Count()) >= BATCH_DELETE_LIMIT) { - s = db_->Write(default_write_options_, &batch); - if (s.ok()) { - total_delete += static_cast(batch.Count()); - batch.Clear(); - } else { - *ret = total_delete; - return s; - } + remove_keys->push_back(key); } iter->Next(); } @@ -196,6 +186,8 @@ Status RedisLists::PKPatternMatchDelWithRemoveKeys(const DataType& data_type, co if (s.ok()) { total_delete += static_cast(batch.Count()); batch.Clear(); + } else { + remove_keys->clear(); } } diff --git a/src/storage/src/redis_sets.cc b/src/storage/src/redis_sets.cc index ac84c95b7e..7a6678da46 100644 --- a/src/storage/src/redis_sets.cc +++ b/src/storage/src/redis_sets.cc @@ -177,17 +177,7 @@ rocksdb::Status RedisSets::PKPatternMatchDelWithRemoveKeys(const DataType& data_ (StringMatch(pattern.data(), pattern.size(), key.data(), key.size(), 0) != 0)) { parsed_sets_meta_value.InitialMetaValue(); batch.Put(handles_[0], key, meta_value); - remove_keys->push_back(key.data()); - } - if (static_cast(batch.Count()) >= BATCH_DELETE_LIMIT) { - s = db_->Write(default_write_options_, &batch); - if (s.ok()) { - total_delete += static_cast(batch.Count()); - batch.Clear(); - } else { - *ret = total_delete; - return s; - } + remove_keys->push_back(key); } iter->Next(); } @@ -196,6 +186,8 @@ rocksdb::Status RedisSets::PKPatternMatchDelWithRemoveKeys(const DataType& data_ if (s.ok()) { total_delete += static_cast(batch.Count()); batch.Clear(); + } else { + remove_keys->clear(); } } diff --git a/src/storage/src/redis_streams.cc b/src/storage/src/redis_streams.cc index 4f195bf95d..ee1eddaa91 100644 --- a/src/storage/src/redis_streams.cc +++ b/src/storage/src/redis_streams.cc @@ -469,17 +469,7 @@ Status RedisStreams::PKPatternMatchDelWithRemoveKeys(const DataType& data_type, (StringMatch(pattern.data(), pattern.size(), key.data(), key.size(), 0) != 0)) { stream_meta_value.InitMetaValue(); batch.Put(handles_[0], key, stream_meta_value.value()); - remove_keys->push_back(key.data()); - } - if (static_cast(batch.Count()) >= BATCH_DELETE_LIMIT) { - s = db_->Write(default_write_options_, &batch); - if (s.ok()) { - total_delete += static_cast(batch.Count()); - batch.Clear(); - } else { - *ret = total_delete; - return s; - } + remove_keys->push_back(key); } iter->Next(); } @@ -488,6 +478,8 @@ Status RedisStreams::PKPatternMatchDelWithRemoveKeys(const DataType& data_type, if (s.ok()) { total_delete += static_cast(batch.Count()); batch.Clear(); + } else { + remove_keys->clear(); } } diff --git a/src/storage/src/redis_strings.cc b/src/storage/src/redis_strings.cc index bf7eb7f5d4..5b1b09ff07 100644 --- a/src/storage/src/redis_strings.cc +++ b/src/storage/src/redis_strings.cc @@ -138,19 +138,7 @@ Status RedisStrings::PKPatternMatchDelWithRemoveKeys(const DataType& data_type, ParsedStringsValue parsed_strings_value(&value); if (!parsed_strings_value.IsStale() && (StringMatch(pattern.data(), pattern.size(), key.data(), key.size(), 0) != 0)) { batch.Delete(key); - remove_keys->push_back(key.data()); - } - // In order to be more efficient, we use batch deletion here - if (static_cast(batch.Count()) >= BATCH_DELETE_LIMIT) { - s = db_->Write(default_write_options_, &batch); - if (s.ok()) { - total_delete += static_cast(batch.Count()); - batch.Clear(); - } else { - *ret = total_delete; - delete iter; - return s; - } + remove_keys->push_back(key); } iter->Next(); } @@ -159,9 +147,10 @@ Status RedisStrings::PKPatternMatchDelWithRemoveKeys(const DataType& data_type, if (s.ok()) { total_delete += static_cast( batch.Count()); batch.Clear(); + } else { + remove_keys->clear(); } } - delete iter; *ret = total_delete; return s; } diff --git a/src/storage/src/redis_zsets.cc b/src/storage/src/redis_zsets.cc index 312e9a0ddd..ba6094575c 100644 --- a/src/storage/src/redis_zsets.cc +++ b/src/storage/src/redis_zsets.cc @@ -192,17 +192,7 @@ Status RedisZSets::PKPatternMatchDelWithRemoveKeys(const DataType& data_type, co (StringMatch(pattern.data(), pattern.size(), key.data(), key.size(), 0) != 0)) { parsed_zsets_meta_value.InitialMetaValue(); batch.Put(handles_[0], key, meta_value); - remove_keys->push_back(key.data()); - } - if (static_cast(batch.Count()) >= BATCH_DELETE_LIMIT) { - s = db_->Write(default_write_options_, &batch); - if (s.ok()) { - total_delete += static_cast(batch.Count()); - batch.Clear(); - } else { - *ret = total_delete; - return s; - } + remove_keys->push_back(key); } iter->Next(); } @@ -211,6 +201,8 @@ Status RedisZSets::PKPatternMatchDelWithRemoveKeys(const DataType& data_type, co if (s.ok()) { total_delete += static_cast(batch.Count()); batch.Clear(); + } else { + remove_keys->clear(); } } diff --git a/src/storage/src/storage.cc b/src/storage/src/storage.cc index b5c2a0d300..b2f8f02f41 100644 --- a/src/storage/src/storage.cc +++ b/src/storage/src/storage.cc @@ -1140,25 +1140,24 @@ Status Storage::PKRScanRange(const DataType& data_type, const Slice& key_start, Status Storage::PKPatternMatchDelWithRemoveKeys(const DataType& data_type, const std::string& pattern, int64_t* ret, std::vector* remove_keys, const int64_t& max_count) { Status s; - int64_t tmp_ret = 0; switch (data_type) { case DataType::kStrings: - s = strings_db_->PKPatternMatchDelWithRemoveKeys(DataType::kStrings, pattern, &tmp_ret, remove_keys, max_count - *ret); + s = strings_db_->PKPatternMatchDelWithRemoveKeys(DataType::kStrings, pattern, ret, remove_keys, max_count - *ret); break; case DataType::kHashes: - s = hashes_db_->PKPatternMatchDelWithRemoveKeys(DataType::kHashes, pattern, &tmp_ret, remove_keys, max_count - *ret); + s = hashes_db_->PKPatternMatchDelWithRemoveKeys(DataType::kHashes, pattern, ret, remove_keys, max_count - *ret); break; case DataType::kLists: - s = lists_db_->PKPatternMatchDelWithRemoveKeys(DataType::kLists, pattern, &tmp_ret, remove_keys, max_count - *ret); + s = lists_db_->PKPatternMatchDelWithRemoveKeys(DataType::kLists, pattern, ret, remove_keys, max_count - *ret); break; case DataType::kZSets: - s = zsets_db_->PKPatternMatchDelWithRemoveKeys(DataType::kZSets, pattern, &tmp_ret, remove_keys, max_count - *ret); + s = zsets_db_->PKPatternMatchDelWithRemoveKeys(DataType::kZSets, pattern, ret, remove_keys, max_count - *ret); break; case DataType::kSets: - s = sets_db_->PKPatternMatchDelWithRemoveKeys(DataType::kSets, pattern, &tmp_ret, remove_keys, max_count - *ret); + s = sets_db_->PKPatternMatchDelWithRemoveKeys(DataType::kSets, pattern, ret, remove_keys, max_count - *ret); break; case DataType::kStreams: - s = streams_db_->PKPatternMatchDelWithRemoveKeys(DataType::kStreams, pattern, &tmp_ret, remove_keys, max_count - *ret); + s = streams_db_->PKPatternMatchDelWithRemoveKeys(DataType::kStreams, pattern, ret, remove_keys, max_count - *ret); break; default: s = Status::Corruption("Unsupported data type"); diff --git a/src/storage/tests/keys_test.cc b/src/storage/tests/keys_test.cc index 6d91a78138..745b3eaf1a 100644 --- a/src/storage/tests/keys_test.cc +++ b/src/storage/tests/keys_test.cc @@ -2459,8 +2459,6 @@ for (const auto& kv : kvs) { // ASSERT_EQ(keys.size(), 0); // //=============================== List =============================== - - // // ***************** Group 1 Test ***************** // db.LPush("GP1_PKPATTERNMATCHDEL_LIST_KEY1", {"VALUE"}, &ret64); // db.LPush("GP1_PKPATTERNMATCHDEL_LIST_KEY2", {"VALUE"}, &ret64);