From 5562b80040fdc295072e4c91227c55205075cded Mon Sep 17 00:00:00 2001 From: anand76 Date: Fri, 8 Jan 2021 16:06:51 -0800 Subject: [PATCH 1/2] Make ldb load column family options from OPTIONS file Summary: When the --try_load_options is used in conjunction with the --column_family option, ldb incorrectly sets the ColumnFamilyOptions for that column family to defaults. This PR fixes that by retaining from the OPTIONS file and applying command line overrides. Test Plan: Add a unit test in ldb_cmd_test --- HISTORY.md | 1 + include/rocksdb/utilities/ldb_cmd.h | 2 + tools/ldb_cmd.cc | 95 +++++++++++++++-------------- tools/ldb_cmd_impl.h | 4 +- tools/ldb_cmd_test.cc | 47 ++++++++++++++ 5 files changed, 101 insertions(+), 48 deletions(-) diff --git a/HISTORY.md b/HISTORY.md index 923f2e44999..96909e73252 100644 --- a/HISTORY.md +++ b/HISTORY.md @@ -23,6 +23,7 @@ * Fixed prefix extractor with timestamp issues. * Fixed a bug in atomic flush: in two-phase commit mode, the minimum WAL log number to keep is incorrect. * Fixed a bug related to checkpoint in PR7789: if there are multiple column families, and the checkpoint is not opened as read only, then in rare cases, data loss may happen in the checkpoint. Since backup engine relies on checkpoint, it may also be affected. +* When ldb --try_load_options is used with the --column_family option, the ColumnFamilyOptions for the specified column family was not loaded from the OPTIONS file. Fix it so its loaded from OPTIONS and then overridden with command line overrides. ### New Features * User defined timestamp feature supports `CompactRange` and `GetApproximateSizes`. diff --git a/include/rocksdb/utilities/ldb_cmd.h b/include/rocksdb/utilities/ldb_cmd.h index c7f227fc02a..af0556a3200 100644 --- a/include/rocksdb/utilities/ldb_cmd.h +++ b/include/rocksdb/utilities/ldb_cmd.h @@ -88,6 +88,8 @@ class LDBCommand { virtual void OverrideBaseOptions(); + virtual void OverrideBaseCFOptions(ColumnFamilyOptions* cf_opts); + virtual void SetDBOptions(Options options) { options_ = options; } virtual void SetColumnFamilies( diff --git a/tools/ldb_cmd.cc b/tools/ldb_cmd.cc index cf581904251..80c6c7778c3 100644 --- a/tools/ldb_cmd.cc +++ b/tools/ldb_cmd.cc @@ -553,6 +553,26 @@ bool LDBCommand::ParseStringOption( void LDBCommand::OverrideBaseOptions() { options_.create_if_missing = false; + int db_write_buffer_size; + if (ParseIntOption(option_map_, ARG_DB_WRITE_BUFFER_SIZE, + db_write_buffer_size, exec_state_)) { + if (db_write_buffer_size >= 0) { + options_.db_write_buffer_size = db_write_buffer_size; + } else { + exec_state_ = LDBCommandExecuteResult::Failed(ARG_DB_WRITE_BUFFER_SIZE + + " must be >= 0."); + } + } + + if (options_.db_paths.size() == 0) { + options_.db_paths.emplace_back(db_path_, + std::numeric_limits::max()); + } + + OverrideBaseCFOptions(static_cast(&options_)); +} + +void LDBCommand::OverrideBaseCFOptions(ColumnFamilyOptions* cf_opts) { BlockBasedTableOptions table_options; bool use_table_options = false; int bits; @@ -577,35 +597,35 @@ void LDBCommand::OverrideBaseOptions() { } } - options_.force_consistency_checks = force_consistency_checks_; + cf_opts->force_consistency_checks = force_consistency_checks_; if (use_table_options) { - options_.table_factory.reset(NewBlockBasedTableFactory(table_options)); + cf_opts->table_factory.reset(NewBlockBasedTableFactory(table_options)); } auto itr = option_map_.find(ARG_AUTO_COMPACTION); if (itr != option_map_.end()) { - options_.disable_auto_compactions = !StringToBool(itr->second); + cf_opts->disable_auto_compactions = !StringToBool(itr->second); } itr = option_map_.find(ARG_COMPRESSION_TYPE); if (itr != option_map_.end()) { std::string comp = itr->second; if (comp == "no") { - options_.compression = kNoCompression; + cf_opts->compression = kNoCompression; } else if (comp == "snappy") { - options_.compression = kSnappyCompression; + cf_opts->compression = kSnappyCompression; } else if (comp == "zlib") { - options_.compression = kZlibCompression; + cf_opts->compression = kZlibCompression; } else if (comp == "bzip2") { - options_.compression = kBZip2Compression; + cf_opts->compression = kBZip2Compression; } else if (comp == "lz4") { - options_.compression = kLZ4Compression; + cf_opts->compression = kLZ4Compression; } else if (comp == "lz4hc") { - options_.compression = kLZ4HCCompression; + cf_opts->compression = kLZ4HCCompression; } else if (comp == "xpress") { - options_.compression = kXpressCompression; + cf_opts->compression = kXpressCompression; } else if (comp == "zstd") { - options_.compression = kZSTD; + cf_opts->compression = kZSTD; } else { // Unknown compression. exec_state_ = @@ -617,29 +637,18 @@ void LDBCommand::OverrideBaseOptions() { if (ParseIntOption(option_map_, ARG_COMPRESSION_MAX_DICT_BYTES, compression_max_dict_bytes, exec_state_)) { if (compression_max_dict_bytes >= 0) { - options_.compression_opts.max_dict_bytes = compression_max_dict_bytes; + cf_opts->compression_opts.max_dict_bytes = compression_max_dict_bytes; } else { exec_state_ = LDBCommandExecuteResult::Failed( ARG_COMPRESSION_MAX_DICT_BYTES + " must be >= 0."); } } - int db_write_buffer_size; - if (ParseIntOption(option_map_, ARG_DB_WRITE_BUFFER_SIZE, - db_write_buffer_size, exec_state_)) { - if (db_write_buffer_size >= 0) { - options_.db_write_buffer_size = db_write_buffer_size; - } else { - exec_state_ = LDBCommandExecuteResult::Failed(ARG_DB_WRITE_BUFFER_SIZE + - " must be >= 0."); - } - } - int write_buffer_size; if (ParseIntOption(option_map_, ARG_WRITE_BUFFER_SIZE, write_buffer_size, exec_state_)) { if (write_buffer_size > 0) { - options_.write_buffer_size = write_buffer_size; + cf_opts->write_buffer_size = write_buffer_size; } else { exec_state_ = LDBCommandExecuteResult::Failed(ARG_WRITE_BUFFER_SIZE + " must be > 0."); @@ -649,23 +658,18 @@ void LDBCommand::OverrideBaseOptions() { int file_size; if (ParseIntOption(option_map_, ARG_FILE_SIZE, file_size, exec_state_)) { if (file_size > 0) { - options_.target_file_size_base = file_size; + cf_opts->target_file_size_base = file_size; } else { exec_state_ = LDBCommandExecuteResult::Failed(ARG_FILE_SIZE + " must be > 0."); } } - if (options_.db_paths.size() == 0) { - options_.db_paths.emplace_back(db_path_, - std::numeric_limits::max()); - } - int fix_prefix_len; if (ParseIntOption(option_map_, ARG_FIX_PREFIX_LEN, fix_prefix_len, exec_state_)) { if (fix_prefix_len > 0) { - options_.prefix_extractor.reset( + cf_opts->prefix_extractor.reset( NewFixedPrefixTransform(static_cast(fix_prefix_len))); } else { exec_state_ = @@ -740,7 +744,7 @@ void LDBCommand::PrepareOptions() { "Non-existing column family " + column_family_name_); return; } - column_families_iter->options = options_; + OverrideBaseCFOptions(&column_families_iter->options); } } @@ -1947,14 +1951,14 @@ void ReduceDBLevelsCommand::Help(std::string& ret) { ret.append("\n"); } -void ReduceDBLevelsCommand::OverrideBaseOptions() { - LDBCommand::OverrideBaseOptions(); - options_.num_levels = old_levels_; - options_.max_bytes_for_level_multiplier_additional.resize(options_.num_levels, +void ReduceDBLevelsCommand::OverrideBaseCFOptions( + ColumnFamilyOptions* cf_opts) { + cf_opts->num_levels = old_levels_; + cf_opts->max_bytes_for_level_multiplier_additional.resize(cf_opts->num_levels, 1); // Disable size compaction - options_.max_bytes_for_level_base = 1ULL << 50; - options_.max_bytes_for_level_multiplier = 1; + cf_opts->max_bytes_for_level_base = 1ULL << 50; + cf_opts->max_bytes_for_level_multiplier = 1; } Status ReduceDBLevelsCommand::GetOldNumOfLevels(Options& opt, @@ -2100,18 +2104,17 @@ void ChangeCompactionStyleCommand::Help(std::string& ret) { ret.append("\n"); } -void ChangeCompactionStyleCommand::OverrideBaseOptions() { - LDBCommand::OverrideBaseOptions(); - +void ChangeCompactionStyleCommand::OverrideBaseCFOptions( + ColumnFamilyOptions* cf_opts) { if (old_compaction_style_ == kCompactionStyleLevel && new_compaction_style_ == kCompactionStyleUniversal) { // In order to convert from level compaction to universal compaction, we // need to compact all data into a single file and move it to level 0. - options_.disable_auto_compactions = true; - options_.target_file_size_base = INT_MAX; - options_.target_file_size_multiplier = 1; - options_.max_bytes_for_level_base = INT_MAX; - options_.max_bytes_for_level_multiplier = 1; + cf_opts->disable_auto_compactions = true; + cf_opts->target_file_size_base = INT_MAX; + cf_opts->target_file_size_multiplier = 1; + cf_opts->max_bytes_for_level_base = INT_MAX; + cf_opts->max_bytes_for_level_multiplier = 1; } } diff --git a/tools/ldb_cmd_impl.h b/tools/ldb_cmd_impl.h index c0bfc19aaca..d20f5b98e77 100644 --- a/tools/ldb_cmd_impl.h +++ b/tools/ldb_cmd_impl.h @@ -262,7 +262,7 @@ class ReduceDBLevelsCommand : public LDBCommand { const std::map& options, const std::vector& flags); - virtual void OverrideBaseOptions() override; + virtual void OverrideBaseCFOptions(ColumnFamilyOptions* cf_opts) override; virtual void DoCommand() override; @@ -294,7 +294,7 @@ class ChangeCompactionStyleCommand : public LDBCommand { const std::map& options, const std::vector& flags); - virtual void OverrideBaseOptions() override; + virtual void OverrideBaseCFOptions(ColumnFamilyOptions* cf_opts) override; virtual void DoCommand() override; diff --git a/tools/ldb_cmd_test.cc b/tools/ldb_cmd_test.cc index 504a5d72ccd..159d752d8b2 100644 --- a/tools/ldb_cmd_test.cc +++ b/tools/ldb_cmd_test.cc @@ -13,6 +13,7 @@ #include "file/filename.h" #include "port/stack_trace.h" #include "rocksdb/file_checksum.h" +#include "rocksdb/utilities/options_util.h" #include "test_util/sync_point.h" #include "test_util/testharness.h" #include "test_util/testutil.h" @@ -672,6 +673,52 @@ TEST_F(LdbCmdTest, TestBadDbPath) { ASSERT_EQ(1, LDBCommandRunner::RunCommand(4, argv, opts, LDBOptions(), nullptr)); } + +TEST_F(LdbCmdTest, LoadCFOptionsAndOverride) { + // Env* base_env = TryLoadCustomOrDefaultEnv(); + // std::unique_ptr env(NewMemEnv(base_env)); + std::unique_ptr env(new EnvWrapper(Env::Default())); + Options opts; + opts.env = env.get(); + opts.create_if_missing = true; + + DB* db = nullptr; + std::string dbname = test::PerThreadDBPath(env.get(), "ldb_cmd_test"); + DestroyDB(dbname, opts); + ASSERT_OK(DB::Open(opts, dbname, &db)); + + ColumnFamilyHandle* cf_handle; + ColumnFamilyOptions cf_opts; + cf_opts.num_levels = 20; + ASSERT_OK(db->CreateColumnFamily(cf_opts, "cf1", &cf_handle)); + + delete cf_handle; + delete db; + + char arg1[] = "./ldb"; + char arg2[1024]; + snprintf(arg2, sizeof(arg2), "--db=%s", dbname.c_str()); + char arg3[] = "put"; + char arg4[] = "key1"; + char arg5[] = "value1"; + char arg6[] = "--try_load_options"; + char arg7[] = "--column_family=cf1"; + char arg8[] = "--write_buffer_size=268435456"; + char* argv[] = {arg1, arg2, arg3, arg4, arg5, arg6, arg7, arg8}; + + ASSERT_EQ(0, + LDBCommandRunner::RunCommand(8, argv, opts, LDBOptions(), nullptr)); + + ConfigOptions config_opts; + Options options; + std::vector column_families; + config_opts.env = env.get(); + ASSERT_OK(LoadLatestOptions(config_opts, dbname, &options, &column_families)); + ASSERT_EQ(column_families.size(), 2); + ASSERT_EQ(options.num_levels, opts.num_levels); + ASSERT_EQ(column_families[1].options.num_levels, cf_opts.num_levels); + ASSERT_EQ(column_families[1].options.write_buffer_size, 268435456); +} } // namespace ROCKSDB_NAMESPACE #ifdef ROCKSDB_UNITTESTS_WITH_CUSTOM_OBJECTS_FROM_STATIC_LIBS From a44fa3098e1b6b44bbb292daf6276f4fffae8384 Mon Sep 17 00:00:00 2001 From: anand76 Date: Mon, 11 Jan 2021 14:40:31 -0800 Subject: [PATCH 2/2] Address review comments --- tools/ldb_cmd.cc | 2 ++ 1 file changed, 2 insertions(+) diff --git a/tools/ldb_cmd.cc b/tools/ldb_cmd.cc index 80c6c7778c3..e1d0f608436 100644 --- a/tools/ldb_cmd.cc +++ b/tools/ldb_cmd.cc @@ -1953,6 +1953,7 @@ void ReduceDBLevelsCommand::Help(std::string& ret) { void ReduceDBLevelsCommand::OverrideBaseCFOptions( ColumnFamilyOptions* cf_opts) { + LDBCommand::OverrideBaseCFOptions(cf_opts); cf_opts->num_levels = old_levels_; cf_opts->max_bytes_for_level_multiplier_additional.resize(cf_opts->num_levels, 1); @@ -2106,6 +2107,7 @@ void ChangeCompactionStyleCommand::Help(std::string& ret) { void ChangeCompactionStyleCommand::OverrideBaseCFOptions( ColumnFamilyOptions* cf_opts) { + LDBCommand::OverrideBaseCFOptions(cf_opts); if (old_compaction_style_ == kCompactionStyleLevel && new_compaction_style_ == kCompactionStyleUniversal) { // In order to convert from level compaction to universal compaction, we