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

Evict wave usage metrics after 120 days #565

Open
wants to merge 21 commits into
base: master
Choose a base branch
from

Conversation

munishchouhan
Copy link
Member

This PR will do the following:

  1. add functionality to scan and delete keys with specific dates.
  2. add corn to run every day and delete keys with current date - 120

Signed-off-by: munishchouhan <[email protected]>
@munishchouhan munishchouhan linked an issue Jul 15, 2024 that may be closed by this pull request
@munishchouhan munishchouhan marked this pull request as draft July 15, 2024 12:30
@munishchouhan munishchouhan self-assigned this Jul 15, 2024
@pditommaso
Copy link
Contributor

I think this can be implemented more easily by specifying the value ttl in the redis map

See this pattern

@Override
void put(String key, V value, Duration ttl) {
delegate.put(key0(key), serialize(value), ttl)
}
@Override
boolean putIfAbsent(String key, V value, Duration ttl) {
delegate.putIfAbsent(key0(key), serialize(value), ttl)
}
boolean putIfAbsent(String key, V value) {
delegate.putIfAbsent(key0(key), serialize(value), getDuration())
}

@munishchouhan
Copy link
Member Author

The problem is we are using hash in this case and TTL is normally applied on the key and not on the fields, I will dig further to see if there is any way to achieve this for a filed in a key

@munishchouhan
Copy link
Member Author

@munishchouhan
Copy link
Member Author

There is this PR that implemented expiration on hashes
redis/redis#13172
its available on version 7.2

@pditommaso
Copy link
Contributor

What's preventing expiring the key ?

def org = getOrg(email)
def key = getKey(prefix, LocalDate.now().format(DATE_FORMATTER), null)

@munishchouhan
Copy link
Member Author

munishchouhan commented Jul 15, 2024

This is the key in our case

protected String getPrefix() {
return 'metrics/v1'
}

This is field:

def org = getOrg(email)
def key = getKey(prefix, LocalDate.now().format(DATE_FORMATTER), null)

see here:

@pditommaso
Copy link
Contributor

I think solution 1 here, would make the trick

@munishchouhan
Copy link
Member Author

I think solution 1 here, would make the trick

I caanot access this
Screenshot 2024-07-16 at 10 37 09

@pditommaso
Copy link
Contributor

pditommaso commented Jul 16, 2024

TLDR;

Set the TTL on the entire hash map key:

HINCRBY myhash field 1
EXPIRE myhash 60  # Set TTL of 60 seconds on the entire hash map

Essentially an inc followed by an expire operation

@pditommaso
Copy link
Contributor

Actually this should be done only for *day* metrics, therefore the ones having /d/<date> in the key

@munishchouhan
Copy link
Member Author

There is this method for expiry. but it still only required key and not field
Screenshot 2024-07-16 at 12 36 57

@pditommaso
Copy link
Contributor

What's wrong with that?

Signed-off-by: munishchouhan <[email protected]>
Signed-off-by: munishchouhan <[email protected]>
@munishchouhan
Copy link
Member Author

What's wrong with that?

ok you are correct, if i use expiry it does work
I will develop it using this

Signed-off-by: munishchouhan <[email protected]>
Signed-off-by: munishchouhan <[email protected]>
Signed-off-by: munishchouhan <[email protected]>
@munishchouhan
Copy link
Member Author

@pditommaso I have added two test failing and success to show why expire with key wont work, because we need to evict the field in a key and not the whole key, i am using 1 second as ttl

failing test:

success test: where everything will be null

@munishchouhan
Copy link
Member Author

the problem is we are using redis hash, where we have to fields, key and field
expirying key does not help our use case

@pditommaso
Copy link
Contributor

Considering this is not critical. I'd say to park until Redis 7.2 is available, so that we can use expired on the field

@munishchouhan munishchouhan added the on hold put on hold label Jul 17, 2024
@munishchouhan
Copy link
Member Author

Hash expiration functionality will be available in jedis 5.2.0
https://github.com/redis/jedis/releases/tag/v5.2.0-beta4
and required redis 7.4

@munishchouhan
Copy link
Member Author

Hash expiration functionality has been added, but the jedis dependency is still in beta, once the final version is released. I will make this pr ready for review

@munishchouhan
Copy link
Member Author

Redis v 5.2.0 has been released
https://github.com/redis/jedis/releases/tag/v5.2.0

Now this PR needs Redis 7.4 or later

@munishchouhan munishchouhan marked this pull request as ready for review October 2, 2024 13:53
@munishchouhan munishchouhan added blocker and removed on hold put on hold labels Oct 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Evict metrics from redis after 120 days
2 participants