Aetherance commented on code in PR #3541:
URL: https://github.com/apache/kvrocks/pull/3541#discussion_r3689329309
##########
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:
> Would it be better to emit the event immediately after the deletion is
performed in the storage layer, rather than propagating the information all the
way up to the command layer?
The initial version of this PR used a similar approach, but a reviewer
pointed out that it was not very extensible:
> Yes, I fully agree with this concern. We have an entry point to execute
commands, so it should be possible to catch the changed events/data in one
place instead of doing this in each command.
Therefore, I changed it to the current approach, where events are emitted at
the unified command execution entry point.
--
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]
