chore: allow config set notify_keyspace_events #3790
Conversation
Signed-off-by: kostas <[email protected]>
| // Does not check for non supported events. Callers must parse the string and reject it | ||
| // if it's not empty and not EX. | ||
| void SetNotifyKeyspaceEvents(std::string_view notify_keyspace_events); |
There was a problem hiding this comment.
Agree but it's only meant to be used in the callback of config set command. There is no real reason to do the parsing here...
src/server/db_slice.cc
Outdated
| void DbSlice::SetNotifyKeyspaceEvents(std::string_view notify_keyspace_events) { | ||
| if (notify_keyspace_events.empty()) { | ||
| for (auto& db : db_arr_) { | ||
| db->expired_keys_events_.clear(); | ||
| } | ||
| } | ||
| expired_keys_events_recording_ = !notify_keyspace_events.empty(); |
There was a problem hiding this comment.
Not sure it's important, but there are lots of small questions about correctness 🤷🏻♂️ Is this change transactional? Do we really discard all previous events? etc...
There was a problem hiding this comment.
Good questions! config set is not transactional so why would this be an exception? Sure you might argue that for certain transactions there might be events that are "lost" but I would say this is fine + the only solution for this would be to make it a global transaction which I don't think it would be a great idea 🤷♂️
As for discarding for all previous events that's something I should check with Valkey. I will get back to you for this.
There was a problem hiding this comment.
@dranikpg I did check how it's implemented in Valkey.
- It does not clear the current keyspace events. So I will address this in a second and push.
- For transactions, yes it's "racy" if the keyspace events are disabled. So for certain transactions they might not include all the keyspace events for expirations but I would say this is fine.
CONFIG SETis not transactional so 🤷
There was a problem hiding this comment.
Let me know if you have any other thoughts
There was a problem hiding this comment.
Ok, if it's racy, let it be racy for us as well 🤷🏻 🙂
We do not allow
notify_keyspace_eventsto be set at runtime viaconfig setcommand. This PR addresses this.Resolves #3708