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.

Sign up for GitHub

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

Jump to bottom

Use Velox filesystem for SSD cache data file #11190

Closed
wants to merge 1 commit into from
Closed

Use Velox filesystem for SSD cache data file #11190

wants to merge 1 commit into from

Conversation

zacw7
Copy link
Contributor

@zacw7 zacw7 commented Oct 7, 2024 •
edited
Loading

Summary:
SSD cache currently uses native functions for file operations. This needs to be switched to Velox filesystem, so more advanced testing can be built by leveraging features like fault injections.

Differential Revision: D64008883

@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 Oct 7, 2024
@facebook-github-bot
Copy link
Contributor

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

@netlify Netlify
Copy link

netlify bot commented Oct 7, 2024 •
edited
Loading

✅ Deploy Preview for meta-velox canceled.

Name Link
🔨 Latest commit aa952de
🔍 Latest deploy log https://app.netlify.com/sites/meta-velox/deploys/6722fb4285bc6e0008f5f002

@zacw7 zacw7 force-pushed the export-D64008883 branch 4 times, most recently from 6c197bb to 6091625 Compare October 7, 2024 23:49
@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 force-pushed the export-D64008883 branch from 6091625 to 6b290f6 Compare October 8, 2024 00:57
@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.

xiaoxmeng
xiaoxmeng reviewed Oct 11, 2024
View reviewed changes
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!

velox/common/file/File.cpp Outdated
@@ -367,9 +367,12 @@ void LocalWriteFile::write(
VELOX_CHECK_EQ(
bytesWritten,
length,
"Failure in LocalWriteFile::write, {} vs {}",
"Failure in LocalWriteFile::write, {} vs {}, error: {}, offset: {}, count: {}",
Copy link
Contributor

Choose a reason for hiding this comment

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

s/count/iovec count/

velox/common/file/File.cpp Outdated
bytesWritten,
length);
length,
strerror(errno),
Copy link
Contributor

Choose a reason for hiding this comment

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

use folly version of this? Check the other places.

velox/common/caching/SsdFile.h Outdated
// WriteFile made from 'fd_'.
std::unique_ptr<WriteFile> writeFile_;

// WriteFile for logging evictions.
Copy link
Contributor

Choose a reason for hiding this comment

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

s/evictLogWriteFile_/evictLogFile_/

velox/common/caching/SsdFile.h Outdated
std::unique_ptr<WriteFile> evictLogWriteFile_;

// WriteFile for checkpoint.
std::unique_ptr<WriteFile> checkpointWriteFile_;
Copy link
Contributor

Choose a reason for hiding this comment

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

s/checkpointWriteFile_/checkpointFile_

velox/common/caching/SsdFile.h Show resolved Hide resolved
velox/common/caching/SsdFile.h Outdated

// Synchronously logs that 'regions' are no longer valid in a possibly
// existing checkpoint.
void logEviction(const std::vector<int32_t>& regions);
void logEviction(std::vector<int32_t>& regions);
Copy link
Contributor

Choose a reason for hiding this comment

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

Why change this? Thanks!

velox/common/caching/SsdFile.h Outdated
@@ -489,7 +489,7 @@ class SsdFile {

// Returns true if checkpoint is needed.
bool needCheckpoint(bool force) const {
if (!checkpointEnabled()) {
if (!checkpointEnabled() || checkpointWriteFile_ == nullptr) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why need this check? Thanks!

velox/common/caching/SsdFile.cpp Show resolved Hide resolved
velox/common/caching/SsdFile.cpp Outdated
evictLogFileOptions.shouldThrowOnFileAlreadyExists = false;
try {
evictLogWriteFile_ = fs_->openFileForWrite(logPath, evictLogFileOptions);
} catch (std::exception& e) {
Copy link
Contributor

Choose a reason for hiding this comment

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

We probably can't leverage std::ofstream for this.

velox/common/caching/SsdFile.cpp Outdated
@@ -1002,6 +977,9 @@ void SsdFile::readCheckpoint(std::ifstream& state) {
idMap[id] = StringIdLease(fileIds(), id, name);
}

const auto logPath = getEvictLogFilePath();
auto evictLogFd_ =
Copy link
Contributor

Choose a reason for hiding this comment

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

This hasn't been replaced? Thanks!

@zacw7 zacw7 requested a review from majetideepak as a code owner October 25, 2024 22:14
@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 changed the title Use Velox filesystem in SSD cache Use Velox filesystem for SSD cache data file Oct 26, 2024
@zacw7 zacw7 force-pushed the export-D64008883 branch 12 times, most recently from ad95226 to c239279 Compare October 28, 2024 22:03
@zacw7 zacw7 force-pushed the export-D64008883 branch 5 times, most recently from 8ee8994 to ffa39f0 Compare October 29, 2024 22:53
@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.

@xiaoxmeng xiaoxmeng requested a review from tanjialiang October 30, 2024 04:31
xiaoxmeng
xiaoxmeng reviewed Oct 30, 2024
View reviewed changes
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 LGTM % minors. Thanks!

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
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 Show resolved Hide resolved
@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.

xiaoxmeng
xiaoxmeng approved these changes Oct 30, 2024
View reviewed changes
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 LGTM. Thanks!

@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
Use Velox filesystem for SSD cache data file
aa952de 
SSD cache currently uses native functions for file operations. This
needs to be switched to Velox filesystem, so more advanced testing can
be built by leveraging features like fault injections.
@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 merged this pull request in ee8a683.

@conbench-facebook conbench-facebook
Copy link

Conbench analyzed the 1 benchmark run on commit ee8a6836.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details.

@zacw7 zacw7 deleted the export-D64008883 branch November 1, 2024 02:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@xiaoxmeng xiaoxmeng xiaoxmeng approved these changes

@majetideepak majetideepak Awaiting requested review from majetideepak majetideepak is a code owner

@tanjialiang tanjialiang Awaiting requested review from tanjialiang

Assignees
No one assigned
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 Merged
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@zacw7 @facebook-github-bot @xiaoxmeng