-
Notifications
You must be signed in to change notification settings - Fork 626
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add a key update notification #1144
base: unstable
Are you sure you want to change the base?
Conversation
src/server.c
Outdated
@@ -3537,6 +3537,10 @@ void call(client *c, int flags) { | |||
dirty = server.dirty - dirty; | |||
if (dirty < 0) dirty = 0; | |||
|
|||
if (dirty > 0) { | |||
notifyKeyspaceEvent(NOTIFY_DIRTY, "dirty", c->argv[1], c->db->id); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will not target specific key, but only indicate the dataset is dirty, there are many multi key commands (like mset, eval etc...) which will make many keys dirty. I think maybe a better place is to place it in signalModifiedKey
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for your guidance. I had put it in signalModifiedKey
.
src/notify.c
Outdated
@@ -58,6 +58,7 @@ int keyspaceEventsStringToFlags(char *classes) { | |||
case 'm': flags |= NOTIFY_KEY_MISS; break; | |||
case 'd': flags |= NOTIFY_MODULE; break; | |||
case 'n': flags |= NOTIFY_NEW; break; | |||
case 'c': flags |= NOTIFY_DIRTY; break; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Personally I do not find the 'dirty' as the correct state here. IMO dirty means that the key was modified and was not yet consisted (like for example successfully written to the replica).
Although funny name but maybe: NOTIFY_MODIFY is a better match for what really happened?
Also maybe NOTIFY_UPDATE?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I perfer NOTIFY_UPDATE, because we can use 'u' to stand it. (The letter 'm' has been used)
src/config.c
Outdated
@@ -2906,7 +2906,7 @@ static int setConfigNotifyKeyspaceEventsOption(standardConfig *config, sds *argv | |||
} | |||
int flags = keyspaceEventsStringToFlags(argv[0]); | |||
if (flags == -1) { | |||
*err = "Invalid event class character. Use 'Ag$lshzxeKEtmdn'."; | |||
*err = "Invalid event class character. Use 'Ag$lshzxeKEtmdnc'."; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why 'c'? maybe 'u' (stands for updated)?
@jjz921024 you will have to sign your commit in order for the DCO to pass. |
Signed-off-by: anotherJJz <[email protected]>
Signed-off-by: anotherJJz <[email protected]>
@valkey-io/core-team since this is an interface/config change, tagged it as major decision. can you please take a look? |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## unstable #1144 +/- ##
============================================
- Coverage 70.67% 70.63% -0.04%
============================================
Files 114 114
Lines 61717 61678 -39
============================================
- Hits 43617 43568 -49
- Misses 18100 18110 +10
|
How is this new setting different from |
I agree. this will basically aggregate different events under the same event name. mainly to simplify the client side logic and can be achieved without this. However that got me thinking if we see any value to provide some "update" notification indicating an existing value was changed (as apposed to created/deleted/expired etc...). however that would require a different change introduced. |
@PingXie Hi, It might be different with |
@ranshid Hi, It will aggregate different events under the same event name. But, if we want to provide update notification when a key has been modify. Maybe we should introduce it. |
I assume you meant "value modification" rather than "key modification" here? The current key notification mechanism breaks events down by data/command types like SET, HSET, etc. From my quick look at the source code, it appears to cover all relevant operations. Are you seeing any "update" operations being missed? I'm cautious about introducing another "superset" event, as it might add unnecessary complexity/overhead. BTW, we do have "rename" (or "key modification") events too. |
Yes we do. we have rename_from and rename_to events |
I think, as @PingXie correctly stated, that the current events ARE reflecting most of the mutative data commands. However currently there is not easy way for the client to understand a VALUE was changed other then combining the "new" notification and all other different events which are per operation/datatype. I think it might simplify the client logic to be able to have event notification for "modified" value but as I said this IS currently possible to achieve by clients IMO. |
I have the similar concern as what Ping's: for existing commands, such as saddCommand function, we already have NOTIFY_SET notification, if we introduce Key-update, do we have redundancy behavior for this command? |
I think the current notifications already covers all "update" operations, so there's no need to add a redundant event. |
this pr want to close #1143