Skip to content
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

Introduce Hash Field Expiration to the Spring Data Redis framework #3054

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

tishun
Copy link

@tishun tishun commented Nov 25, 2024

This change introduces the Hash Field Expiration feature as part of the features provided by the spring-data-redis project.

As part of the change the following commands would be made available:

  • HEXPIRE - expire a set of hash fields after a specified time, measured in seconds
  • HPEXPIRE - expire a set of hash fields after a specified time, measured in milliseconds
  • HEXPIREAT - expire a set of hash fields at a specified UNIX time, measured in seconds since Unix epoch
  • HPEXPIREAT - expire a set of hash fields at a specified UNIX time, measured in milliseconds since Unix epoch
  • HEXPIRETIME (omitted on the grounds it is missing for keys too)
  • HPERSIST - persist a field that was previously marked for expiration
  • HTTL - return the time-to-live of a given hash field in seconds
  • HPTTL - return the time-to-live of a given hash field in milliseconds

The new commands are available as part of the following interfaces:
(based on similar PR #283)

  • RedisHashCommands
  • HashOperations / BoundHashOperations
  • ReactiveHashOperations
  • StringRedisConnection
  • RedisMap

This feature is available starting from Redis CE version 7.4.x and later

For more information on HFE you can also check out ...
... the blog article on the topic.
... examples of how one can take advantage of the feature in their application


  • You have read the Spring Data contribution guidelines.
  • You use the code formatters provided here and have them applied to your changes. Don’t submit any formatting related changes.
  • You submit test cases (unit or integration tests) that back your changes.
  • You added yourself as author in the headers of the classes you touched. Amend the date range in the Apache license header if needed. For new types, add the license header (copy from another file and set the current year only).

@pivotal-cla
Copy link

@tishun Please sign the Contributor License Agreement!

Click here to manually synchronize the status of this Pull Request.

See the FAQ for frequently asked questions.

@leonovdmitry
Copy link

hi @tishun !

Is it any updates about your PR? i guess it`s wating for you to take action?

@tishun
Copy link
Author

tishun commented Jan 10, 2025

hi @tishun !

Is it any updates about your PR? i guess it`s wating for you to take action?

Hey @leonovdmitry ,
Happy to see there is some interest in the change.

The current agreement with the Pivotal team is for them to try and review and push this change with the 3.5 release. Mid-January was a tentative and nonbinding ETA that I hope we will be able to meet to start work on approving this.

In the mean time if you feel there are use cases that we've missed please let me know.

I have a follow-up commit for the RedisMap that I hope to push today or tomorrow.

@leonovdmitry
Copy link

hi @tishun !
Is it any updates about your PR? i guess it`s wating for you to take action?

Hey @leonovdmitry , Happy to see there is some interest in the change.

The current agreement with the Pivotal team is for them to try and review and push this change with the 3.5 release. Mid-January was a tentative and nonbinding ETA that I hope we will be able to meet to start work on approving this.

In the mean time if you feel there are use cases that we've missed please let me know.

I have a follow-up commit for the RedisMap that I hope to push today or tomorrow.

@tishun thank you for feedback, yep, i was a bit surprised not to find this function in the current library version, but happy to see PR, so thanks for your work.

About use cases, I don't know if this could be part of the core, but it would be convenient to have a method for get/set ttl for only one field with the result in the form of a response code(Long) and not a list of codes(List)

Good luck and have a nice day!

Copy link
Member

@mp911de mp911de left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your pull request. There are some minor things to be updated. Can you please squash your commits and force-push these along with a DCO sign-off in the commit message?

@tishun tishun force-pushed the topic/introduce-hash-field-expiration branch 2 times, most recently from 662c20c to 5b987c3 Compare January 21, 2025 10:28
@tishun tishun force-pushed the topic/introduce-hash-field-expiration branch from 5b987c3 to ea7e8c4 Compare January 21, 2025 10:29
@tishun tishun requested a review from mp911de January 21, 2025 10:30
@tishun
Copy link
Author

tishun commented Jan 21, 2025

Thanks for your pull request. There are some minor things to be updated. Can you please squash your commits and force-push these along with a DCO sign-off in the commit message?

Addressed the review comments and added the signoff.

Also added changes to the RedisMap, please have a look at them so we do not miss them

@mp911de mp911de self-assigned this Jan 27, 2025
@mp911de mp911de added type: enhancement A general enhancement and removed status: waiting-for-triage An issue we've not yet triaged labels Jan 27, 2025
@christophstrobl
Copy link
Member

Thank you again for the PR @tishun.

From what I've seen so far things look pretty good, still I think there are a couple of things we need to take care of before we can actually bring the change in.

It would be nice if:

  • all hash command (imperative/reactive/string) interfaces share an equivalent feature set and implement httl, hpttl, ...
  • hpttl would be issued when requesting ttl with msec precision
  • we use either a long, TimeUnit or Duration method signature - maybe both make sense and we put in some default methods delegating Duration to long, TimeUnit
  • XX, NX, GT, LT options could be passed on. Maybe Expiration can help with that to avoid overloaded methods
  • Reactive-/HashOperations interfaces have an equivalent featureset for ttl operations
  • the return type of commands keeps reference to its input, so that users invoking eg. httl are not forced to memorize the input fields to map them back to the actual values returned by the command
  • reactive methods which require output to be processed in order would, by declaration, lift the burden of paying close attention to enforced sequential mapping from the consuming side.

@tishun
Copy link
Author

tishun commented Feb 10, 2025

Thank you again for the PR @tishun.

Hey @christophstrobl , thanks for spending time and looking at this change!

From what I've seen so far things look pretty good, still I think there are a couple of things we need to take care of before we can actually bring the change in.

Sure! Could you please confirm my understanding of your comments (below)?

It would be nice if:

  • all hash command (imperative/reactive/string) interfaces share an equivalent feature set and implement httl, hpttl, ...

Apologies, I may need some elaboration on share an equivalent feature set. I see I've missed the hpttl command in some of the interfaces, I will add it, is there something else you meant?

If there are some differences in the naming of the API methods it is because I was aiming at consistency with the existing codebase, for example BoundHashOperations.getExpire() is similar to BoundKeyOperations.getExpire(), while RedisHashCommands.hTtl() is similar to RedisKeyCommands.ttl(). All of these end up calling the HTTL command.

  • hpttl would be issued when requesting ttl with msec precision

It is, actually we always are always executing HPTTL even for seconds precision. I was thinking that the HPTTL command could be used in both cases for simplification, but having spent some more time thinking on that it is not correct behavior so I will address that.

  • we use either a long, TimeUnit or Duration method signature - maybe both make sense and we put in some default methods delegating Duration to long, TimeUnit

@mp911de mentioned (weird, I cant see this comment now, is it because I force pushed?) that the long, TimeUnit was an old way of doing Duration and asked that I change all usages to Duration, as this is a new API and we need not maintain two different ways to handle that. Same comment was made about using Instant instead of Date.

I seem to use TimeUnit only for httl (since I combined them both with HPTTL). When I make the change from the previous point I will completely remove the use of TimeUnit and only stick with Duration.

Does that make sense?

  • XX, NX, GT, LT options could be passed on. Maybe Expiration can help with that to avoid overloaded methods

I omitted that because it was missing for key expiration too. If you think it is important I will add it to the existing APIs.
It would end up with some inconsistency, but I don't think it is a big problem in this case, as long as you are okay with it.

  • Reactive-/HashOperations interfaces have an equivalent featureset for ttl operations

Oh, I seem to have missed the entire ReactiveHashOperations interface. So many of them. Will address this too.

  • the return type of commands keeps reference to its input, so that users invoking eg. httl are not forced to memorize the input fields to map them back to the actual values returned by the command

Well ... this would be a rather huge inconsistency if I am getting this right.
None of the existing methods that accept multiple fields actually return the fields (or key) as a result, e.g.

class HashOperations {
   ...
   Long delete(H key, Object... hashKeys);

   List<HV> multiGet(H key, Collection<HK> hashKeys);
   ... 

Did you mean we should, instead of returning a List of results, rather return a Map of <Field, Result>?

  • reactive methods which require output to be processed in order would, by declaration, lift the burden of paying close attention to enforced sequential mapping from the consuming side.

... and not do the above for Reactive methods?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: enhancement A general enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants