From ec346da98c6326f65473331cec06818a6fa3d23b Mon Sep 17 00:00:00 2001 From: Andrew Kryczka Date: Wed, 11 Nov 2020 20:30:58 -0800 Subject: [PATCH] Always apply bottommost_compression_opts when enabled (#7633) Summary: Previously, even when `bottommost_compression_opts`'s `enabled` flag was set, it only took effect when `bottommost_compression` was also set to something other than `kDisableCompressionOption`. This wasn't documented and, if we kept the old behavior, it'd make things complicated like the migration instructions in https://github.com/facebook/rocksdb/issues/7619. We can simplify the API by making `bottommost_compression_opts` always take effect when its `enabled` flag is set. Fixes https://github.com/facebook/rocksdb/issues/7631. Pull Request resolved: https://github.com/facebook/rocksdb/pull/7633 Reviewed By: ltamasi Differential Revision: D24710358 Pulled By: ajkr fbshipit-source-id: bbbdf9c1b53c63a4239d902cc3f5a11da1874647 --- HISTORY.md | 1 + db/compaction/compaction_picker.cc | 10 ++--- db/db_options_test.cc | 60 ++++++++++++++++++++++++++++++ 3 files changed, 66 insertions(+), 5 deletions(-) diff --git a/HISTORY.md b/HISTORY.md index 58c178ac3..0f5739e2c 100644 --- a/HISTORY.md +++ b/HISTORY.md @@ -20,6 +20,7 @@ ### Behavior Changes * The dictionary compression settings specified in `ColumnFamilyOptions::compression_opts` now additionally affect files generated by flush and compaction to non-bottommost level. Previously those settings at most affected files generated by compaction to bottommost level, depending on whether `ColumnFamilyOptions::bottommost_compression_opts` overrode them. Users who relied on dictionary compression settings in `ColumnFamilyOptions::compression_opts` affecting only the bottommost level can keep the behavior by moving their dictionary settings to `ColumnFamilyOptions::bottommost_compression_opts` and setting its `enabled` flag. +* When the `enabled` flag is set in `ColumnFamilyOptions::bottommost_compression_opts`, those compression options now take effect regardless of the value in `ColumnFamilyOptions::bottommost_compression`. Previously, those compression options only took effect when `ColumnFamilyOptions::bottommost_compression != kDisableCompressionOption`. Now, they additionally take effect when `ColumnFamilyOptions::bottommost_compression == kDisableCompressionOption` (such a setting causes bottommost compression type to fall back to `ColumnFamilyOptions::compression_per_level` if configured, and otherwise fall back to `ColumnFamilyOptions::compression`). ## 6.14 (10/09/2020) ### Bug fixes diff --git a/db/compaction/compaction_picker.cc b/db/compaction/compaction_picker.cc index 523418c3d..05b30d97e 100644 --- a/db/compaction/compaction_picker.cc +++ b/db/compaction/compaction_picker.cc @@ -139,11 +139,9 @@ CompressionOptions GetCompressionOptions(const MutableCFOptions& cf_options, if (!enable_compression) { return cf_options.compression_opts; } - // If bottommost_compression is set and we are compacting to the - // bottommost level then we should use the specified compression options - // for the bottmomost_compression. - if (cf_options.bottommost_compression != kDisableCompressionOption && - level >= (vstorage->num_non_empty_levels() - 1) && + // If bottommost_compression_opts is enabled and we are compacting to the + // bottommost level then we should use the specified compression options. + if (level >= (vstorage->num_non_empty_levels() - 1) && cf_options.bottommost_compression_opts.enabled) { return cf_options.bottommost_compression_opts; } @@ -1045,6 +1043,8 @@ void CompactionPicker::RegisterCompaction(Compaction* c) { level0_compactions_in_progress_.insert(c); } compactions_in_progress_.insert(c); + TEST_SYNC_POINT_CALLBACK("CompactionPicker::RegisterCompaction:Registered", + c); } void CompactionPicker::UnregisterCompaction(Compaction* c) { diff --git a/db/db_options_test.cc b/db/db_options_test.cc index a950cb56e..d29a082f0 100644 --- a/db/db_options_test.cc +++ b/db/db_options_test.cc @@ -952,6 +952,66 @@ TEST_F(DBOptionsTest, ChangeCompression) { #endif // ROCKSDB_LITE +TEST_F(DBOptionsTest, BottommostCompressionOptsWithFallbackType) { + // Verify the bottommost compression options still take effect even when the + // bottommost compression type is left at its default value. Verify for both + // automatic and manual compaction. + if (!Snappy_Supported() || !LZ4_Supported()) { + return; + } + + constexpr int kUpperCompressionLevel = 1; + constexpr int kBottommostCompressionLevel = 2; + constexpr int kNumL0Files = 2; + + Options options = CurrentOptions(); + options.level0_file_num_compaction_trigger = kNumL0Files; + options.compression = CompressionType::kLZ4Compression; + options.compression_opts.level = kUpperCompressionLevel; + options.bottommost_compression_opts.level = kBottommostCompressionLevel; + options.bottommost_compression_opts.enabled = true; + Reopen(options); + + CompressionType compression_used = CompressionType::kDisableCompressionOption; + CompressionOptions compression_opt_used; + bool compacted = false; + SyncPoint::GetInstance()->SetCallBack( + "CompactionPicker::RegisterCompaction:Registered", [&](void* arg) { + Compaction* c = static_cast(arg); + compression_used = c->output_compression(); + compression_opt_used = c->output_compression_opts(); + compacted = true; + }); + SyncPoint::GetInstance()->EnableProcessing(); + + // First, verify for automatic compaction. + for (int i = 0; i < kNumL0Files; ++i) { + ASSERT_OK(Put("foo", "foofoofoo")); + ASSERT_OK(Put("bar", "foofoofoo")); + ASSERT_OK(Flush()); + } + ASSERT_OK(dbfull()->TEST_WaitForCompact()); + + ASSERT_TRUE(compacted); + ASSERT_EQ(CompressionType::kLZ4Compression, compression_used); + ASSERT_EQ(kBottommostCompressionLevel, compression_opt_used.level); + + // Second, verify for manual compaction. + compacted = false; + compression_used = CompressionType::kDisableCompressionOption; + compression_opt_used = CompressionOptions(); + CompactRangeOptions cro; + cro.bottommost_level_compaction = BottommostLevelCompaction::kForceOptimized; + ASSERT_OK(dbfull()->CompactRange(cro, nullptr, nullptr)); + + ROCKSDB_NAMESPACE::SyncPoint::GetInstance()->DisableProcessing(); + ROCKSDB_NAMESPACE::SyncPoint::GetInstance()->ClearAllCallBacks(); + + ASSERT_TRUE(compacted); + ASSERT_EQ(CompressionType::kLZ4Compression, compression_used); + ASSERT_EQ(kBottommostCompressionLevel, compression_opt_used.level); +} + } // namespace ROCKSDB_NAMESPACE int main(int argc, char** argv) {