-
Notifications
You must be signed in to change notification settings - Fork 182
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
Add negative metadata cache ttl #1246
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: notoriaga <[email protected]>
Signed-off-by: notoriaga <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @notoriaga, thank you for opening this pull request!
I've got a few suggestions, but the next step would be to add a couple of tests. See existing tests for the negative cache, e.g. test_negative_lookup_with_caching
in superblock.rs
and lookup_with_negative_cache
in lookup_test.rs
. Happy to help if you need more guidance.
pub fn with_negative_cache_ttl(self, negative_cache_ttl: TimeToLive) -> Self { | ||
match negative_cache_ttl { | ||
TimeToLive::Minimal => Self { | ||
negative_cache_ttl: Self::default().negative_cache_ttl, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just setting the TTL here may not be enough. We use the same serve_lookup_from_cache
flag both for the "positive" cache and the negative cache (here, in particular). I suspect we need to introduce a separate flag and ensure it is set to false on Minimal
here.
Co-authored-by: Alessandro Passaro <[email protected]> Signed-off-by: Steven Meyer <[email protected]>
Co-authored-by: Alessandro Passaro <[email protected]> Signed-off-by: Steven Meyer <[email protected]>
Signed-off-by: notoriaga <[email protected]>
Adds a new CLI argument
--negative-cache-ttl
that lets you set the TTL for negative metadata entries separately from--metadata-ttl
. My use case is a write once read many bucket. Objects do not get deleted from this bucket, and new objects are added every few minutes. I'd like to be able to set--metadata-ttl indefinite
and--negative-cache-ttl 60
to effectively utilize the caching while still being able to pick up new objects. There is an open issue for this here - #831Does this change impact existing behavior?
No, if
--negative-cache-ttl
is omitted the existing behavior is maintained (use--metadata-ttl
or the default file_ttl).Does this change need a changelog entry? Does it require a version change?
Because this is a new feature I believe it would require both.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license and I agree to the terms of the Developer Certificate of Origin (DCO).