[GitHub] [kvrocks] mapleFU commented on a diff in pull request #1631: Fix MSETNX not allow overriding the same key

["mapleFU (via GitHub)" <gi...@apache.org>]

mapleFU commented on code in PR #1631:
URL: https://github.com/apache/kvrocks/pull/1631#discussion_r1282773834


##########
src/types/redis_string.cc:
##########
@@ -213,12 +213,12 @@ rocksdb::Status String::GetDel(const std::string &user_key, std::string *value)
 
 rocksdb::Status String::Set(const std::string &user_key, const std::string &value) {
   std::vector<StringPair> pairs{StringPair{user_key, value}};
-  return MSet(pairs, 0);
+  return MSet(pairs, 0, true);

Review Comment:
   We'd better mark the argument:
   ```suggestion
     return MSet(pairs, /*ttl=*/0, /*lock=*/true);
   ```



##########
src/types/redis_string.cc:
##########
@@ -384,7 +384,7 @@ rocksdb::Status String::MSet(const std::vector<StringPair> &pairs, uint64_t ttl)
     batch->PutLogData(log_data.Encode());
     AppendNamespacePrefix(pair.key, &ns_key);
     batch->Put(metadata_cf_handle_, ns_key, bytes);
-    LockGuard guard(storage_->GetLockManager(), ns_key);
+    if (lock) LockGuard guard(storage_->GetLockManager(), ns_key);

Review Comment:
   Would lifetime of LockGuard be a temperal?
   
   ```
   std::optional<LockGuard> guard;
   if (lock) {
     guard = ...
   }
   ```
   
   What about this?



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org