feat(keyspace): emit set and del notifications - #3541
Conversation
|
I added optional output parameters to For The alternative would be to keep these storage APIs unchanged and infer the notification decisions in the command layer. That would keep the APIs smaller, but the trade-off is that I chose the optional-output approach for this PR, but I’m open to maintainers’ preference here. |
There was a problem hiding this comment.
Pull request overview
This PR adds initial Redis-compatible keyspace notification support to Kvrocks by introducing notify-keyspace-events flag parsing and emitting notifications for SET and DEL only when those commands actually apply changes. It also ensures notifications behave correctly with MULTI/EXEC (queued until successful commit) and handles Redis DB index mapping when redis-databases is enabled.
Changes:
- Add
notify-keyspace-eventsconfiguration parsing/validation and a server-sideNotifyKeyspaceEventpublisher. - Emit
setnotifications only whenSETactually writes (including conditional variants) and emitdelonly for keys actually deleted (including deduping duplicate keys in a singleDEL). - Queue keyspace events during
EXECand publish them only after a successful transaction commit; add Go and C++ unit tests for flags and observable pubsub behavior.
Reviewed changes
Copilot reviewed 19 out of 19 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| tests/gocase/unit/keyspacenotify/keyspacenotify_test.go | New Go tests validating SET/DEL keyspace+keyevent messages, conditional SET behavior, runtime CONFIG SET, MULTI/EXEC queuing, and redis-databases DB mapping. |
| tests/gocase/unit/keyspace/keyspace_test.go | Extends existing DEL tests to assert duplicate keys are only counted/deleted once. |
| tests/cppunit/keyspace_events_test.cc | New C++ unit tests for notify flag parsing, A expansion, unsupported flag rejection, and namespace→DB mapping. |
| src/common/keyspace_events.h / src/common/keyspace_events.cc | New helpers for parsing notify-keyspace-events and mapping namespace to keyspace DB name (with percent-encoding for non-db namespaces). |
| src/config/config.h / src/config/config.cc | Adds stored parsed notify flags and wires up the notify-keyspace-events config field with validation and runtime callback parsing. |
| src/server/server.h / src/server/server.cc | Adds Server::NotifyKeyspaceEvent that filters by configured flags and publishes keyspace then keyevent messages. |
| src/server/redis_connection.h / src/server/redis_connection.cc | Adds per-connection queueing for keyspace events during EXEC, flushing after successful commit, and clearing on reset/abort. |
| src/commands/cmd_txn.cc | Flushes queued keyspace events only after a successful CommitTxn() in EXEC. |
| src/types/redis_string.h / src/types/redis_string.cc | Extends String::Set with an applied out-param to distinguish “command OK” from “write actually happened”. |
| src/commands/cmd_string.cc | Emits set notifications only when String::Set reports an applied write and notifications are enabled. |
| src/storage/redis_db.h / src/storage/redis_db.cc | Extends MDel to optionally return the list of deleted user keys, and deduplicates repeated keys so deletes/counts match Redis behavior. |
| src/commands/cmd_key.cc | For DEL, requests deleted key list from MDel and emits one del notification per actually-deleted key (skipping non-deletes and duplicates). |
| kvrocks.conf | Documents the new keyspace notification configuration and supported flags. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
jihuayu
left a comment
jihuayu
left a comment
There was a problem hiding this comment.
Looks great. I like the code you wrote.
Redis keyspace notifications treat $ as the string command class. Besides SET, commands such as SETEX/PSETEX, GETSET, MSET, APPEND, INCR*, and SETRANGE also emit string events (set, append, incrby, setrange, etc.; SETEX/PSETEX also emit expire). This PR currently only emits notifications for literal SET. Is the remaining string command coverage intentionally deferred to a follow-up PR?
Yes, that is intentionally deferred to follow-up PRs. This PR focuses on the minimal I’m happy to cover the remaining string commands such as |
jihuayu
left a comment
jihuayu
left a comment
There was a problem hiding this comment.
Thank you! I’ll wait for others to reply over the next two days.
|
@jihuayu I will take a look at this implementation since it might heavily impact the performance. |
PragmaTwice
left a comment
PragmaTwice
left a comment
There was a problem hiding this comment.
I don’t think this keyspace notification design is scalable.
Adding more code for each new event is easy, especially with AI agents. But that is not the real problem. The real problem is whether the design is maintainable.
This PR does not solve that. It adds duplicated code for each event and requires changes in many command implementations. I don’t see a clear shared abstraction here.
I don’t think we should merge this in the current form. We should first design a better general mechanism.
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. |
|
@PragmaTwice @git-hulk Thanks for the careful review. When I started implementing keyspace notifications, I also considered using a single common trigger point. However, while working on SET/DEL, I found that the generic command execution entry does not have enough semantic information to generate correct notifications. For example, a conditional SET may return OK without actually writing the key, such as SET ... NX when the key already exists. DEL also needs to know the exact keys that were actually removed; cases like DEL k k k must not emit duplicate I agree the current implementation is not scalable enough. I’ll revisit the design and try to centralize the shared parts better. I also think it may not be realistic to add more keyspace notification coverage without touching individual command/type implementations at all, because some event decisions still need to be made where the Redis semantics are fully known. |
|
@Aetherance Thanks for your effort and reply.
After iterating on the current implementation and proposal, I understood your intention. And yes, it's a bit tricky to depend on rocksdb WAL to achieve this feature. For the implementation, we can check config flags, channel names, Pub/Sub publishing in one place like |
36915d9 to
ac611f8
Compare
This comment was marked as outdated.
This comment was marked as outdated.
2911a37 to
e2f6660
Compare
|
|
It looks like the full CI has not run on the latest commit. The previous CI run failed during startup because it was blocked by the repository’s action policy. Could a maintainer please help re-run or enable the CI when convenient? |
|
I found that this PR has significant room for performance optimization. I’ll convert it to a draft for now and request a review once I’ve completed the optimizations. Thanks for all reviewers! |
Aetherance
left a comment
Aetherance
left a comment
There was a problem hiding this comment.
To make the review easier, I added inline comments in several places where the intent may not be immediately clear from the code itself. I hope this makes the implementation easier to understand.
Thanks for the review! 🙏
|
|
||
| #include "status.h" | ||
|
|
||
| // Flags for notify-keyspace-events, separate from RedisType. |
There was a problem hiding this comment.
To reduce the review burden and keep the scope of the changes manageable, this PR only adds support for the SET and DEL commands. Currently, only the $ and g event classes are supported. Support for other commands will be added in follow-up PRs.
| #include "config/config.h" | ||
| #include "fmt/format.h" | ||
|
|
||
| bool ShouldNotifyKeyspaceEvent(int notify_flags, int type_flag) { |
There was a problem hiding this comment.
A notification is generated here only when both conditions are met: the event type is enabled, and at least one of K or E is enabled.
| auto start = std::chrono::high_resolution_clock::now(); | ||
| bool is_profiling = IsProfilingEnabled(cmd_name); | ||
|
|
||
| keyspace_event_notify_flags_ = srv_->GetConfig()->notify_keyspace_events; |
There was a problem hiding this comment.
The keyspace_event_collector is created only when keyspace notifications are enabled, and notifications are emitted only when the collector exists. Therefore, when keyspace notifications are disabled—or when a command’s event type is not enabled—the command skips all notification-related work except for a few lightweight condition checks. As a result, the performance impact in these cases should be negligible.
There was a problem hiding this comment.
By the way, support for additional commands can be added by simply calling conn->AddKeyspaceEvent. Some minor changes may also be needed to accurately capture the full semantics of the event, but no major changes to the notification mechanism should be necessary.
| // Retain capacity for typical transactions, but request releasing unusually large buffers. | ||
| constexpr std::size_t kMaxRetainedKeyspaceEvents = 1024; | ||
| if (pending_keyspace_events_.capacity() > kMaxRetainedKeyspaceEvents) { | ||
| pending_keyspace_events_.shrink_to_fit(); |
There was a problem hiding this comment.
clear() does not release the vector’s capacity, so a large transaction may cause a long-lived connection to retain excessive memory and potentially lead to OOM. To mitigate this, I call shrink_to_fit() when the capacity exceeds 1,024.
For small transactions, we can simply call clear() and reuse the allocated memory.
| } | ||
|
|
||
| void Server::NotifyKeyspaceEvent(int flags, const std::string &event, const std::string &ns, const std::string &key) { | ||
| const std::string db = MapNamespaceToKeyspaceDB(ns, GetConfig()->redis_databases); |
There was a problem hiding this comment.
Kvrocks has both ns and db. MapNamespaceToKeyspaceDB provides a unified representation of the two when naming Pub/Sub channels.
|
|
||
| rocksdb::Status String::Set(engine::Context &ctx, const std::string &user_key, const std::string &value, | ||
| const StringSetArgs &args, std::optional<std::string> &ret) { | ||
| const StringSetArgs &args, std::optional<std::string> &ret, bool *applied) { |
There was a problem hiding this comment.
Here, Status::OK does not necessarily mean that a conditional SET was applied, so an additional applied flag is needed. So does it in DEL
| redis::Database redis(srv->storage, conn->GetNamespace()); | ||
|
|
||
| auto s = redis.MDel(ctx, keys, &cnt); | ||
| const bool notify_del = GetAttributes()->name == "del" && conn->IsKeyspaceEventEnabled(kNotifyGeneric); |
There was a problem hiding this comment.
UNLINK is intentionally excluded here. Whether to support it will be decided in a follow-up PR or discussion.
|
I ran multiple rounds of local performance benchmarks using For example, the SET benchmark was run using the following command: redis-benchmark -h 127.0.0.1 -p 6670 --threads 4 -c 64 -P 16 \
-n 1000000 -r 1000000000 --seed 2002 --csv \
SET bench:set:__rand_int__ 0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdefThe benchmarks covered the Kvrocks Kvrocks
Redis reference
I ran multiple rounds of these benchmarks and observed no significant variation between runs. The results show no meaningful performance difference between the current PR and the original With Because Redis and Kvrocks use different storage engines, their absolute RPS values are not directly comparable. Instead, the relevant comparison is the relative performance change before and after notifications are enabled. The official Redis documentation also notes that keyspace notifications are disabled by default partly because the feature consumes additional CPU resources. Redis Keyspace Notify To further identify the source of the performance overhead, I profiled the PR branch using Linux SET 
DEL 
The call graphs show that the performance bottleneck is in |
|
@Aetherance Wow, you’re incredibly thorough, and the diagrams are excellent. I really like them. I’m currently away on a business trip and have been quite busy, but I’ll make time to take a look. Thank you for your patience. |
@jihuayu Thank you! I hope the comments and diagrams make this large PR easier to review—it’s my responsibility as the author. Feel free to reach out with any questions. |
| for (const auto &key : deleted_keys) { | ||
| conn->AddKeyspaceEvent(kNotifyGeneric, "del", std::string_view(key.data(), key.size())); | ||
| } | ||
|
|
There was a problem hiding this comment.
cc @git-hulk @PragmaTwice
Do you think this is a good design? My concern is that, with this approach, all the affected keys would need to be propagated from the storage layer up to the command layer, and every storage-layer method would need an additional field for them.
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?
There was a problem hiding this 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.
There was a problem hiding this comment.
According to my first commit: 44d332b#diff-4680a40ab45d8476a5a37bb17d76163432115086c0fa2dd613ea80e5aee1cdfbR393
There was a problem hiding this comment.
What I mean is, could we emit the event here instead of propagating it up to the command layer?
Please don’t rush to make changes. I’d like to hear what others think first.
There was a problem hiding this comment.
Thank you for the patient explanation.
I considered emitting the notification directly here when I first started implementing this PR, since this is indeed the cleanest place to access the complete semantics of the operation. I ultimately decided against it for the following reasons, which I hope help explain the decision:
The main reason is that this layer has no access to Server or Connection, so it cannot call PublishMessage. I also don't think the storage layer should be responsible for publishing messages.
There was a problem hiding this comment.
Also, if this PR should be split into several smaller PRs, should I first refactor the current PR to use the context-based approach you suggested, and then split it? Or should I implement this approach only after the PR has been split?
There was a problem hiding this 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:
Do you think this approach would be acceptable? I’d like to get your feedback before proceeding further.
There was a problem hiding this comment.
You can take a quick look at this implementation. If the approach looks good, we can continue with a more detailed review in this PR.
There was a problem hiding this comment.
I’ve added some comments to explain the refactored implementation. I hope they’re helpful.
|
@Aetherance I think this PR would be a great fit for stacked PRs. This is not a requirement. If you are interested in stacked PRs and would like to try them out, you could split this PR into a stack of smaller PRs. If that feels like too much trouble, you do not have enough time, or you are simply not interested, please feel free to ignore this message. This is only an invitation in case the idea interests you. |
Hi @jihuayu . Thanks for the suggestion! I’d be happy to try this approach. However, since this PR is being submitted from a fork and I don’t have permission to push branches directly to
From: If the native stacked PR workflow is indeed not applicable here, I’d also be happy to split the current PR into several regular PRs. |
|
@Aetherance I think your new implementation looks good. You can merge the code in, and then we can have the other reviewers take a look as well. Regarding stacked PRs, I checked that they’re indeed not supported yet. The official response says it’s on the roadmap github/gh-stack#46, so we can leave it aside for now. |
Got it! Thank you for your patience. |
FCALL can apply writes before a later statement returns an error. Publish the events collected for those successful writes regardless of the final command status.
|
Hi @jihuayu, sorry to bother you again. It looks like the CI needs maintainer approval before it can run. Could you please help run the CI when you have a chance? |
|
Hi @git-hulk. I think the new abstraction looks good. Could you take a look? |
|
Some CI checks failed, but judging from the error logs, the failures don't appear to have been introduced by this PR. |
|
Hi @git-hulk @PragmaTwice , we’d love to hear your thoughts on this PR. Would you have time to take a look? |
Implement initial keyspace notifications for set and del events, compatible with Redis notify-keyspace-events.
This PR adds the initial notification emitters and configuration support:
K,E,g,$, andAflags, whereAcurrently expands to the implemented event classesg$.setnotifications only when the sharedSetpath actually applies a write, including conditionalSETvariants.delnotifications only for keys that are actually deleted through the shared delete path.MULTI/EXECand publish them after a successful commit.0redis-databasesnamespaces map back to Redis DB indexesThis PR only adds the initial set and del event paths on primary nodes, keeping the initial patch small and reviewable. Commands sharing the same underlying mutation APIs may also produce these events. Replica-side notifications and additional event types, if supported, can be added in follow-up PRs.
Unsupported notification classes are rejected for now until their emitters are implemented.
Ref Proposal: #3533
Tracking Issue: #2915
Assisted by Codex/GPT-5.5.