-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Added Changes to support Cache pinning in FileCache #17617
base: main
Are you sure you want to change the base?
Added Changes to support Cache pinning in FileCache #17617
Conversation
Signed-off-by: Mayank Sharma <[email protected]>
Signed-off-by: Mayank Sharma <[email protected]>
❌ Gradle check result for 353bcdf: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
@@ -81,15 +71,18 @@ static class Node<K, V> { | |||
|
|||
int refCount; | |||
|
|||
boolean pinned; |
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.
The behavior of this class is defined as:
Cache maintains it's capacity using LRU Eviction while ignoring entries with {@link Node#refCount} greater than 0 from eviction
That means any entry with a reference count greater than zero is effectively "pinned" as it is not eligible for eviction. Do you need to add an explicit pinning function? Instead of requiring the code using this cache to pin and unpin, it seems like you should be able to just ensure you've taken a reference when you want to "pin", and then decrement that reference when you want to "unpin".
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.
Agree with the point that we can always take a reference when we want to 'pin' and decrement that reference for 'unpin' behaviour. This is what we are currently leveraging in Writable Warm as well (pinning file that hasn't been uploaded to remote yet by taking it's reference and then decrementing the reference once it is uploaded to remote).
However I think pinning and reference counting are two separate logical things and doing an incRef to pin and decRef to unpin seems a hacky way of sorts to achieve that. Hence we are proposing adding a separate property altogether. This would also help in segregating the stats for unpinned files(with refCount>0) and pinned files, which otherwise wouldn't have been possible with the earlier hacky way.
Your thought on this @andrross ?
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.
However I think pinning and reference counting are two separate logical things
I'm not sure I agree. The whole point of reference counting is to pin the file to prevent eviction when the count is greater than zero. Pinning/unpinning just seems like a specialized case of reference counting where the count can never go higher than one. Adding this mechanism to get the segregated stats doesn't seem ideal to me. Do you have some other component tracking these pending uploads that could keep track of the stats of files that have yet to be uploaded?
Description
[Describe what this change achieves]
Related Issues
Resolves #[Issue number to be closed when this PR is merged]
Check List
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.