-
Notifications
You must be signed in to change notification settings - Fork 313
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
feat: enable caching when using object store #928
Conversation
Codecov Report
@@ Coverage Diff @@
## develop #928 +/- ##
===========================================
- Coverage 86.14% 85.59% -0.55%
===========================================
Files 434 445 +11
Lines 58411 59936 +1525
===========================================
+ Hits 50316 51305 +989
- Misses 8095 8631 +536
Flags with carried forward coverage won't be shown. Click here to find out more.
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
Looks great. It's a pity that opendal doesn't support cache eviction by disk or memory quota, the cached objects may consume too much memory or disk. But I think it's okay right now, we still have to design and impl a more powerful cache layer for object storages. |
I would prefer to leave memory to a more fine-grained cache manager like page cache or query cache. They are much more expensive than disks and may not provide a comparable performance improvement when used to cache the entire file. |
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.
LGTM
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.
Looking great 👍 Thanks @e1ijah1, I only left a nit-picking style comment.
* feat: enable caching when using object store * feat: support file cache for object store * feat: maintaining the cached files with lru * fix: improve the code * empty commit * improve the code
I hereby agree to the terms of the GreptimeDB CLA
What's changed and what's your intention?
Enable cache layer by default when using s3/OSS storage in the GreptimeDB.
Checklist
Refer to a related PR or issue link (optional)
closes #918