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
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
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
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.
| rocksdb::Status Database::MDel(engine::Context &ctx, const std::vector<Slice> &keys, uint64_t *deleted_cnt, | ||
| std::vector<std::string> *deleted_user_keys) { |
There was a problem hiding this comment.
I think it's better to use Slice or string_view instead.
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 |



Implement keyspace notifications for
SETandDEL, compatible with Redisnotify-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 whenSETactually applies a write, including conditionalSETvariants.delnotifications only for keys that are actually deleted.DEL, soDEL a adeletes and notifies once.MULTI/EXECand publish them after a successful commit.0redis-databasesnamespaces map back to Redis DB indexesThis PR only adds notification emitters on primary nodes for
SETandDEL, keeping the initial patch small and reviewable. Replica-side notifications and additional commands, 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.