dragonflydb / dragonfly

A modern replacement for Redis and Memcached
https://www.dragonflydb.io/
Other
25.47k stars 928 forks source link

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

Closed Le0Developer closed 3 days ago

Le0Developer commented 5 months ago

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 commented 3 months ago

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

azuredream commented 3 weeks ago

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 commented 3 weeks ago

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

azuredream commented 3 weeks ago

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 commented 3 weeks ago

@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 commented 2 weeks ago

@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! :pray:

azuredream commented 2 weeks ago

@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 commented 2 weeks ago

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: https://github.com/dragonflydb/dragonfly/issues/1878

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

azuredream commented 2 weeks ago

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 commented 2 weeks ago

@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 :+1: 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 commented 2 weeks ago

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 commented 3 days ago

This should now be done :slightly_smiling_face: 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 commented 3 days ago

@NegatioN not really sure :smile: Maybe @romange knows

romange commented 3 days ago

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