-
Notifications
You must be signed in to change notification settings - Fork 472
feat(Type Cache Deprecation): Add FetchFromCache flag and enable/disable caching with respect to flag value #4243
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
base: master
Are you sure you want to change the base?
Conversation
6089975 to
df74e56
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #4243 +/- ##
=======================================
Coverage 83.09% 83.10%
=======================================
Files 154 154
Lines 18980 19008 +28
=======================================
+ Hits 15772 15796 +24
- Misses 2631 2635 +4
Partials 577 577
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
/gemini review |
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.
Code Review
This pull request refactors the GetFolder function across various bucket abstraction layers to accept a *gcs.GetFolderRequest struct instead of a direct folder name string, with corresponding updates to test files. The gcs.GetFolderRequest struct, along with StatObjectRequest and ListObjectsRequest, now includes a FetchFromCache boolean field. Review comments indicate that the FetchFromCache flag is not yet implemented for StatObject within fastStatBucket.StatObject and that fastStatBucket.ListObjects does not currently utilize caching for listings, prompting a need for further implementation or clarification on future plans.
e362824 to
2cf7164
Compare
|
/gemini review |
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.
Code Review
This pull request introduces a FetchFromCache flag to give callers explicit control over using the stat cache, which is a useful feature for performance tuning and consistency. The changes are well-contained, primarily affecting the fastStatBucket and propagating the new request structs through various bucket wrappers.
My review focuses on the new caching logic in fast_stat_bucket.go. I've identified a couple of areas for improvement:
- A potential panic due to an unsafe type assertion in
ListObjects. - A minor formatting inconsistency in an error message.
The addition of extensive unit tests for the new caching scenarios is excellent and greatly improves confidence in the changes. Overall, this is a solid contribution.
384b757 to
b8f1a0e
Compare
Description
The primary goal of this PR is to provide a mechanism to explicitly control whether a metadata request should be served from the local cache or fetched directly from GCS.
Changes By adding FetchFromCache to the request structs, the system can now prioritize fetching responses from the stat cache. If the entry is not found, it will fall back to making GCS calls.
Updates to Fast Stat Bucket
Link to the issue in case of a bug fix.
b/473671143
Testing details
Any backward incompatible change? If so, please explain.