Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Assert unlimited max_open_files for FIFO compaction. #8172

Closed
wants to merge 1 commit into from
Closed

Assert unlimited max_open_files for FIFO compaction. #8172

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions HISTORY.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
6 changes: 6 additions & 0 deletions db/column_family.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Comment on lines +1358 to +1359
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you explain the ttl > 0? If ttl == 0, is it okay if max_open_files != -1 ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As far as we know, the max_open_files == -1 is only needed to ensure the files' creation_time properties are read, which are used for deciding to trigger TTL compaction. When ttl == 0, TTL compaction is disabled.

return Status::NotSupported(
"FIFO compaction only supported with max_open_files = -1.");
}

return s;
}

Expand Down
2 changes: 2 additions & 0 deletions db/db_bloom_filter_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand Down
1 change: 1 addition & 0 deletions db/db_properties_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);

Expand Down
12 changes: 8 additions & 4 deletions db/db_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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));
}
Expand Down
1 change: 1 addition & 0 deletions db/db_test_util.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -487,6 +487,7 @@ Options DBTestBase::GetOptions(
}
case kFIFOCompaction: {
options.compaction_style = kCompactionStyleFIFO;
options.max_open_files = -1;
break;
}
case kBlockBasedTableWithPrefixHashIndex: {
Expand Down
26 changes: 24 additions & 2 deletions utilities/option_change_migration/option_change_migration_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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;
Expand Down