Aetherance commented on PR #3541:
URL: https://github.com/apache/kvrocks/pull/3541#issuecomment-4892250784
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:
```cpp
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:
```cpp
// 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:
```cpp
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:
```cpp
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, `DEL a a a`
should emit only one `del` event, so `MDel` returns the actual deleted keys.
Similarly, other commands may need to expose minimal outcome information such
as whether a conditional write was applied or which keys were really changed.
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.
--
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]
