Aetherance commented on code in PR #3541:
URL: https://github.com/apache/kvrocks/pull/3541#discussion_r3729835636
##########
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’ve implemented this refactoring locally and rerun the benchmarks. The
performance issue I was previously concerned about does not appear to exist; in
fact, this version performs slightly better than my previous implementation.
I also found that, with this design, simply adjusting where `notify` is
triggered can significantly reduce the overall complexity and eliminate a
considerable amount of unnecessary code, such as the guards for re-entrant
execution in Lua scripts or `EXEC`.
However, I’m still not sure whether this approach would be acceptable, so
I’d like to hear your thoughts before updating this PR. For now, I’ve opened a
demonstration PR in my own repository with the local implementation:
https://github.com/Aetherance/kvrocks/pull/2
Do you think this approach would be acceptable? I’d like to get your
feedback before proceeding further.
--
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]
