PragmaTwice commented on code in PR #1935:
URL: https://github.com/apache/kvrocks/pull/1935#discussion_r1428823940


##########
src/types/redis_string.cc:
##########
@@ -217,38 +206,87 @@ rocksdb::Status String::Set(const std::string &user_key, 
const std::string &valu
   return MSet(pairs, /*ttl=*/0, /*lock=*/true);
 }
 
-rocksdb::Status String::SetEX(const std::string &user_key, const std::string 
&value, uint64_t ttl) {
-  std::vector<StringPair> pairs{StringPair{user_key, value}};
-  return MSet(pairs, /*ttl=*/ttl, /*lock=*/true);
-}
+rocksdb::Status String::Set(const std::string &user_key, const std::string 
&value, uint64_t ttl, StringSetType type,
+                            bool get, bool keep_ttl, 
std::optional<std::string> &ret) {
+  std::string ns_key = AppendNamespacePrefix(user_key);
 
-rocksdb::Status String::SetNX(const std::string &user_key, const std::string 
&value, uint64_t ttl, bool *flag) {
-  std::vector<StringPair> pairs{StringPair{user_key, value}};
-  return MSetNX(pairs, ttl, flag);
-}
+  LockGuard guard(storage_->GetLockManager(), ns_key);
 
-rocksdb::Status String::SetXX(const std::string &user_key, const std::string 
&value, uint64_t ttl, bool *flag) {
-  *flag = false;
-  int exists = 0;
+  // Get old value for NX/XX/GET/KEEPTTL option
+  std::string old_raw_value;
+  auto s = getRawValue(ns_key, &old_raw_value);
+  if (!s.ok() && !s.IsNotFound() && !s.IsInvalidArgument()) return s;
+  auto old_key_found = !s.IsNotFound();
+  // The reply following Redis doc: https://redis.io/commands/set/
+  // Handle GET option
+  if (get) {
+    if (s.IsInvalidArgument()) {
+      return s;
+    }
+    if (old_key_found) {
+      // if GET option given: return The previous value of the key.
+      auto offset = Metadata::GetOffsetAfterExpire(old_raw_value[0]);
+      ret = std::make_optional(old_raw_value.substr(offset));
+    } else {
+      // if GET option given, the key didn't exist before: return nil
+      ret = std::nullopt;
+    }
+  }
+
+  // Handle NX/XX option
+  if (old_key_found && type == StringSetType::NX) {
+    // if GET option not given, operation aborted: return nil
+    if (!get) ret = std::nullopt;
+    return rocksdb::Status::OK();
+  } else if (!old_key_found && type == StringSetType::XX) {
+    // if GET option not given, operation aborted: return nil
+    if (!get) ret = std::nullopt;
+    return rocksdb::Status::OK();
+  } else {
+    // if GET option not given, make ret not nil
+    if (!get) ret = std::make_optional("");

Review Comment:
   ```suggestion
       if (!get) ret = "";
   ```



-- 
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: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]

Reply via email to