From 17b8f786a332883ff3d48022050ac93c9e40276b Mon Sep 17 00:00:00 2001 From: Xing Jin Date: Fri, 2 Aug 2013 11:46:47 -0700 Subject: [PATCH] Fix unit tests/bugs for universal compaction (first step) Summary: This is the first step to fix unit tests and bugs for universal compactiion. I added universal compaction option to ChangeOptions(), and fixed all unit tests calling ChangeOptions(). Some of these tests obviously assume more than 1 level and check file number/values in level 1 or above levels. I set kSkipUniversalCompaction for these tests. The major bug I found is manual compaction with universal compaction never stops. I have put a fix for it. I have also set universal compaction as the default compaction and found at least 20+ unit tests failing. I haven't looked into the details. The next step is to check all unit tests without calling ChangeOptions(). Test Plan: make all check Reviewers: dhruba, haobo Differential Revision: https://reviews.facebook.net/D12051 --- db/db_impl.cc | 14 +++++++++++++- db/db_test.cc | 40 +++++++++++++++++++++++++++++++--------- db/version_set.cc | 2 +- util/options.cc | 10 +++++----- 4 files changed, 50 insertions(+), 16 deletions(-) diff --git a/db/db_impl.cc b/db/db_impl.cc index dd942f36b..1d568d0b1 100644 --- a/db/db_impl.cc +++ b/db/db_impl.cc @@ -1498,6 +1498,18 @@ Status DBImpl::BackgroundCompaction(bool* madeProgress, if (!status.ok()) { m->done = true; } + // For universal compaction: + // Because universal compaction always happens at level 0, so one + // compaction will pick up all overlapped files. No files will be + // filtered out due to size limit and left for a successive compaction. + // So we can safely conclude the current compaction. + // + // Also note that, if we don't stop here, then the current compaction + // writes a new file back to level 0, which will be used in successive + // compaction. Hence the manual compaction will never finish. + if (options_.compaction_style == kCompactionStyleUniversal) { + m->done = true; + } if (!m->done) { // We only compacted part of the requested range. Update *m // to the range that is left to be compacted. @@ -2088,7 +2100,7 @@ Status DBImpl::DoCompactionWork(CompactionState* compact) { versions_->LevelSummary(&tmp), (stats.bytes_readn + stats.bytes_readnp1 + stats.bytes_written) / (double) stats.micros, - compact->compaction->level() + 1, + compact->compaction->output_level(), stats.files_in_leveln, stats.files_in_levelnp1, stats.files_out_levelnp1, stats.bytes_readn / 1048576.0, stats.bytes_readnp1 / 1048576.0, diff --git a/db/db_test.cc b/db/db_test.cc index b861a4cc4..01ff1e046 100644 --- a/db/db_test.cc +++ b/db/db_test.cc @@ -219,6 +219,7 @@ class DBTest { kCompactOnFlush, kPerfOptions, kDeletesFilterFirst, + kUniversalCompaction, kEnd }; int option_config_; @@ -232,6 +233,14 @@ class DBTest { Options last_options_; + // Skip some options, as they may not be applicable to a specific test. + // To add more skip constants, use values 4, 8, 16, etc. + enum OptionSkip { + kNoSkip = 0, + kSkipDeletesFilterFirst = 1, + kSkipUniversalCompaction = 2 + }; + DBTest() : option_config_(kDefault), merge_operator_(MergeOperators::CreatePutOperator()), env_(new SpecialEnv(Env::Default())) { @@ -251,8 +260,19 @@ class DBTest { // Switch to a fresh database with the next option configuration to // test. Return false if there are no more configurations to test. - bool ChangeOptions() { + bool ChangeOptions(int skip_mask = kNoSkip) { option_config_++; + + // skip some options + if (skip_mask & kSkipDeletesFilterFirst && + option_config_ == kDeletesFilterFirst) { + option_config_++; + } + if (skip_mask & kSkipUniversalCompaction && + option_config_ == kUniversalCompaction) { + option_config_++; + } + if (option_config_ >= kEnd) { return false; } else { @@ -293,6 +313,9 @@ class DBTest { case kDeletesFilterFirst: options.filter_deletes = true; break; + case kUniversalCompaction: + options.compaction_style = kCompactionStyleUniversal; + break; default: break; } @@ -769,7 +792,7 @@ TEST(DBTest, GetEncountersEmptyLevel) { env_->SleepForMicroseconds(1000000); ASSERT_EQ(NumTableFilesAtLevel(0), 1); // XXX - } while (ChangeOptions()); + } while (ChangeOptions(kSkipUniversalCompaction)); } // KeyMayExist can lead to a few false positives, but not false negatives. @@ -1791,7 +1814,7 @@ TEST(DBTest, ApproximateSizes) { ASSERT_EQ(NumTableFilesAtLevel(0), 0); ASSERT_GT(NumTableFilesAtLevel(1), 0); } - } while (ChangeOptions()); + } while (ChangeOptions(kSkipUniversalCompaction)); } TEST(DBTest, ApproximateSizes_MixOfSmallAndLarge) { @@ -1911,7 +1934,7 @@ TEST(DBTest, HiddenValuesAreRemoved) { ASSERT_EQ(AllEntriesFor("foo"), "[ tiny ]"); ASSERT_TRUE(Between(Size("", "pastfoo"), 0, 1000)); - } while (ChangeOptions()); + } while (ChangeOptions(kSkipUniversalCompaction)); } TEST(DBTest, CompactBetweenSnapshots) { @@ -2065,7 +2088,7 @@ TEST(DBTest, OverlapInLevel0) { dbfull()->TEST_CompactMemTable(); ASSERT_EQ("3", FilesPerLevel()); ASSERT_EQ("NOT_FOUND", Get("600")); - } while (ChangeOptions()); + } while (ChangeOptions(kSkipUniversalCompaction)); } TEST(DBTest, L0_CompactionBug_Issue44_a) { @@ -3243,9 +3266,6 @@ static bool CompareIterators(int step, TEST(DBTest, Randomized) { Random rnd(test::RandomSeed()); do { - if (CurrentOptions().filter_deletes) { - ChangeOptions(); // DBTest.Randomized not suited for filter_deletes - } ModelDB model(CurrentOptions()); const int N = 10000; const Snapshot* model_snap = nullptr; @@ -3308,7 +3328,9 @@ TEST(DBTest, Randomized) { } if (model_snap != nullptr) model.ReleaseSnapshot(model_snap); if (db_snap != nullptr) db_->ReleaseSnapshot(db_snap); - } while (ChangeOptions()); + // TODO (xjin): remove kSkipUniversalCompaction after bug in + // IsBaseLevelForKey() is fixed. + } while (ChangeOptions(kSkipDeletesFilterFirst | kSkipUniversalCompaction)); } TEST(DBTest, MultiGetSimple) { diff --git a/db/version_set.cc b/db/version_set.cc index 1b5001d59..79e868ecb 100644 --- a/db/version_set.cc +++ b/db/version_set.cc @@ -2403,7 +2403,7 @@ Compaction* VersionSet::PickCompaction() { if (level != 0 || compactions_in_progress_[0].empty()) { if(!ParentRangeInCompaction(&f->smallest, &f->largest, level, &parent_index)) { - c = new Compaction(level, level, MaxFileSizeForLevel(level+1), + c = new Compaction(level, level+1, MaxFileSizeForLevel(level+1), MaxGrandParentOverlapBytes(level), NumberLevels(), true); c->inputs_[0].push_back(f); c->parent_index_ = parent_index; diff --git a/util/options.cc b/util/options.cc index 800c8d321..127fe8181 100644 --- a/util/options.cc +++ b/util/options.cc @@ -185,7 +185,7 @@ Options::Dump(Logger* log) const max_background_compactions); Log(log," Options.hard_rate_limit: %.2f", hard_rate_limit); - Log(log," Options.rate_limit_delay_max_milliseconds: %d", + Log(log," Options.rate_limit_delay_max_milliseconds: %u", rate_limit_delay_max_milliseconds); Log(log," Options.disable_auto_compactions: %d", disable_auto_compactions); @@ -205,7 +205,7 @@ Options::Dump(Logger* log) const is_fd_close_on_exec); Log(log," Options.skip_log_error_on_recovery: %d", skip_log_error_on_recovery); - Log(log," Options.stats_dump_period_sec: %d", + Log(log," Options.stats_dump_period_sec: %u", stats_dump_period_sec); Log(log," Options.block_size_deviation: %d", block_size_deviation); @@ -221,11 +221,11 @@ Options::Dump(Logger* log) const filter_deletes); Log(log," Options.compaction_style: %d", compaction_style); - Log(log," Options.compaction_options_universal.size_ratio: %d", + Log(log," Options.compaction_options_universal.size_ratio: %u", compaction_options_universal.size_ratio); - Log(log," Options.compaction_options_universal.min_merge_width: %d", + Log(log," Options.compaction_options_universal.min_merge_width: %u", compaction_options_universal.min_merge_width); - Log(log," Options.compaction_options_universal.max_merge_width: %d", + Log(log," Options.compaction_options_universal.max_merge_width: %u", compaction_options_universal.max_merge_width); } // Options::Dump