Skip to content

feat(keyspace): emit set and del notifications#3541

Open
Aetherance wants to merge 7 commits into
apache:unstablefrom
Aetherance:feat/notify-keyspace-events
Open

feat(keyspace): emit set and del notifications#3541
Aetherance wants to merge 7 commits into
apache:unstablefrom
Aetherance:feat/notify-keyspace-events

Conversation

@Aetherance

@Aetherance Aetherance commented Jul 1, 2026

Copy link
Copy Markdown

Implement keyspace notifications for SET and DEL, compatible with Redis notify-keyspace-events.

This PR adds the initial notification emitters and configuration support:

  • Support K, E, g, $, and A flags, where A currently expands to the implemented event classes g$.
  • Emit set notifications only when SET actually applies a write, including conditional SET variants.
  • Emit del notifications only for keys that are actually deleted.
  • Deduplicate repeated keys in a single DEL, so DEL a a deletes and notifies once.
  • Queue notifications inside MULTI/EXEC and publish them after a successful commit.
  • Map notification DB names correctly:
    • default namespace uses DB 0
    • redis-databases namespaces map back to Redis DB indexes

This PR only adds notification emitters on primary nodes for SET and DEL, 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.

@Aetherance

Copy link
Copy Markdown
Author

I added optional output parameters to String::Set and Database::MDel so the command layer can emit notifications based on what actually changed.

For SET, this avoids duplicating the conditional SET decision logic in the command layer. For DEL, this avoids an extra EXISTS pass before deletion, which would add unnecessary storage reads and overhead. It also lets MDel return the keys it actually deleted.

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 SET would duplicate conditional SET/GET logic outside String::Set, and DEL would need extra reads before MDel. Over time, this could also drift from the actual write behavior.

I chose the optional-output approach for this PR, but I’m open to maintainers’ preference here.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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-events configuration parsing/validation and a server-side NotifyKeyspaceEvent publisher.
  • Emit set notifications only when SET actually writes (including conditional variants) and emit del only for keys actually deleted (including deduping duplicate keys in a single DEL).
  • Queue keyspace events during EXEC and 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.

@sonarqubecloud

sonarqubecloud Bot commented Jul 3, 2026

Copy link
Copy Markdown

@jihuayu jihuayu left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

@Aetherance

Copy link
Copy Markdown
Author

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?Redis 键空间通知将 $ 视为字符串命令类型。除了 SET 之外, SETEXPSETEXGETSETMSETAPPENDINCR*SETRANGE 等命令也会触发字符串相关的事件(如 setappendincrbysetrange 等; SETEX / PSETEX 也会触发 expire 相关事件)。目前的这个 PR 仅针对 SET 这种命令类型发出通知。至于其他字符串命令的处理,是否打算在后续的 PR 中再处理呢?

@jihuayu

Yes, that is intentionally deferred to follow-up PRs.

This PR focuses on the minimal SET and DEL notification support first. Adding the rest of the string command coverage in the same PR would make the change much larger and harder to review, so I intentionally kept the scope limited here.

I’m happy to cover the remaining string commands such as SETEX/PSETEX, GETSET, MSET, APPEND, INCR*, and SETRANGE in follow-up PRs once the basic notification path is accepted.

@jihuayu jihuayu left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you! I’ll wait for others to reply over the next two days.

@git-hulk

git-hulk commented Jul 4, 2026

Copy link
Copy Markdown
Member

@jihuayu I will take a look at this implementation since it might heavily impact the performance.

@PragmaTwice PragmaTwice left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread src/types/redis_string.cc
Comment thread src/storage/redis_db.cc
Comment on lines +162 to +163
rocksdb::Status Database::MDel(engine::Context &ctx, const std::vector<Slice> &keys, uint64_t *deleted_cnt,
std::vector<std::string> *deleted_user_keys) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's better to use Slice or string_view instead.

@git-hulk

git-hulk commented Jul 4, 2026

Copy link
Copy Markdown
Member

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.

@Aetherance

Copy link
Copy Markdown
Author

@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
notifications. These details are only known after the concrete command/type logic evaluates the command semantics, and cannot be derived reliably from the command name, arguments, Status, or reply at the generic execution entry.

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.

@git-hulk

git-hulk commented Jul 4, 2026

Copy link
Copy Markdown
Member

@Aetherance Thanks for your effort and reply.

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
notifications.

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 srv->NotifyKeyspaceEvent. So we can just add one line for each place where we need to emit the event.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants