-
Notifications
You must be signed in to change notification settings - Fork 4
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
base: master
Are you sure you want to change the base?
Evict wave usage metrics after 120 days #565
Conversation
Signed-off-by: munishchouhan <[email protected]>
I think this can be implemented more easily by specifying the value ttl in the redis map
See this pattern wave/src/main/groovy/io/seqera/wave/service/cache/AbstractCacheStore.groovy Lines 70 to 82 in b2790ee
|
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 |
here is a thread saying its not possible |
There is this PR that implemented expiration on hashes |
What's preventing expiring the key ? wave/src/main/groovy/io/seqera/wave/service/metric/impl/MetricsServiceImpl.groovy Lines 106 to 107 in b2790ee
|
This is the key in our case wave/src/main/groovy/io/seqera/wave/service/metric/MetricsCounterStore.groovy Lines 39 to 41 in 909bda1
This is field: wave/src/main/groovy/io/seqera/wave/service/metric/impl/MetricsServiceImpl.groovy Lines 106 to 107 in b2790ee
see here:
|
I think solution 1 here, would make the trick |
|
TLDR;
Essentially an |
Actually this should be done only for *day* metrics, therefore the ones having |
What's wrong with that? |
Signed-off-by: munishchouhan <[email protected]>
Signed-off-by: munishchouhan <[email protected]>
ok you are correct, if i use expiry it does work |
Signed-off-by: munishchouhan <[email protected]>
Signed-off-by: munishchouhan <[email protected]>
Signed-off-by: munishchouhan <[email protected]>
Signed-off-by: munishchouhan <[email protected]>
@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: wave/src/test/groovy/io/seqera/wave/service/counter/impl/RedisCounterProviderTest.groovy Line 87 in abd522a
success test: where everything will be null wave/src/test/groovy/io/seqera/wave/service/counter/impl/RedisCounterProviderTest.groovy Line 100 in abd522a
|
the problem is we are using redis hash, where we have to fields, key and field |
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 |
Hash expiration functionality will be available in jedis 5.2.0 |
Signed-off-by: munishchouhan <[email protected]>
Signed-off-by: munishchouhan <[email protected]>
Signed-off-by: munishchouhan <[email protected]>
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 |
Signed-off-by: munishchouhan <[email protected]>
Signed-off-by: munishchouhan <[email protected]>
Redis v 5.2.0 has been released Now this PR needs Redis 7.4 or later |
This PR will do the following: