From 94f90ac6bccecaa041d1c71043a69f36c2a33e41 Mon Sep 17 00:00:00 2001 From: sdong Date: Wed, 8 Apr 2020 14:37:01 -0700 Subject: [PATCH] =?UTF-8?q?compression=20related=20options=20are=20not=20c?= =?UTF-8?q?opied=20back=20from=20MutableCFOptions=E2=80=A6=20(#6668)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Summary: … to CFOptions https://github.com/facebook/rocksdb/pull/6615 made several compression related options dynamically changeable. They are moved to MutableCFOptions. However, they are not copied back to ColumnFamilyOptions, so the changed values are not written to option files and for some other uses. Fix it by copying them back. Pull Request resolved: https://github.com/facebook/rocksdb/pull/6668 Test Plan: Add a unit test to make sure that when a MutableCFOptions is converted to CFOptions and back to MutableCFOptions, they stay the same. This test would fail without the fix. Reviewed By: ajkr Differential Revision: D20923999 fbshipit-source-id: c3bccd6923b00d677764e2269bed6a95ad7ed780 --- HISTORY.md | 3 ++ options/options_helper.cc | 4 ++ options/options_settable_test.cc | 67 +++++++++++++++++++++++++++++--- 3 files changed, 69 insertions(+), 5 deletions(-) diff --git a/HISTORY.md b/HISTORY.md index e4b49f044..a65500b1e 100644 --- a/HISTORY.md +++ b/HISTORY.md @@ -3,6 +3,9 @@ ### New Features * Added support for pipelined & parallel compression optimization for `BlockBasedTableBuilder`. This optimization makes block building, block compression and block appending a pipeline, and uses multiple threads to accelerate block compression. Users can set `CompressionOptions::parallel_threads` greater than 1 to enable compression parallelism. +### Bug Fixes +* Fix a bug when making options.bottommost_compression, options.compression_opts and options.bottommost_compression_opts dynamically changeable: the modified values are not written to option files or returned back to users when being queried. + ## 6.9.0 (03/29/2020) ### Behavior changes * Since RocksDB 6.8, ttl-based FIFO compaction can drop a file whose oldest key becomes older than options.ttl while others have not. This fix reverts this and makes ttl-based FIFO compaction use the file's flush time as the criterion. This fix also requires that max_open_files = -1 and compaction_options_fifo.allow_compaction = false to function properly. diff --git a/options/options_helper.cc b/options/options_helper.cc index f3ee13ad8..d844888bf 100644 --- a/options/options_helper.cc +++ b/options/options_helper.cc @@ -208,6 +208,10 @@ ColumnFamilyOptions BuildColumnFamilyOptions( cf_opts.paranoid_file_checks = mutable_cf_options.paranoid_file_checks; cf_opts.report_bg_io_stats = mutable_cf_options.report_bg_io_stats; cf_opts.compression = mutable_cf_options.compression; + cf_opts.compression_opts = mutable_cf_options.compression_opts; + cf_opts.bottommost_compression = mutable_cf_options.bottommost_compression; + cf_opts.bottommost_compression_opts = + mutable_cf_options.bottommost_compression_opts; cf_opts.sample_for_compression = mutable_cf_options.sample_for_compression; cf_opts.table_factory = options.table_factory; diff --git a/options/options_settable_test.cc b/options/options_settable_test.cc index 12a0ec9d8..94ca5be1e 100644 --- a/options/options_settable_test.cc +++ b/options/options_settable_test.cc @@ -42,13 +42,14 @@ const char kSpecialChar = 'z'; typedef std::vector> OffsetGap; void FillWithSpecialChar(char* start_ptr, size_t total_size, - const OffsetGap& blacklist) { + const OffsetGap& blacklist, + char special_char = kSpecialChar) { size_t offset = 0; for (auto& pair : blacklist) { - std::memset(start_ptr + offset, kSpecialChar, pair.first - offset); + std::memset(start_ptr + offset, special_char, pair.first - offset); offset = pair.first + pair.second; } - std::memset(start_ptr + offset, kSpecialChar, total_size - offset); + std::memset(start_ptr + offset, special_char, total_size - offset); } int NumUnsetBytes(char* start_ptr, size_t total_size, @@ -71,6 +72,26 @@ int NumUnsetBytes(char* start_ptr, size_t total_size, return total_unset_bytes_base; } +// Return true iff two structs are the same except blacklist fields. +bool CompareBytes(char* start_ptr1, char* start_ptr2, size_t total_size, + const OffsetGap& blacklist) { + size_t offset = 0; + for (auto& pair : blacklist) { + for (; offset < pair.first; offset++) { + if (*(start_ptr1 + offset) != *(start_ptr2 + offset)) { + return false; + } + } + offset = pair.first + pair.second; + } + for (; offset < total_size; offset++) { + if (*(start_ptr1 + offset) != *(start_ptr2 + offset)) { + return false; + } + } + return true; +} + // If the test fails, likely a new option is added to BlockBasedTableOptions // but it cannot be set through GetBlockBasedTableOptionsFromString(), or the // test is not updated accordingly. @@ -373,6 +394,7 @@ TEST_F(OptionsSettableTest, ColumnFamilyOptionsAllFieldsSettable) { ColumnFamilyOptions* options = new (options_ptr) ColumnFamilyOptions(); FillWithSpecialChar(options_ptr, sizeof(ColumnFamilyOptions), kColumnFamilyOptionsBlacklist); + // It based on the behavior of compiler that padding bytes are not changed // when copying the struct. It's prone to failure when compiler behavior // changes. We verify there is unset bytes to detect the case. @@ -395,8 +417,6 @@ TEST_F(OptionsSettableTest, ColumnFamilyOptionsAllFieldsSettable) { // GetColumnFamilyOptionsFromString(): options->rate_limit_delay_max_milliseconds = 33; options->compaction_options_universal = CompactionOptionsUniversal(); - options->compression_opts = CompressionOptions(); - options->bottommost_compression_opts = CompressionOptions(); options->hard_rate_limit = 0; options->soft_rate_limit = 0; options->purge_redundant_kvs_while_flush = false; @@ -434,6 +454,8 @@ TEST_F(OptionsSettableTest, ColumnFamilyOptionsAllFieldsSettable) { "max_bytes_for_level_multiplier=60;" "memtable_factory=SkipListFactory;" "compression=kNoCompression;" + "compression_opts=5:6:7:8:9:10:true;" + "bottommost_compression_opts=4:5:6:7:8:9:true;" "bottommost_compression=kDisableCompressionOption;" "level0_stop_writes_trigger=33;" "num_levels=99;" @@ -470,11 +492,46 @@ TEST_F(OptionsSettableTest, ColumnFamilyOptionsAllFieldsSettable) { NumUnsetBytes(new_options_ptr, sizeof(ColumnFamilyOptions), kColumnFamilyOptionsBlacklist)); + ColumnFamilyOptions rnd_filled_options = *new_options; + options->~ColumnFamilyOptions(); new_options->~ColumnFamilyOptions(); delete[] options_ptr; delete[] new_options_ptr; + + // Test copying to mutabable and immutable options and copy back the mutable + // part. + const OffsetGap kMutableCFOptionsBlacklist = { + {offset_of(&MutableCFOptions::prefix_extractor), + sizeof(std::shared_ptr)}, + {offset_of(&MutableCFOptions::max_bytes_for_level_multiplier_additional), + sizeof(std::vector)}, + {offset_of(&MutableCFOptions::max_file_size), + sizeof(std::vector)}, + }; + + // Construct two pieces of memory with controlled base. This is needed as + // otherwise padding bytes might not be filled. + char* mcfo1_ptr = new char[sizeof(MutableCFOptions)]; + MutableCFOptions* mcfo1 = new (mcfo1_ptr) MutableCFOptions(); + FillWithSpecialChar(mcfo1_ptr, sizeof(MutableCFOptions), + kMutableCFOptionsBlacklist, 'x'); + char* mcfo2_ptr = new char[sizeof(MutableCFOptions)]; + MutableCFOptions* mcfo2 = new (mcfo2_ptr) MutableCFOptions(); + FillWithSpecialChar(mcfo2_ptr, sizeof(MutableCFOptions), + kMutableCFOptionsBlacklist, 'x'); + + rnd_filled_options.num_levels = 66; + *mcfo1 = MutableCFOptions(rnd_filled_options); + ColumnFamilyOptions cfo_back = + BuildColumnFamilyOptions(ColumnFamilyOptions(), *mcfo1); + *mcfo2 = MutableCFOptions(cfo_back); + + ASSERT_TRUE(CompareBytes(mcfo1_ptr, mcfo2_ptr, sizeof(MutableCFOptions), + kMutableCFOptionsBlacklist)); + delete[] mcfo1; + delete[] mcfo2; } #endif // !__clang__ #endif // OS_LINUX || OS_WIN