-
Notifications
You must be signed in to change notification settings - Fork 148
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
Implement CacheConfig field permitting some operations to return non-expired cache data #547
Conversation
542947e
to
951d541
Compare
a3dbc1c
to
c152a73
Compare
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.
Looks good. Mostly minor comments here and there.
Only slight concern is that we need to check the new cache config flag in quite a few different places. It seems to me that lookup_by_name
could almost be the central place where we check it, if not for one case (when called by getattr
on open
).
Not critical, but it could interesting to explore alternatives where the flag is only used in a single layer: e.g. move the getattr
code to a new SuperblockInner::lookup_by_ino
method, which would also check the flag (to ignore force_revalidate
) and then try the remote lookup.
This plumbs in checks for if the filesystem should maintain strong consistency for operations like open. There is no way to configure mountpoint-s3 itself to relax the consistency model - this change only impacts internals. Signed-off-by: Daniel Carl Jones <djonesoa@amazon.com>
c152a73
to
bd10d31
Compare
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.
Looks good. Feel free to ignore the one tiny nit.
Description of change
This plumbs in checks for if the filesystem should check with S3 to ensure we're consistent with S3 at open, create, etc.
There is no way to configure mountpoint-s3 itself to relax the consistency model - this change only impacts internals.
Relevant issues: #255
Does this change impact existing behavior?
If the new field is set to false, it does impact the consistency model of the
mountpoint-s3::fs::S3Filesystem
. This is not configurable during mounting at this time, but may be in the future.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).