Add support to flush multiple CFs atomically #4262
Conversation
Test plan: ``` $make clean && make -j20 all check ```
There was a problem hiding this comment.
riversand963 has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
|
@riversand963 has updated the pull request. Re-import the pull request |
There was a problem hiding this comment.
riversand963 has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
|
@riversand963 has updated the pull request. Re-import the pull request |
There was a problem hiding this comment.
riversand963 has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
db/db_impl_compaction_flush.cc
Outdated
| #endif /* !ROCKSDB_LITE */ | ||
| if (logfile_number_ > 0 && | ||
| versions_->GetColumnFamilySet()->NumberOfColumnFamilies() > 0) { | ||
| s = SyncClosedLogs(job_context); |
There was a problem hiding this comment.
I'm lost here. Are we still holding DB mutex here?
There was a problem hiding this comment.
IMO it looks fine since the calls doing I/O (like SyncClosedLogs) look like they all release/reacquire the lock.
|
@riversand963 has updated the pull request. Re-import the pull request |
There was a problem hiding this comment.
riversand963 has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
|
@riversand963 has updated the pull request. Re-import the pull request |
There was a problem hiding this comment.
riversand963 has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
|
@riversand963 has updated the pull request. Re-import the pull request |
|
Thanks @ajkr for the review comments! |
|
@riversand963 has updated the pull request. Re-import the pull request |
There was a problem hiding this comment.
riversand963 has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
|
@riversand963 has updated the pull request. Re-import the pull request |
|
@riversand963 has updated the pull request. Re-import the pull request |
|
@riversand963 has updated the pull request. Re-import the pull request |
|
@riversand963 has updated the pull request. Re-import the pull request |
There was a problem hiding this comment.
riversand963 has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
|
@riversand963 has updated the pull request. Re-import the pull request |
There was a problem hiding this comment.
riversand963 has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
|
@riversand963 has updated the pull request. Re-import the pull request |
There was a problem hiding this comment.
riversand963 has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
|
@riversand963 has updated the pull request. Re-import the pull request |
There was a problem hiding this comment.
riversand963 has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
|
@riversand963 has updated the pull request. Re-import the pull request |
There was a problem hiding this comment.
riversand963 has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
Leverage existing
FlushJobto implement atomic flush of multiple column families.This PR depends on other PRs and is a subset of #3752 . This PR itself is not sufficient in fulfilling atomic flush.
Test plan: