Skip to content

Commit

Permalink
Don't run background jobs (flush, compactions) when bg_error_ is set
Browse files Browse the repository at this point in the history
Summary:
If bg_error_ is set, that means that we mark DB read only. However, current behavior still continues the flushes and compactions, even though bg_error_ is set.

On the other hand, if bg_error_ is set, we will return Status::OK() from CompactRange(), although the compaction didn't actually succeed.

This is clearly not desired behavior. I found this when I was debugging t5132159, although I'm pretty sure these aren't related.

Also, when we're shutting down, it's dangerous to exit RunManualCompaction(), since that will destruct ManualCompaction object. Background compaction job might still hold a reference to manual_compaction_ and this will lead to undefined behavior. I changed the behavior so that we only exit RunManualCompaction when manual compaction job is marked done.

Test Plan: make check

Reviewers: sdong, ljin, yhchiang

Reviewed By: yhchiang

Subscribers: leveldb

Differential Revision: https://reviews.facebook.net/D23223
  • Loading branch information
igorcanadi committed Sep 11, 2014
1 parent a9639bd commit 9c0e66c
Showing 1 changed file with 19 additions and 1 deletion.
20 changes: 19 additions & 1 deletion db/db_impl.cc
Expand Up @@ -1891,7 +1891,10 @@ Status DBImpl::RunManualCompaction(ColumnFamilyData* cfd, int input_level,
Log(db_options_.info_log, "[%s] Manual compaction starting",
cfd->GetName().c_str());

while (!manual.done && !shutting_down_.Acquire_Load() && bg_error_.ok()) {
// We don't check bg_error_ here, because if we get the error in compaction,
// the compaction will set manual.status to bg_error_ and set manual.done to
// true.
while (!manual.done) {
assert(bg_manual_only_ > 0);
if (manual_compaction_ != nullptr) {
// Running either this or some other manual compaction
Expand Down Expand Up @@ -2041,6 +2044,11 @@ Status DBImpl::BackgroundFlush(bool* madeProgress,
DeletionState& deletion_state,
LogBuffer* log_buffer) {
mutex_.AssertHeld();

if (!bg_error_.ok()) {
return bg_error_;
}

// call_status is failure if at least one flush was a failure. even if
// flushing one column family reports a failure, we will continue flushing
// other column families. however, call_status will be a failure in that case.
Expand Down Expand Up @@ -2228,6 +2236,16 @@ Status DBImpl::BackgroundCompaction(bool* madeProgress,
bool is_manual = (manual_compaction_ != nullptr) &&
(manual_compaction_->in_progress == false);

if (!bg_error_.ok()) {
if (is_manual) {
manual_compaction_->status = bg_error_;
manual_compaction_->done = true;
manual_compaction_->in_progress = false;
manual_compaction_ = nullptr;
}
return bg_error_;
}

if (is_manual) {
// another thread cannot pick up the same work
manual_compaction_->in_progress = true;
Expand Down

0 comments on commit 9c0e66c

Please sign in to comment.