From ac6f435a9ab3819256607ad5de79c256cca0ce0c Mon Sep 17 00:00:00 2001 From: Andrew Kryczka Date: Mon, 1 Oct 2018 01:16:16 -0700 Subject: [PATCH] Fix CompactFiles support for kDisableCompressionOption (#4438) Summary: Previously `CompactFiles` with `CompressionType::kDisableCompressionOption` caused program to crash on assertion failure. This PR fixes the crash by adding support for that setting. Now, that setting will cause RocksDB to choose compression according to the column family's options. Pull Request resolved: https://github.com/facebook/rocksdb/pull/4438 Differential Revision: D10115761 Pulled By: ajkr fbshipit-source-id: a553c6fa76fa5b6f73b0d165d95640da6f454122 --- HISTORY.md | 2 ++ db/compact_files_test.cc | 41 +++++++++++++++++++++++++++++++++++++++ db/compaction_picker.cc | 30 +++++++++++++++++++++------- include/rocksdb/options.h | 3 +++ 4 files changed, 69 insertions(+), 7 deletions(-) diff --git a/HISTORY.md b/HISTORY.md index a4d28e0c1..b4baa7162 100644 --- a/HISTORY.md +++ b/HISTORY.md @@ -7,10 +7,12 @@ ### New Features * TransactionOptions::skip_concurrency_control allows pessimistic transactions to skip the overhead of concurrency control. Could be used for optimizing certain transactions or during recovery. + ### Bug Fixes * Avoid creating empty SSTs and subsequently deleting them in certain cases during compaction. * Sync CURRENT file contents during checkpoint. * Fix format_version 4 bug with partitioned filters +* Fix crash caused when `CompactFiles` run with `CompactionOptions::compression == CompressionType::kDisableCompressionOption`. Now that setting causes the compression type to be chosen according to the column family-wide compression options. ## 5.16.0 (8/21/2018) ### Public API Change diff --git a/db/compact_files_test.cc b/db/compact_files_test.cc index 499d88b3e..b93a88611 100644 --- a/db/compact_files_test.cc +++ b/db/compact_files_test.cc @@ -308,6 +308,47 @@ TEST_F(CompactFilesTest, CompactionFilterWithGetSv) { delete db; } +TEST_F(CompactFilesTest, SentinelCompressionType) { + // Check that passing `CompressionType::kDisableCompressionOption` to + // `CompactFiles` causes it to use the column family compression options. + for (auto compaction_style : + {CompactionStyle::kCompactionStyleLevel, + CompactionStyle::kCompactionStyleUniversal, + CompactionStyle::kCompactionStyleNone}) { + DestroyDB(db_name_, Options()); + Options options; + options.compaction_style = compaction_style; + // L0: Snappy, L1: ZSTD, L2: Snappy + options.compression_per_level = {CompressionType::kSnappyCompression, + CompressionType::kZlibCompression, + CompressionType::kSnappyCompression}; + options.create_if_missing = true; + FlushedFileCollector* collector = new FlushedFileCollector(); + options.listeners.emplace_back(collector); + DB* db = nullptr; + ASSERT_OK(DB::Open(options, db_name_, &db)); + + db->Put(WriteOptions(), "key", "val"); + db->Flush(FlushOptions()); + + auto l0_files = collector->GetFlushedFiles(); + ASSERT_EQ(1, l0_files.size()); + + // L0->L1 compaction, so output should be ZSTD-compressed + CompactionOptions compaction_opts; + compaction_opts.compression = CompressionType::kDisableCompressionOption; + ASSERT_OK(db->CompactFiles(compaction_opts, l0_files, 1)); + + rocksdb::TablePropertiesCollection all_tables_props; + ASSERT_OK(db->GetPropertiesOfAllTables(&all_tables_props)); + for (const auto& name_and_table_props : all_tables_props) { + ASSERT_EQ(CompressionTypeToString(CompressionType::kZlibCompression), + name_and_table_props.second->compression_name); + } + delete db; + } +} + } // namespace rocksdb int main(int argc, char** argv) { diff --git a/db/compaction_picker.cc b/db/compaction_picker.cc index 00e660a07..4931eddfe 100644 --- a/db/compaction_picker.cc +++ b/db/compaction_picker.cc @@ -315,13 +315,29 @@ Compaction* CompactionPicker::CompactFiles( // shouldn't have been released since. assert(!FilesRangeOverlapWithCompaction(input_files, output_level)); - auto c = - new Compaction(vstorage, ioptions_, mutable_cf_options, input_files, - output_level, compact_options.output_file_size_limit, - mutable_cf_options.max_compaction_bytes, output_path_id, - compact_options.compression, ioptions_.compression_opts, - compact_options.max_subcompactions, - /* grandparents */ {}, true); + CompressionType compression_type; + if (compact_options.compression == kDisableCompressionOption) { + int base_level; + if (ioptions_.compaction_style == kCompactionStyleLevel) { + base_level = vstorage->base_level(); + } else { + base_level = 1; + } + compression_type = + GetCompressionType(ioptions_, vstorage, mutable_cf_options, + output_level, base_level); + } else { + // TODO(ajkr): `CompactionOptions` offers configurable `CompressionType` + // without configurable `CompressionOptions`, which is inconsistent. + compression_type = compact_options.compression; + } + auto c = new Compaction( + vstorage, ioptions_, mutable_cf_options, input_files, output_level, + compact_options.output_file_size_limit, + mutable_cf_options.max_compaction_bytes, output_path_id, compression_type, + GetCompressionOptions(ioptions_, vstorage, output_level), + compact_options.max_subcompactions, + /* grandparents */ {}, true); RegisterCompaction(c); return c; } diff --git a/include/rocksdb/options.h b/include/rocksdb/options.h index 3b851874f..c4597c489 100644 --- a/include/rocksdb/options.h +++ b/include/rocksdb/options.h @@ -1200,6 +1200,9 @@ extern Status CreateLoggerFromOptions(const std::string& dbname, struct CompactionOptions { // Compaction output compression type // Default: snappy + // If set to `kDisableCompressionOption`, RocksDB will choose compression type + // according to the `ColumnFamilyOptions`, taking into account the output + // level if `compression_per_level` is specified. CompressionType compression; // Compaction will create files of size `output_file_size_limit`. // Default: MAX, which means that compaction will create a single file