Aetherance commented on code in PR #3541:
URL: https://github.com/apache/kvrocks/pull/3541#discussion_r3522868777
##########
src/types/redis_string.cc:
##########
@@ -345,7 +347,9 @@ rocksdb::Status String::Set(engine::Context &ctx, const
std::string &user_key, c
metadata.expire = expire;
metadata.Encode(&new_raw_value);
new_raw_value.append(value);
- return updateRawValue(ctx, ns_key, new_raw_value);
+ auto s = updateRawValue(ctx, ns_key, new_raw_value);
+ if (s.ok() && applied != nullptr) *applied = true;
+ return s;
Review Comment:
`Status` only tells whether the command errored, not whether it mutated the
key. For example, src/types/redis_string.cc:L286-L289 returns OK for SET ... NX
when the key already exists:
```cpp
if (args.type == StringSetType::NX && s.ok()) {
// if NX option given, the key already exist: return nil
if (!args.get) ret = std::nullopt;
return rocksdb::Status::OK();
}
```
This is a valid no-op per SET key value NX semantics, while a real
successful write also returns OK. So the caller still needs applied to
distinguish no-op success from write success.
--
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]
