Skip to content
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

Read blob from blob cache if exists when GetBlob() #10178

Closed
wants to merge 15 commits into from

Conversation

gangliao
Copy link
Contributor

@gangliao gangliao commented Jun 15, 2022

Summary:

There is currently no caching mechanism for blobs, which is not ideal especially when the database resides on remote storage (where we cannot rely on the OS page cache). As part of this task, we would like to make it possible for the application to configure a blob cache.

Tasks:

In this task, we added a new abstraction layer BlobSource to retrieve blobs from either blob cache or raw blob file. Note: For simplicity, the current PR only includes GetBlob(). MultiGetBlob() will be included in the next PR.

This PR is a part of #10156

Copy link
Contributor

@ltamasi ltamasi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR @gangliao ! Took a first pass

db/blob/blob_source.cc Outdated Show resolved Hide resolved
db/blob/blob_source.cc Outdated Show resolved Hide resolved
db/blob/blob_source.cc Outdated Show resolved Hide resolved
db/blob/blob_source.cc Outdated Show resolved Hide resolved
db/blob/blob_source.cc Outdated Show resolved Hide resolved
db/blob/blob_source.cc Outdated Show resolved Hide resolved
db/blob/blob_source.cc Outdated Show resolved Hide resolved
db/blob/blob_source.cc Outdated Show resolved Hide resolved
db/blob/blob_source.cc Outdated Show resolved Hide resolved
db/blob/blob_source.h Outdated Show resolved Hide resolved
Copy link
Contributor

@riversand963 riversand963 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @gangliao for the PR. Left some minor comments.

db/blob/blob_source.h Outdated Show resolved Hide resolved
db/blob/blob_source.h Outdated Show resolved Hide resolved
db/blob/blob_source.h Show resolved Hide resolved
db/blob/blob_source.cc Outdated Show resolved Hide resolved
db/blob/blob_source.h Outdated Show resolved Hide resolved
db/blob/blob_source.cc Outdated Show resolved Hide resolved
Copy link
Contributor

@ltamasi ltamasi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the updates @gangliao ! 👍👍

db/blob/blob_source.cc Outdated Show resolved Hide resolved
db/blob/blob_source.cc Outdated Show resolved Hide resolved
db/blob/blob_source.cc Outdated Show resolved Hide resolved
db/blob/blob_source.cc Outdated Show resolved Hide resolved
db/blob/blob_source.cc Outdated Show resolved Hide resolved
db/blob/blob_source.cc Outdated Show resolved Hide resolved
db/blob/blob_source.h Outdated Show resolved Hide resolved
db/blob/blob_source.h Outdated Show resolved Hide resolved
db/blob/blob_source_test.cc Outdated Show resolved Hide resolved
db/blob/blob_source_test.cc Show resolved Hide resolved
Makefile Show resolved Hide resolved
db/blob/blob_source.cc Outdated Show resolved Hide resolved
db/blob/blob_source.cc Outdated Show resolved Hide resolved
db/blob/blob_source.cc Outdated Show resolved Hide resolved
db/blob/blob_source.cc Outdated Show resolved Hide resolved
Copy link
Contributor

@ltamasi ltamasi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks nice, thanks for the updates @gangliao ! Just some (mostly minor) comments

db/blob/blob_source.cc Outdated Show resolved Hide resolved
db/blob/blob_source.cc Outdated Show resolved Hide resolved
db/blob/blob_source.cc Outdated Show resolved Hide resolved
db/blob/blob_source.cc Outdated Show resolved Hide resolved
db/blob/blob_source.cc Outdated Show resolved Hide resolved
db/blob/blob_source.cc Outdated Show resolved Hide resolved
db/blob/blob_source.h Outdated Show resolved Hide resolved
db/blob/blob_source.h Outdated Show resolved Hide resolved
db/blob/blob_source_test.cc Outdated Show resolved Hide resolved
Copy link
Contributor

@ltamasi ltamasi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks @gangliao !!

db/blob/blob_source.h Outdated Show resolved Hide resolved
@gangliao
Copy link
Contributor Author

Thank you very much for taking the time to write such a thorough review. @ltamasi @riversand963 @pdillinger

@facebook-github-bot
Copy link
Contributor

@gangliao has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@facebook-github-bot
Copy link
Contributor

@gangliao has updated the pull request. You must reimport the pull request before landing.

@facebook-github-bot
Copy link
Contributor

@gangliao has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

facebook-github-bot pushed a commit that referenced this pull request Jul 9, 2022
Summary:
Update HISTORY.md for blob cache.  Implementation can be found from Github issue #10156 (or Github PRs #10155, #10178, #10225, #10198, and #10272).

Pull Request resolved: #10328

Reviewed By: riversand963

Differential Revision: D37732514

Pulled By: gangliao

fbshipit-source-id: 4c942a41c07914bfc8db56a0d3cf4d3e53d5963f
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants