From 98174bc3a31e8fd195a734d8e7c835bd3f14dc6c Mon Sep 17 00:00:00 2001 From: Yanqin Jin Date: Fri, 13 Nov 2020 11:51:24 -0800 Subject: [PATCH] Hack to load OPTIONS file for read_amp_bytes_per_bit (#7659) Summary: A temporary hack to work around a bug in 6.10, 6.11, 6.12, 6.13 and 6.14. The bug will write out 8 bytes to OPTIONS file from the starting address of BlockBasedTableOptions.read_amp_bytes_per_bit which is actually a uint32. Consequently, the value of read_amp_bytes_per_bit written in the OPTIONS file is wrong. From 6.15, RocksDB will try to parse the read_amp_bytes_per_bit from OPTIONS file as a uint32. To be able to load OPTIONS file generated by affected releases before the fix, we need to manually parse read_amp_bytes_per_bit with this hack. Pull Request resolved: https://github.com/facebook/rocksdb/pull/7659 Test Plan: Generate a db with current 6.14.fb (head at https://github.com/facebook/rocksdb/commit/b6db05dbb5364c658c5401a8078d73697bb5f31d). Maybe use db_stress. Checkout this PR, run ``` ~/rocksdb/ldb --db=. --try_load_options --ignore_unknown_options idump --count_only ``` Expect success, and should not see ``` Failed: Invalid argument: Error parsing read_amp_bytes_per_bit:17179869184 ``` Also make check Reviewed By: anand1976 Differential Revision: D24954752 Pulled By: riversand963 fbshipit-source-id: c7b802fc3e52acd050a4fc1cd475016122234394 --- options/options_test.cc | 20 +++++++++++++------ .../block_based/block_based_table_factory.cc | 17 +++++++++++++++- 2 files changed, 30 insertions(+), 7 deletions(-) diff --git a/options/options_test.cc b/options/options_test.cc index c068843d3..ecacab0ea 100644 --- a/options/options_test.cc +++ b/options/options_test.cc @@ -813,7 +813,12 @@ TEST_F(OptionsTest, GetBlockBasedTableOptionsFromString) { "block_cache=1M;block_cache_compressed=1k;block_size=1024;" "block_size_deviation=8;block_restart_interval=4;" "format_version=5;whole_key_filtering=1;" - "filter_policy=bloomfilter:4.567:false;", + "filter_policy=bloomfilter:4.567:false;" + // A bug caused read_amp_bytes_per_bit to be a large integer in OPTIONS + // file generated by 6.10 to 6.14. Though bug is fixed in these releases, + // we need to handle the case of loading OPTIONS file generated before the + // fix. + "read_amp_bytes_per_bit=17179869185;", &new_opt)); ASSERT_TRUE(new_opt.cache_index_and_filter_blocks); ASSERT_EQ(new_opt.index_type, BlockBasedTableOptions::kHashSearch); @@ -834,6 +839,9 @@ TEST_F(OptionsTest, GetBlockBasedTableOptionsFromString) { dynamic_cast(*new_opt.filter_policy); EXPECT_EQ(bfp.GetMillibitsPerKey(), 4567); EXPECT_EQ(bfp.GetWholeBitsPerKey(), 5); + // Verify that only the lower 32bits are stored in + // new_opt.read_amp_bytes_per_bit. + EXPECT_EQ(1U, new_opt.read_amp_bytes_per_bit); // unknown option ASSERT_NOK(GetBlockBasedTableOptionsFromString( @@ -1445,7 +1453,7 @@ TEST_F(OptionsTest, ConvertOptionsTest) { // This test suite tests the old APIs into the Configure options methods. // Once those APIs are officially deprecated, this test suite can be deleted. class OptionsOldApiTest : public testing::Test {}; - + TEST_F(OptionsOldApiTest, GetOptionsFromMapTest) { std::unordered_map cf_options_map = { {"write_buffer_size", "1"}, @@ -1910,7 +1918,7 @@ TEST_F(OptionsOldApiTest, GetColumnFamilyOptionsFromStringTest) { ASSERT_TRUE(new_cf_opt.memtable_factory != nullptr); ASSERT_EQ(std::string(new_cf_opt.memtable_factory->Name()), "SkipListFactory"); } - + TEST_F(OptionsOldApiTest, GetBlockBasedTableOptionsFromString) { BlockBasedTableOptions table_opt; BlockBasedTableOptions new_opt; @@ -2085,7 +2093,7 @@ TEST_F(OptionsOldApiTest, GetBlockBasedTableOptionsFromString) { ->GetHighPriPoolRatio(), 0.5); } - + TEST_F(OptionsOldApiTest, GetPlainTableOptionsFromString) { PlainTableOptions table_opt; PlainTableOptions new_opt; @@ -2116,7 +2124,7 @@ TEST_F(OptionsOldApiTest, GetPlainTableOptionsFromString) { "encoding_type=kPrefixXX", &new_opt)); } - + TEST_F(OptionsOldApiTest, GetOptionsFromStringTest) { Options base_options, new_options; base_options.write_buffer_size = 20; @@ -2674,7 +2682,7 @@ TEST_F(OptionsParserTest, Readahead) { uint64_t file_size = 0; ASSERT_OK(env_->GetFileSize(kOptionsFileName, &file_size)); assert(file_size > 0); - + RocksDBOptionsParser parser; env_->num_seq_file_read_ = 0; diff --git a/table/block_based/block_based_table_factory.cc b/table/block_based/block_based_table_factory.cc index 67b3be2fe..7aeda6896 100644 --- a/table/block_based/block_based_table_factory.cc +++ b/table/block_based/block_based_table_factory.cc @@ -332,7 +332,22 @@ static std::unordered_map {"read_amp_bytes_per_bit", {offsetof(struct BlockBasedTableOptions, read_amp_bytes_per_bit), OptionType::kUInt32T, OptionVerificationType::kNormal, - OptionTypeFlags::kNone, 0}}, + OptionTypeFlags::kNone, 0, + [](const ConfigOptions& /*opts*/, const std::string& /*name*/, + const std::string& value, char* addr) { + // A workaround to fix a bug in 6.10, 6.11, 6.12, 6.13 + // and 6.14. The bug will write out 8 bytes to OPTIONS file from the + // starting address of BlockBasedTableOptions.read_amp_bytes_per_bit + // which is actually a uint32. Consequently, the value of + // read_amp_bytes_per_bit written in the OPTIONS file is wrong. + // From 6.15, RocksDB will try to parse the read_amp_bytes_per_bit + // from OPTIONS file as a uint32. To be able to load OPTIONS file + // generated by affected releases before the fix, we need to + // manually parse read_amp_bytes_per_bit with this special hack. + uint64_t read_amp_bytes_per_bit = ParseUint64(value); + EncodeFixed32(addr, static_cast(read_amp_bytes_per_bit)); + return Status::OK(); + }}}, {"enable_index_compression", {offsetof(struct BlockBasedTableOptions, enable_index_compression), OptionType::kBoolean, OptionVerificationType::kNormal,