fix review change

OpenAtomFoundation · Aug 7, 2024 · 5f22ac2 · 5f22ac2
1 parent 407ac39
commit 5f22ac2
Show file tree

Hide file tree

Showing 9 changed files with 46 additions and 102 deletions.
diff --git 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<std::string> 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
@@ -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<size_t>(batch.Count()) >= BATCH_DELETE_LIMIT) {
-      s = db_->Write(default_write_options_, &batch);
-      if (s.ok()) {
-        total_delete += static_cast<int64_t>( 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<int64_t>(batch.Count());
       batch.Clear();
+    } else {
+      remove_keys->clear();
     }
   }
 

diff --git 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<size_t>(batch.Count()) >= BATCH_DELETE_LIMIT) {
-      s = db_->Write(default_write_options_, &batch);
-      if (s.ok()) {
-        total_delete += static_cast<int64_t>(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<int64_t>(batch.Count());
       batch.Clear();
+    } else {
+      remove_keys->clear();
     }
   }
 

diff --git 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<size_t>(batch.Count()) >= BATCH_DELETE_LIMIT) {
-      s = db_->Write(default_write_options_, &batch);
-      if (s.ok()) {
-        total_delete += static_cast<int64_t>(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<int64_t>(batch.Count());
       batch.Clear();
+    } else {
+      remove_keys->clear();
     }
   }
 

diff --git 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<size_t>(batch.Count()) >= BATCH_DELETE_LIMIT) {
-      s = db_->Write(default_write_options_, &batch);
-      if (s.ok()) {
-        total_delete += static_cast<int64_t>(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<int64_t>(batch.Count());
       batch.Clear();
+    } else {
+      remove_keys->clear();
     }
   }
 

diff --git 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<size_t>(batch.Count()) >= BATCH_DELETE_LIMIT) {
-      s = db_->Write(default_write_options_, &batch);
-      if (s.ok()) {
-        total_delete += static_cast<int64_t>(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<int64_t>( 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
@@ -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<size_t>(batch.Count()) >= BATCH_DELETE_LIMIT) {
-      s = db_->Write(default_write_options_, &batch);
-      if (s.ok()) {
-        total_delete += static_cast<int64_t>(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<int64_t>(batch.Count());
       batch.Clear();
+    } else {
+      remove_keys->clear();
     }
   }
 

diff --git 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<std::string>* 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
@@ -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);