Aetherance commented on code in PR #3541:
URL: https://github.com/apache/kvrocks/pull/3541#discussion_r3702252654
##########
src/commands/cmd_key.cc:
##########
@@ -372,9 +374,17 @@ class CommandDel : public Commander {
uint64_t cnt = 0;
redis::Database redis(srv->storage, conn->GetNamespace());
- auto s = redis.MDel(ctx, keys, &cnt);
+ const bool notify_del = GetAttributes()->name == "del" &&
conn->IsKeyspaceEventEnabled(kNotifyGeneric);
+ std::vector<rocksdb::Slice> deleted_keys;
+ if (notify_del) deleted_keys.reserve(keys.size());
+
+ auto s = redis.MDel(ctx, keys, &cnt, notify_del ? &deleted_keys : nullptr);
if (!s.ok()) return {Status::RedisExecErr, s.ToString()};
+ for (const auto &key : deleted_keys) {
+ conn->AddKeyspaceEvent(kNotifyGeneric, "del",
std::string_view(key.data(), key.size()));
+ }
+
Review Comment:
Hi @jihuayu , I took a closer look at this approach, and passing key events
through the context seems like a good fit. The context’s lifetime naturally
spans the entire command execution flow, each command has its own independent
context, and the context itself is designed to support passing information
across components.
The only remaining concern may be performance. At the moment, it does not
appear likely to introduce any significant overhead, but we may still need to
benchmark it after the implementation is complete before drawing a final
conclusion.
For now, I think passing key events through the context is the best solution.
--
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]
