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

Use Velox fs for ssd cache evictlog file #11495

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

zacw7
Copy link
Contributor

@zacw7 zacw7 commented Nov 11, 2024

Switch the eviction log file to use Velox filesystem for file r/w operations, so that more advanced testing can be built by leveraging features like fault injections.

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Nov 11, 2024
Copy link

netlify bot commented Nov 11, 2024

Deploy Preview for meta-velox canceled.

Name Link
🔨 Latest commit 7a60927
🔍 Latest deploy log https://app.netlify.com/sites/meta-velox/deploys/6735a249fb42600008bf0e7f

@facebook-github-bot
Copy link
Contributor

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

@facebook-github-bot
Copy link
Contributor

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

@zacw7 zacw7 marked this pull request as ready for review November 11, 2024 07:34
Copy link
Contributor

@xiaoxmeng xiaoxmeng left a comment

Choose a reason for hiding this comment

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

Can you briefly comment in PR description? Thanks!

@zacw7 zacw7 requested a review from xiaoxmeng November 11, 2024 17:40
Copy link
Contributor

@xiaoxmeng xiaoxmeng left a comment

Choose a reason for hiding this comment

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

@zacw7 thanks for the change % comments!

@@ -66,28 +69,6 @@ void disableCow(int32_t fd) {
#endif // linux
}

// TODO: Remove this function once we migrate all files to velox fs.
Copy link
Contributor

Choose a reason for hiding this comment

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

We still have checkpoint file write though posix api? thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes

velox/common/caching/SsdFile.cpp Outdated Show resolved Hide resolved
velox/common/caching/SsdFile.cpp Outdated Show resolved Hide resolved
velox/common/caching/SsdFile.cpp Outdated Show resolved Hide resolved
velox/common/caching/SsdFile.cpp Outdated Show resolved Hide resolved
if (evictLogFd_ < 0) {
filesystems::FileOptions evictLogFileOptions;
evictLogFileOptions.shouldThrowOnFileAlreadyExists = false;
try {
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider to provide a macro in followup

#define FILE_CALL(fileFunction, errorCallback);

errorCallback expects a std::exception and both fileFunction and errorCallback could be a lambda function to capture requires execution context.

@@ -965,6 +948,9 @@ void SsdFile::disableFileCow() {
const std::unordered_map<std::string, std::string> attributes = {
{std::string(LocalWriteFile::Attributes::kNoCow), "true"}};
writeFile_->setAttributes(attributes);
if (evictLogWriteFile_ != nullptr) {
evictLogWriteFile_->setAttributes(attributes);
Copy link
Contributor

Choose a reason for hiding this comment

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

We don't disable checkpoint file?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is disabled in a separate path as we haven't switched checkpoint files to velox fs.

try {
evictLogReadFile->pread(0, logSize, evicted.data());
} catch (const std::exception& e) {
++stats_.deleteCheckpointErrors;
Copy link
Contributor

Choose a reason for hiding this comment

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

We shall throw by VELOX_FAIL here? Can you use file fault injection to trigger the log eviction read failure?

@zacw7 zacw7 force-pushed the fs-evictlog branch 2 times, most recently from 2181e3b to 69db055 Compare November 14, 2024 00:44
@facebook-github-bot
Copy link
Contributor

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

zacw7 added a commit to zacw7/velox that referenced this pull request Nov 14, 2024
Summary:
Switch the eviction log file to use Velox filesystem for file r/w operations, so that more advanced testing can be built by leveraging features like fault injections.


Differential Revision: D65740612

Pulled By: zacw7
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D65740612

Summary:
Switch the eviction log file to use Velox filesystem for file r/w operations, so that more advanced testing can be built by leveraging features like fault injections.


Differential Revision: D65740612

Pulled By: zacw7
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D65740612

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. fb-exported
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants