diff --git a/HISTORY.md b/HISTORY.md index 1cfa1af77..791ceb98c 100644 --- a/HISTORY.md +++ b/HISTORY.md @@ -1,5 +1,7 @@ # Rocksdb Change Log ## Unreleased +### Bug fixes +* Fixed a bug after a `CompactRange()` with `CompactRangeOptions::change_level` set fails due to a conflict in the level change step, which caused all subsequent calls to `CompactRange()` with `CompactRangeOptions::change_level` set to incorrectly fail with a `Status::NotSupported("another thread is refitting")` error. ### Public API Change * The methods to create and manage EncrypedEnv have been changed. The EncryptionProvider is now passed to NewEncryptedEnv as a shared pointer, rather than a raw pointer. Comparably, the CTREncryptedProvider now takes a shared pointer, rather than a reference, to a BlockCipher. CreateFromString methods have been added to BlockCipher and EncryptionProvider to provide a single API by which different ciphers and providers can be created, respectively. * The internal classes (CTREncryptionProvider, ROT13BlockCipher, CTRCipherStream) associated with the EncryptedEnv have been moved out of the public API. To create a CTREncryptionProvider, one can either use EncryptionProvider::NewCTRProvider, or EncryptionProvider::CreateFromString("CTR"). To create a new ROT13BlockCipher, one can either use BlockCipher::NewROT13Cipher or BlockCipher::CreateFromString("ROT13"). diff --git a/db/db_compaction_test.cc b/db/db_compaction_test.cc index 47378d2b1..5b14534ae 100644 --- a/db/db_compaction_test.cc +++ b/db/db_compaction_test.cc @@ -5655,7 +5655,6 @@ TEST_F(DBCompactionTest, ChangeLevelCompactRangeConflictsWithManual) { GenerateNewFile(&rnd, &key_idx); GenerateNewFile(&rnd, &key_idx); - ASSERT_OK(dbfull()->TEST_WaitForCompact()); ASSERT_EQ("1,1,2", FilesPerLevel(0)); // The background thread will refit L2->L1 while the @@ -5719,6 +5718,81 @@ TEST_F(DBCompactionTest, ChangeLevelCompactRangeConflictsWithManual) { refit_level_thread.join(); } +TEST_F(DBCompactionTest, ChangeLevelErrorPathTest) { + // This test is added to ensure that RefitLevel() error paths are clearing + // internal flags and to test that subsequent valid RefitLevel() calls + // succeeds + Options options = CurrentOptions(); + options.memtable_factory.reset( + new SpecialSkipListFactory(KNumKeysByGenerateNewFile - 1)); + options.level0_file_num_compaction_trigger = 2; + options.num_levels = 3; + Reopen(options); + + ASSERT_EQ("", FilesPerLevel(0)); + + // Setup an LSM with three levels populated. + Random rnd(301); + int key_idx = 0; + GenerateNewFile(&rnd, &key_idx); + ASSERT_EQ("1", FilesPerLevel(0)); + { + CompactRangeOptions cro; + cro.change_level = true; + cro.target_level = 2; + ASSERT_OK(dbfull()->CompactRange(cro, nullptr, nullptr)); + } + ASSERT_EQ("0,0,2", FilesPerLevel(0)); + + auto start_idx = key_idx; + GenerateNewFile(&rnd, &key_idx); + GenerateNewFile(&rnd, &key_idx); + auto end_idx = key_idx - 1; + ASSERT_EQ("1,1,2", FilesPerLevel(0)); + + // Next two CompactRange() calls are used to test exercise error paths within + // RefitLevel() before triggering a valid RefitLevel() call + + // Trigger a refit to L1 first + { + std::string begin_string = Key(start_idx); + std::string end_string = Key(end_idx); + Slice begin(begin_string); + Slice end(end_string); + + CompactRangeOptions cro; + cro.change_level = true; + cro.target_level = 1; + ASSERT_OK(dbfull()->CompactRange(cro, &begin, &end)); + } + ASSERT_EQ("0,3,2", FilesPerLevel(0)); + + // Try a refit from L2->L1 - this should fail and exercise error paths in + // RefitLevel() + { + // Select key range that matches the bottom most level (L2) + std::string begin_string = Key(0); + std::string end_string = Key(start_idx - 1); + Slice begin(begin_string); + Slice end(end_string); + + CompactRangeOptions cro; + cro.change_level = true; + cro.target_level = 1; + ASSERT_NOK(dbfull()->CompactRange(cro, &begin, &end)); + } + ASSERT_EQ("0,3,2", FilesPerLevel(0)); + + // Try a valid Refit request to ensure, the path is still working + { + CompactRangeOptions cro; + cro.change_level = true; + cro.target_level = 1; + ASSERT_OK(dbfull()->CompactRange(cro, nullptr, nullptr)); + } + ASSERT_EQ("0,5", FilesPerLevel(0)); +} + #endif // !defined(ROCKSDB_LITE) } // namespace ROCKSDB_NAMESPACE diff --git a/db/db_impl/db_impl_compaction_flush.cc b/db/db_impl/db_impl_compaction_flush.cc index e6cfe80c9..9115289d4 100644 --- a/db/db_impl/db_impl_compaction_flush.cc +++ b/db/db_impl/db_impl_compaction_flush.cc @@ -1348,12 +1348,14 @@ Status DBImpl::ReFitLevel(ColumnFamilyData* cfd, int level, int target_level) { if (to_level != level) { if (to_level > level) { if (level == 0) { + refitting_level_ = false; return Status::NotSupported( "Cannot change from level 0 to other levels."); } // Check levels are empty for a trivial move for (int l = level + 1; l <= to_level; l++) { if (vstorage->NumLevelFiles(l) > 0) { + refitting_level_ = false; return Status::NotSupported( "Levels between source and target are not empty for a move."); } @@ -1363,6 +1365,7 @@ Status DBImpl::ReFitLevel(ColumnFamilyData* cfd, int level, int target_level) { // Check levels are empty for a trivial move for (int l = to_level; l < level; l++) { if (vstorage->NumLevelFiles(l) > 0) { + refitting_level_ = false; return Status::NotSupported( "Levels between source and target are not empty for a move."); }