From 98f11d01bdf2924eae13434e4762687f41904e90 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 | 4 ++++ db/compact_files_test.cc | 41 +++++++++++++++++++++++++++++++++++++++ db/compaction_picker.cc | 30 +++++++++++++++++++++------- include/rocksdb/options.h | 3 +++ include/rocksdb/version.h | 2 +- 5 files changed, 72 insertions(+), 8 deletions(-) diff --git a/HISTORY.md b/HISTORY.md index c0c9ba7d0..a291bfeeb 100644 --- a/HISTORY.md +++ b/HISTORY.md @@ -1,4 +1,8 @@ # Rocksdb Change Log +## 5.16.3 (10/1/2018) +### Bug Fixes +* 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.2 (9/21/2018) ### Bug Fixes * Fix bug in partition filters with format_version=4. 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 b8f566afe..f3f62933c 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 687871dc5..7171d7c5d 100644 --- a/include/rocksdb/options.h +++ b/include/rocksdb/options.h @@ -1201,6 +1201,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 diff --git a/include/rocksdb/version.h b/include/rocksdb/version.h index ca58d7a21..1a14b3ef1 100644 --- a/include/rocksdb/version.h +++ b/include/rocksdb/version.h @@ -6,7 +6,7 @@ #define ROCKSDB_MAJOR 5 #define ROCKSDB_MINOR 16 -#define ROCKSDB_PATCH 2 +#define ROCKSDB_PATCH 3 // Do not use these. We made the mistake of declaring macros starting with // double underscore. Now we have to live with our choice. We'll deprecate these