Skip to content

Conversation

sundb
Copy link
Collaborator

@sundb sundb commented Jun 7, 2024

When the hash field expired, we will send a new hexpired notification.
It mainly includes the following three cases:

  1. When field expired by active expiration.
  2. When field expired by lazy expiration.
  3. When the user uses the h(p)expire(at) command, the user will also get a hexpired notification if the field expires during the command.

Improvement

  1. Now if more than one field expires in the hmget command, we will only send a hexpired notification.
  2. When a field with TTL is deleted by commands like hdel without updating the global DS, active expire will not send a notification.

@tezc
Copy link
Collaborator

tezc commented Jun 10, 2024

@sundb There is one issue with hmgetCommand(), it may lazily expire several fields and we'll be sending a notification for each field. I guess ideally, we want to send a single notification similar to active expiry. Maybe we can ignore this issue or maybe you can find a solution for that.

@sundb
Copy link
Collaborator Author

sundb commented Jun 10, 2024

@sundb There is one issue with hmgetCommand(), it may lazily expire several fields and we'll be sending a notification for each field. I guess ideally, we want to send a single notification similar to active expiry. Maybe we can ignore this issue or maybe you can find a solution for that.

you reminded me of this discussion: #13285 (comment)
perhaps it's time to consider taking expiration operations outside, rather than underground.

@tezc
Copy link
Collaborator

tezc commented Jun 10, 2024

you reminded me of this discussion: #13285 (comment)

@sundb lol, I just realize I didn't see your comment there and clicked resolve. Sorry about that.

Yes, it is a similar discussion. I still feel like lazy expiry is not necessary in cases like hget or hmget but maybe the solution is doing expiry outside as you said.

sundb and others added 4 commits June 11, 2024 11:01
1. send "hexpired" notification first and then "del"
2. add signalModifiedKey() in hashTypeGetValue()

Co-authored-by: Ozan Tezcan <ozantezcan@gmail.com>
Co-authored-by: Moti Cohen <moti.cohen@redis.com>
sundb and others added 2 commits June 11, 2024 16:10
Co-authored-by: Ozan Tezcan <ozantezcan@gmail.com>
@sundb
Copy link
Collaborator Author

sundb commented Jun 11, 2024

@tezc i finnaly gave up to move out the expiration code from hashTypeGetValue(), too complex and too many changes.
please have a look the last commit, i choose a simply way to do that for the hmgetcommand.

@tezc
Copy link
Collaborator

tezc commented Jun 11, 2024

@sundb LGTM! It's quite simple and clear!

Co-authored-by: Ozan Tezcan <ozantezcan@gmail.com>
@sundb sundb marked this pull request as ready for review June 11, 2024 10:13
tezc
tezc previously approved these changes Jun 11, 2024
moticless
moticless previously approved these changes Jun 11, 2024
tezc
tezc previously approved these changes Jun 11, 2024
@sundb sundb dismissed stale reviews from tezc and moticless via 3afb587 June 11, 2024 13:25
@sundb sundb changed the title Add new hexpired notification for hash field expiration Add new hexpired notification for HFE Jun 11, 2024
@sundb sundb merged commit ed10f73 into redis:unstable Jun 11, 2024
@sundb sundb added state:needs-doc-pr requires a PR to redis-doc repository release-notes indication that this issue needs to be mentioned in the release notes labels Jun 17, 2024
@YaacovHazan YaacovHazan mentioned this pull request Jun 27, 2024
YaacovHazan added a commit that referenced this pull request Jun 27, 2024
Upgrade urgency LOW: This is the second Release Candidate for Redis 7.4.

Performance and resource utilization improvements
=================================================
* #13296 Optimize CPU cache efficiency

Changes to new 7.4 new features (compared to 7.4 RC1)
=====================================================
* #13343 Hash - expiration of individual fields: when key does not exist
- reply with an array (nonexisting code for each field)
* #13329 Hash - expiration of individual fields: new keyspace event:
`hexpired`

Modules API - Potentially breaking changes to new 7.4 features (compared
to 7.4 RC1)

====================================================================================
* #13326 Hash - expiration of individual fields: avoid lazy expire when
called from a Modules API function
@sundb sundb deleted the hexpired_notification branch August 9, 2024 01:36
sundb added a commit to sundb/redis that referenced this pull request Aug 29, 2024
When the hash field expired, we will send a new `hexpired` notification.
It mainly includes the following three cases:
1. When field expired by active expiration.
2. When field expired by lazy expiration.
3. When the user uses the `h(p)expire(at)` command, the user will also
get a `hexpired` notification if the field expires during the command.

## Improvement
1. Now if more than one field expires in the hmget command, we will only
send a `hexpired` notification.
2. When a field with TTL is deleted by commands like hdel without
updating the global DS, active expire will not send a notification.

---------

Co-authored-by: Ozan Tezcan <ozantezcan@gmail.com>
Co-authored-by: Moti Cohen <moti.cohen@redis.com>
funny-dog pushed a commit to funny-dog/redis that referenced this pull request Sep 17, 2025
When the hash field expired, we will send a new `hexpired` notification.
It mainly includes the following three cases:
1. When field expired by active expiration.
2. When field expired by lazy expiration.
3. When the user uses the `h(p)expire(at)` command, the user will also
get a `hexpired` notification if the field expires during the command.

## Improvement
1. Now if more than one field expires in the hmget command, we will only
send a `hexpired` notification.
2. When a field with TTL is deleted by commands like hdel without
updating the global DS, active expire will not send a notification.

---------

Co-authored-by: Ozan Tezcan <ozantezcan@gmail.com>
Co-authored-by: Moti Cohen <moti.cohen@redis.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-notes indication that this issue needs to be mentioned in the release notes state:needs-doc-pr requires a PR to redis-doc repository
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants