Assert unlimited max_open_files for FIFO compaction. (facebook#8172)

Summary:
Resolves facebook#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: facebook#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

@@ -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.

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;
 }
 

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;

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);
 

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));
 }

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: {

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;