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

FIELDEXPIRE: update expiration time without updating the hash's value #3027

Closed
Le0Developer opened this issue May 8, 2024 · 14 comments
Closed
Assignees
Labels
enhancement New feature or request good first issue Good for newcomers hacktoberfest Participates in Hacktoberfest help wanted Extra attention is needed

Comments

@Le0Developer
Copy link

Is your feature request related to a problem? Please describe.
For ratelimit tracking, we currently use hashes instead of full keys for more efficient bulk retrieval (instead of using hash slots when clustering).

Currently, we need to do HINCRBY key field 1 and if the value is 1, we do a followup HSETEX key date field 1.
This means increments that happen between the HINCRBY and HSETEX command may be lost because the HSETEX command will always reset it to 1.

The EXPIRE command exists to solve this exact problem, so having HEXPIRE would prevent race conditions nicely here too.

Describe the solution you'd like
HEXPIRE key field duration

Related
#3026 for HTTL

@romange romange added enhancement New feature or request help wanted Extra attention is needed labels May 8, 2024
@romange
Copy link
Collaborator

romange commented Jul 4, 2024

we have "FIELDTTL" which applies to both hashes and sets. We probably would it to be "FIELDEXPIRE key seconds field field..."
that works for both sets and hashes

@romange romange added the good first issue Good for newcomers label Jul 4, 2024
@azuredream
Copy link
Contributor

Hi @romange,
I'm llooking into this. Could you assign this to me?
I understood that we'll implement FIELDEXPIRE in generic_family.cc?

@romange
Copy link
Collaborator

romange commented Sep 14, 2024

yes please, it should work with both sets and hashes.

@azuredream
Copy link
Contributor

yes please, it should work with both sets and hashes.

I'd like to double check that what you want is "FIELDEXPIRE key sec field1 field2 .." and the return value would be an array of results (0/1). Or, maybe an array of pairs, each item is {key, 0/1}.

Potential Issue:
If a command sets expiration times for multiple fields simultaneously, could there be a scenario where the first field expires before the command completes?
In Redis and Dragonfly, the EXPIRE command does not support batch operations.

@romange romange changed the title HEXPIRE: update expiration time without updating the hash's value FIELDEXPIRE: update expiration time without updating the hash's value Sep 15, 2024
@romange
Copy link
Collaborator

romange commented Sep 15, 2024

@azuredream yes the command should return an array of statuses if it succeeded or an error like wrong type or syntax error otherwise.
Each item in the array should have:

  1. -2 if no field exists
  2. 1 if the expire time has been successfully updated
  3. 2 if fields have been deleted because seconds was 0.

@NegatioN
Copy link
Contributor

NegatioN commented Sep 18, 2024

@azuredream I don't mean for this to sound as nagging/pestering, but just for adjusting my expectations: How long do you think it'll take you to implement? I was also hacking away at this, but I see you've made enough contributions that this will probably take you way less time to get to a working state.

I was hoping to use HEXPIRE in a project in the semi-near future, and it's absolutely awesome if you're enabling that! 🙏

@azuredream
Copy link
Contributor

azuredream commented Sep 18, 2024

@azuredream I don't mean for this to sound as nagging/pestering, but just for adjusting my expectations: How long do you think it'll take you to implement? I was also hacking away at this, but I see you've made enough contributions that this will probably take you way less time to get to a working state.

I was hoping to use HEXPIRE in a project in the semi-near future, and it's absolutely awesome if you're enabling that! 🙏

I've finished research but not started coding yet. If you already have the impl please feel free to open PR.
As for my plan, I'm aiming to open PR by the end of this week.
This issue has been here for months, I hadn't prioritized it as immediate...

@romange
Copy link
Collaborator

romange commented Sep 18, 2024

As a maintainer - it's a good problem to have 😆 There are enough issues for everyone, we will open a new batch of issues next week. @NegatioN do you need any help? Can you estimate when you can submit a draft PR?

@azuredream we also havee this long standing issue:
#1878

I remember you have made substantial contributions to geo API. would you like to take a stab at it?

@azuredream
Copy link
Contributor

azuredream commented Sep 18, 2024

As a maintainer - it's a good problem to have 😆 There are enough issues for everyone, we will open a new batch of issues next week. @NegatioN do you need any help? Can you estimate when you can submit a draft PR?

@azuredream we also havee this long standing issue: #1878

I remember you have made substantial contributions to geo API. would you like to take a stab at it?

Either works for me. Please feel free to move this to him. And I'll work on 1878.
Looking forward to seeing new tasks next week.

@NegatioN
Copy link
Contributor

@azuredream I'm sorry if I offended, and made you think I was expecting things to get solved very quickly. That was not my intention. The end of the week would be more than quickly enough for my needs anyways.

I'll try to take a stab at it though 👍
What I have atm is something which mostly supports HEXPIRE in itself, with a few minor quirks to iron out. Plus I would need to make it fit the desired FIELDEXPIRE-solution that was specified here, and support sets.

@romange I might need some pointers along the way. Is it best if I jump on the discord and ask there, if I do?

@romange
Copy link
Collaborator

romange commented Sep 19, 2024

Sure! :) You can start with any data structure first - sets or hashes and make it work for them if it's easier for you.
The change is not simple at all and requires understanding of how dense_set.* works - right now its API does not support ttl mutations.

@NegatioN
Copy link
Contributor

NegatioN commented Oct 4, 2024

This should now be done 🙂 Thanks again for the patience and help on the journey.
When do you foresee the next release to be done, if I might ask?

@kostasrim
Copy link
Contributor

@NegatioN not really sure 😄 Maybe @romange knows

@romange
Copy link
Collaborator

romange commented Oct 4, 2024

Thank you very much @NegatioN ! 🙏🏼 Most probably end of October.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good first issue Good for newcomers hacktoberfest Participates in Hacktoberfest help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

5 participants