Add a key update notification

[jjz921024]

this pr want to close #1143

[ranshid]

@jjz921024 you will have to sign your commit in order for the DCO to pass.

[ranshid]

@valkey-io/core-team since this is an interface/config change, tagged it as major decision. can you please take a look?

[codecov[bot]]

Codecov Report

Attention: Patch coverage is 75.00000% with 1 line in your changes missing coverage. Please review.

Project coverage is 70.63%. Comparing base (9b8a061) to head (dcdcfb1). Report is 27 commits behind head on unstable.

Files with missing lines	Patch %	Lines
src/notify.c	50.00%	1 Missing :warning:

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     

Files with missing lines	Coverage Δ
src/config.c	78.64% <100.00%> (-0.06%)	:arrow_down:
src/db.c	88.42% <100.00%> (ø)
src/server.h	100.00% <ø> (ø)
src/notify.c	95.65% <50.00%> (-1.37%)	:arrow_down:

... and 43 files with indirect coverage changes

[PingXie]

@valkey-io/core-team since this is an interface/config change, tagged it as major decision. can you please take a look?

How is this new setting different from g$lshzxet (or just g$lshzxe if stream events are not interesting)? Are there any specific updates that are not covered by the existing event types? The way it is implemented now seems to be a super set of the existing event types.

[ranshid]

@valkey-io/core-team since this is an interface/config change, tagged it as major decision. can you please take a look?

How is this new setting different from g$lshzxet (or just g$lshzxe if stream events are not interesting)? Are there any specific updates that are not covered by the existing event types? The way it is implemented now seems to be a super set of the existing event types.

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.

[jjz921024]

@valkey-io/core-team since this is an interface/config change, tagged it as major decision. can you please take a look?

How is this new setting different from g$lshzxet (or just g$lshzxe if stream events are not interesting)? Are there any specific updates that are not covered by the existing event types? The way it is implemented now seems to be a super set of the existing event types.

@PingXie Hi, It might be different with g$lshzxet. The existing event type does not contain events for which the key is modified. Please let me know if I'm wrong.

[jjz921024]

@valkey-io/core-team since this is an interface/config change, tagged it as major decision. can you please take a look?

How is this new setting different from g$lshzxet (or just g$lshzxe if stream events are not interesting)? Are there any specific updates that are not covered by the existing event types? The way it is implemented now seems to be a super set of the existing event types.

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.

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

[PingXie]

The existing event type does not contain events for which the key is modified. Please let me know if I'm wrong

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.

[ranshid]

BTW, we do have "rename" (or "key modification") events too.

Yes we do. we have rename_from and rename_to events

[ranshid]

The existing event type does not contain events for which the key is modified. Please let me know if I'm wrong

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.

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.

[hwware]

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?

[soloestoy]

I think the current notifications already covers all "update" operations, so there's no need to add a redundant event.

[madolson]

@jjz921024 Can you add more information about what specific problem you are trying to solve. The core team discussed the issue offline, and it feels unnecessary, but we might not be understanding the use case. It seems like you could achieve the same result by just subscribing to all the events and then just ignoring what type of event was being sent.

[jjz921024]

Hi, I tried the AKE notification types and it did cover all update events，I'm sorry I didn't look into it. This pr may be clsoed.

[PingXie]

Thanks @jjz921024!