From d89483098ff4a77bd0ef12454a64554805984cc5 Mon Sep 17 00:00:00 2001 From: Justin Chapman Date: Wed, 14 Apr 2021 12:03:56 -0700 Subject: [PATCH] Assert unlimited max_open_files for FIFO compaction. (#8172) Summary: Resolves https://github.com/facebook/rocksdb/issues/8014 - Add an assertion on `DB::Open` to ensure `db_options.max_open_files` is unlimited if FIFO Compaction is being used. - This is to align with what the docs mention and to prevent premature data deletion. - Update tests to work with this assertion. Pull Request resolved: https://github.com/facebook/rocksdb/pull/8172 Test Plan: ```bash $ make check -j$(nproc) Generated TARGETS Summary: - 6 libs - 0 binarys - 180 tests ``` Reviewed By: ajkr Differential Revision: D27768792 Pulled By: thejchap fbshipit-source-id: cf6350535e3a3577fec72bcba75b3c094dc7a6f3 --- HISTORY.md | 1 + db/column_family.cc | 6 +++++ db/db_bloom_filter_test.cc | 2 ++ db/db_properties_test.cc | 1 + db/db_test.cc | 12 ++++++--- db/db_test_util.cc | 1 + .../option_change_migration_test.cc | 26 +++++++++++++++++-- 7 files changed, 43 insertions(+), 6 deletions(-) diff --git a/HISTORY.md b/HISTORY.md index 02b1c4af9..6ab13e5a3 100644 --- a/HISTORY.md +++ b/HISTORY.md @@ -12,6 +12,7 @@ * Made BackupEngine thread-safe and added documentation comments to clarify what is safe for multiple BackupEngine objects accessing the same backup directory. * Fixed crash (divide by zero) when compression dictionary is applied to a file containing only range tombstones. * Fixed a backward iteration bug with partitioned filter enabled: not including the prefix of the last key of the previous filter partition in current filter partition can cause wrong iteration result. +* Fixed a bug that allowed `DBOptions::max_open_files` to be set with a non-negative integer with `ColumnFamilyOptions::compaction_style = kCompactionStyleFIFO`. ### Performance Improvements * On ARM platform, use `yield` instead of `wfe` to relax cpu to gain better performance. diff --git a/db/column_family.cc b/db/column_family.cc index 80c3c9817..d97a5d700 100644 --- a/db/column_family.cc +++ b/db/column_family.cc @@ -1355,6 +1355,12 @@ Status ColumnFamilyData::ValidateOptions( "[0.0, 1.0]."); } + if (cf_options.compaction_style == kCompactionStyleFIFO && + db_options.max_open_files != -1 && cf_options.ttl > 0) { + return Status::NotSupported( + "FIFO compaction only supported with max_open_files = -1."); + } + return s; } diff --git a/db/db_bloom_filter_test.cc b/db/db_bloom_filter_test.cc index 30fc11eba..18b34f0c5 100644 --- a/db/db_bloom_filter_test.cc +++ b/db/db_bloom_filter_test.cc @@ -810,6 +810,7 @@ class TestingContextCustomFilterPolicy TEST_F(DBBloomFilterTest, ContextCustomFilterPolicy) { for (bool fifo : {true, false}) { Options options = CurrentOptions(); + options.max_open_files = fifo ? -1 : options.max_open_files; options.statistics = ROCKSDB_NAMESPACE::CreateDBStatistics(); options.compaction_style = fifo ? kCompactionStyleFIFO : kCompactionStyleLevel; @@ -820,6 +821,7 @@ TEST_F(DBBloomFilterTest, ContextCustomFilterPolicy) { table_options.format_version = 5; options.table_factory.reset(NewBlockBasedTableFactory(table_options)); + TryReopen(options); CreateAndReopenWithCF({fifo ? "abe" : "bob"}, options); const int maxKey = 10000; diff --git a/db/db_properties_test.cc b/db/db_properties_test.cc index 69c2f6b1b..8945ee291 100644 --- a/db/db_properties_test.cc +++ b/db/db_properties_test.cc @@ -1626,6 +1626,7 @@ TEST_F(DBPropertiesTest, EstimateOldestKeyTime) { options.compaction_style = kCompactionStyleFIFO; options.ttl = 300; + options.max_open_files = -1; options.compaction_options_fifo.allow_compaction = false; DestroyAndReopen(options); diff --git a/db/db_test.cc b/db/db_test.cc index cf2590f8a..4e1b660f4 100644 --- a/db/db_test.cc +++ b/db/db_test.cc @@ -3507,17 +3507,21 @@ TEST_F(DBTest, FIFOCompactionStyleWithCompactionAndDelete) { } // Check that FIFO-with-TTL is not supported with max_open_files != -1. +// Github issue #8014 TEST_F(DBTest, FIFOCompactionWithTTLAndMaxOpenFilesTest) { - Options options; + Options options = CurrentOptions(); options.compaction_style = kCompactionStyleFIFO; options.create_if_missing = true; options.ttl = 600; // seconds - // TTL is now supported with max_open_files != -1. + // TTL is not supported with max_open_files != -1. + options.max_open_files = 0; + ASSERT_TRUE(TryReopen(options).IsNotSupported()); + options.max_open_files = 100; - options = CurrentOptions(options); - ASSERT_OK(TryReopen(options)); + ASSERT_TRUE(TryReopen(options).IsNotSupported()); + // TTL is supported with unlimited max_open_files options.max_open_files = -1; ASSERT_OK(TryReopen(options)); } diff --git a/db/db_test_util.cc b/db/db_test_util.cc index bdc0568ee..4dadcff56 100644 --- a/db/db_test_util.cc +++ b/db/db_test_util.cc @@ -487,6 +487,7 @@ Options DBTestBase::GetOptions( } case kFIFOCompaction: { options.compaction_style = kCompactionStyleFIFO; + options.max_open_files = -1; break; } case kBlockBasedTableWithPrefixHashIndex: { diff --git a/utilities/option_change_migration/option_change_migration_test.cc b/utilities/option_change_migration/option_change_migration_test.cc index c457d45cd..126bebea1 100644 --- a/utilities/option_change_migration/option_change_migration_test.cc +++ b/utilities/option_change_migration/option_change_migration_test.cc @@ -54,7 +54,9 @@ TEST_P(DBOptionChangeMigrationTests, Migrate1) { if (old_options.compaction_style == CompactionStyle::kCompactionStyleLevel) { old_options.level_compaction_dynamic_level_bytes = is_dynamic1_; } - + if (old_options.compaction_style == CompactionStyle::kCompactionStyleFIFO) { + old_options.max_open_files = -1; + } old_options.level0_file_num_compaction_trigger = 3; old_options.write_buffer_size = 64 * 1024; old_options.target_file_size_base = 128 * 1024; @@ -92,6 +94,9 @@ TEST_P(DBOptionChangeMigrationTests, Migrate1) { if (new_options.compaction_style == CompactionStyle::kCompactionStyleLevel) { new_options.level_compaction_dynamic_level_bytes = is_dynamic2_; } + if (new_options.compaction_style == CompactionStyle::kCompactionStyleFIFO) { + new_options.max_open_files = -1; + } new_options.target_file_size_base = 256 * 1024; new_options.num_levels = level2_; new_options.max_bytes_for_level_base = 150 * 1024; @@ -123,6 +128,9 @@ TEST_P(DBOptionChangeMigrationTests, Migrate2) { if (old_options.compaction_style == CompactionStyle::kCompactionStyleLevel) { old_options.level_compaction_dynamic_level_bytes = is_dynamic2_; } + if (old_options.compaction_style == CompactionStyle::kCompactionStyleFIFO) { + old_options.max_open_files = -1; + } old_options.level0_file_num_compaction_trigger = 3; old_options.write_buffer_size = 64 * 1024; old_options.target_file_size_base = 128 * 1024; @@ -161,6 +169,9 @@ TEST_P(DBOptionChangeMigrationTests, Migrate2) { if (new_options.compaction_style == CompactionStyle::kCompactionStyleLevel) { new_options.level_compaction_dynamic_level_bytes = is_dynamic1_; } + if (new_options.compaction_style == CompactionStyle::kCompactionStyleFIFO) { + new_options.max_open_files = -1; + } new_options.target_file_size_base = 256 * 1024; new_options.num_levels = level1_; new_options.max_bytes_for_level_base = 150 * 1024; @@ -191,7 +202,9 @@ TEST_P(DBOptionChangeMigrationTests, Migrate3) { if (old_options.compaction_style == CompactionStyle::kCompactionStyleLevel) { old_options.level_compaction_dynamic_level_bytes = is_dynamic1_; } - + if (old_options.compaction_style == CompactionStyle::kCompactionStyleFIFO) { + old_options.max_open_files = -1; + } old_options.level0_file_num_compaction_trigger = 3; old_options.write_buffer_size = 64 * 1024; old_options.target_file_size_base = 128 * 1024; @@ -235,6 +248,9 @@ TEST_P(DBOptionChangeMigrationTests, Migrate3) { if (new_options.compaction_style == CompactionStyle::kCompactionStyleLevel) { new_options.level_compaction_dynamic_level_bytes = is_dynamic2_; } + if (new_options.compaction_style == CompactionStyle::kCompactionStyleFIFO) { + new_options.max_open_files = -1; + } new_options.target_file_size_base = 256 * 1024; new_options.num_levels = level2_; new_options.max_bytes_for_level_base = 150 * 1024; @@ -266,6 +282,9 @@ TEST_P(DBOptionChangeMigrationTests, Migrate4) { if (old_options.compaction_style == CompactionStyle::kCompactionStyleLevel) { old_options.level_compaction_dynamic_level_bytes = is_dynamic2_; } + if (old_options.compaction_style == CompactionStyle::kCompactionStyleFIFO) { + old_options.max_open_files = -1; + } old_options.level0_file_num_compaction_trigger = 3; old_options.write_buffer_size = 64 * 1024; old_options.target_file_size_base = 128 * 1024; @@ -310,6 +329,9 @@ TEST_P(DBOptionChangeMigrationTests, Migrate4) { if (new_options.compaction_style == CompactionStyle::kCompactionStyleLevel) { new_options.level_compaction_dynamic_level_bytes = is_dynamic1_; } + if (new_options.compaction_style == CompactionStyle::kCompactionStyleFIFO) { + new_options.max_open_files = -1; + } new_options.target_file_size_base = 256 * 1024; new_options.num_levels = level1_; new_options.max_bytes_for_level_base = 150 * 1024;