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 |
b742ae5 to
36915d9
Compare
36915d9 to
ac611f8
Compare
|
Hi @PragmaTwice @git-hulk, I have updated the design to introduce a common keyspace notification collection/publishing mechanism around the command execution entry point. The common event representation is: struct KeyspaceEvent {
// Event class flag such as g/$, not K/E channel selectors.
int type_flag;
std::string event;
std::string ns;
std::string key;
};A reusable collector is attached to each command execution: // Collects semantic keyspace events for one command; Connection owns publish
// timing.
class KeyspaceEventCollector {
public:
// Sets namespace/config for the next command and drops any previous events.
void Begin(std::string ns, int notify_flags);
bool IsEnabled(int type_flag) const;
void Add(int type_flag, std::string_view event, std::string_view key);
// Moves out events collected during Execute.
std::vector<KeyspaceEvent> Take();
private:
int notify_flags_ = 0;
std::string ns_;
std::vector<KeyspaceEvent> events_;
};The command execution entry point now owns the shared notification lifecycle: begin collection, execute the command, take the collected events, and publish or queue them only if the command succeeds. The rest of the non-notification execution flow remains unchanged: Status Connection::ExecuteCommand(..., Commander *current_cmd, std::string *reply) {
// existing non-notification logic ...
current_cmd->BeginKeyspaceEventCollection(
GetNamespace(), srv_->GetConfig()->notify_keyspace_events);
auto s = current_cmd->Execute(ctx, srv_, this, reply);
auto events = current_cmd->TakeKeyspaceEvents();
if (s.IsOK()) {
QueueOrPublishKeyspaceEvents(std::move(events));
}
// existing non-notification logic ...
return s;
}With this structure, notification support for a command follows the same pattern: the command/type layer reports factual semantic events into the collector, and the common execution path handles filtering, command success, transaction queueing, and publishing. For example: if (set_applied) {
keyspace_event_collector_.Add(kNotifyString, "set", args_[1]);
}There is still some command-specific work needed when adding notification support for more commands, because the exact event semantics cannot always be inferred from the command name or arguments alone. For example, So the common mechanism is now centralized and reusable, while each command only provides the minimal factual outcome needed to build the correct event list. Does this direction look reasonable to you? If there is still something you would like to see changed, please let me know and I will update it. |
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? |


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.