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

Deflake DBErrorHandlingFSTest.MultiCFWALWriteError #9496

Closed
wants to merge 3 commits into from
Closed

Deflake DBErrorHandlingFSTest.MultiCFWALWriteError #9496

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
c4ee960
clear bg error later
hx235 Feb 3, 2022
ea4cc03
Address feedback: hold lock longer, more comment, add to HISTORY
hx235 Feb 24, 2022
b1b9977
Address feedback: history, error logging
hx235 Feb 25, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions HISTORY.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,9 @@
### New Features
* Allow WriteBatchWithIndex to index a WriteBatch that includes keys with user-defined timestamps. The index itself does not have timestamp.

### Bug Fixes
* * Fixed a data race on `versions_` between `DBImpl::ResumeImpl()` and threads waiting for recovery to complete (#9496)

## 7.0.0 (02/20/2022)
### Bug Fixes
* Fixed a major bug in which batched MultiGet could return old values for keys deleted by DeleteRange when memtable Bloom filter is enabled (memtable_prefix_bloom_size_ratio > 0). (The fix includes a substantial MultiGet performance improvement in the unusual case of both memtable_whole_key_filtering and prefix_extractor.)
Expand Down
30 changes: 21 additions & 9 deletions db/db_impl/db_impl.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -400,14 +400,6 @@ Status DBImpl::ResumeImpl(DBRecoverContext context) {

JobContext job_context(0);
FindObsoleteFiles(&job_context, true);
if (s.ok()) {
s = error_handler_.ClearBGError();
} else {
// NOTE: this is needed to pass ASSERT_STATUS_CHECKED
// in the DBSSTTest.DBWithMaxSpaceAllowedRandomized test.
// See https://github.com/facebook/rocksdb/pull/7715#issuecomment-754947952
error_handler_.GetRecoveryError().PermitUncheckedError();
}
mutex_.Unlock();

job_context.manifest_file_number = 1;
Expand All @@ -428,11 +420,31 @@ Status DBImpl::ResumeImpl(DBRecoverContext context) {
immutable_db_options_.info_log,
"DB resume requested but could not enable file deletions [%s]",
s.ToString().c_str());
assert(false);
}
}
ROCKS_LOG_INFO(immutable_db_options_.info_log, "Successfully resumed DB");
}

mutex_.Lock();
if (s.ok()) {
// This will notify and unblock threads waiting for error recovery to
// finish. Those previouly waiting threads can now proceed, which may
// include closing the db.
s = error_handler_.ClearBGError();
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe add some comment here about how the db may be closed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

} else {
// NOTE: this is needed to pass ASSERT_STATUS_CHECKED
// in the DBSSTTest.DBWithMaxSpaceAllowedRandomized test.
// See https://github.com/facebook/rocksdb/pull/7715#issuecomment-754947952
error_handler_.GetRecoveryError().PermitUncheckedError();
}

if (s.ok()) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

To make ROCKS_LOG_INFO(immutable_db_options_.info_log, "Successfully resumed DB"); more clear, I separated it into its own if-block.

Copy link
Contributor

Choose a reason for hiding this comment

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

You're right that after calling ClearBGError(), nothing is guaranteed. That makes this block problematic since the mutex is unlocked and locked again. In the meantime, the DB may be deleted and lines 445-456 may access stale memory. We can continue to hold the mutex here to avoid that possibility.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got you - the unlock/lock again behavior came from the original PR 52d4c9b#diff-6fdb755f590d9b01ecb89bd8ceb28577e85536d4472f8e4fc3addeb9a65f3645R269 but I will change it. 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.

Fixed

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, maybe it's better to log the status anyway even if it's non-ok.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

ROCKS_LOG_INFO(immutable_db_options_.info_log, "Successfully resumed DB");
} else {
ROCKS_LOG_INFO(immutable_db_options_.info_log, "Failed to resume DB [%s]",
s.ToString().c_str());
}

// Check for shutdown again before scheduling further compactions,
// since we released and re-acquired the lock above
if (shutdown_initiated_) {
Expand Down