From 690f1edf373a5bb42381ebfefb2935e55d1360e7 Mon Sep 17 00:00:00 2001 From: Andrew Kryczka Date: Mon, 18 Apr 2022 23:46:16 -0700 Subject: [PATCH] Avoid overwriting OPTIONS file settings in db_bench (#9862) Summary: `InitializeOptionsGeneral()` was overwriting many options that were already configured by OPTIONS file, potentially with the flag default values. This PR changes that function to only overwrite options in limited scenarios, as described at the top of its definition. Block cache is still a violation. Pull Request resolved: https://github.com/facebook/rocksdb/pull/9862 Test Plan: ran under various scenarios (multi-DB, single DB, OPTIONS file, flags) and verified options are set as expected Reviewed By: jay-zhuang Differential Revision: D35736960 Pulled By: ajkr fbshipit-source-id: 75b77740af37e6f5741618f8a8f5685df2417d03 --- tools/db_bench_tool.cc | 164 ++++++++++++++++++++++++----------------- 1 file changed, 98 insertions(+), 66 deletions(-) diff --git a/tools/db_bench_tool.cc b/tools/db_bench_tool.cc index f356e08f43..586a7884a7 100644 --- a/tools/db_bench_tool.cc +++ b/tools/db_bench_tool.cc @@ -3857,6 +3857,26 @@ class Benchmark { assert(db_.db == nullptr); options.env = FLAGS_env; + options.wal_dir = FLAGS_wal_dir; + options.dump_malloc_stats = FLAGS_dump_malloc_stats; + options.stats_dump_period_sec = + static_cast(FLAGS_stats_dump_period_sec); + options.stats_persist_period_sec = + static_cast(FLAGS_stats_persist_period_sec); + options.persist_stats_to_disk = FLAGS_persist_stats_to_disk; + options.stats_history_buffer_size = + static_cast(FLAGS_stats_history_buffer_size); + options.avoid_flush_during_recovery = FLAGS_avoid_flush_during_recovery; + + options.compression_opts.level = FLAGS_compression_level; + options.compression_opts.max_dict_bytes = FLAGS_compression_max_dict_bytes; + options.compression_opts.zstd_max_train_bytes = + FLAGS_compression_zstd_max_train_bytes; + options.compression_opts.parallel_threads = + FLAGS_compression_parallel_threads; + options.compression_opts.max_dict_buffer_bytes = + FLAGS_compression_max_dict_buffer_bytes; + options.max_open_files = FLAGS_open_files; if (FLAGS_cost_write_buffer_to_cache || FLAGS_db_write_buffer_size != 0) { options.write_buffer_manager.reset( @@ -4285,94 +4305,104 @@ class Benchmark { } void InitializeOptionsGeneral(Options* opts) { + // Be careful about what is set here to avoid accidentally overwriting + // settings already configured by OPTIONS file. Only configure settings that + // are needed for the benchmark to run, settings for shared objects that + // were not configured already, settings that require dynamically invoking + // APIs, and settings for the benchmark itself. Options& options = *opts; - options.create_missing_column_families = FLAGS_num_column_families > 1; - options.statistics = dbstats; - options.wal_dir = FLAGS_wal_dir; - options.create_if_missing = !FLAGS_use_existing_db; - options.dump_malloc_stats = FLAGS_dump_malloc_stats; - options.stats_dump_period_sec = - static_cast(FLAGS_stats_dump_period_sec); - options.stats_persist_period_sec = - static_cast(FLAGS_stats_persist_period_sec); - options.persist_stats_to_disk = FLAGS_persist_stats_to_disk; - options.stats_history_buffer_size = - static_cast(FLAGS_stats_history_buffer_size); - options.avoid_flush_during_recovery = FLAGS_avoid_flush_during_recovery; + // Always set these since they are harmless when not needed and prevent + // a guaranteed failure when they are needed. + options.create_missing_column_families = true; + options.create_if_missing = true; + + if (options.statistics == nullptr) { + options.statistics = dbstats; + } - options.compression_opts.level = FLAGS_compression_level; - options.compression_opts.max_dict_bytes = FLAGS_compression_max_dict_bytes; - options.compression_opts.zstd_max_train_bytes = - FLAGS_compression_zstd_max_train_bytes; - options.compression_opts.parallel_threads = - FLAGS_compression_parallel_threads; - options.compression_opts.max_dict_buffer_bytes = - FLAGS_compression_max_dict_buffer_bytes; - // If this is a block based table, set some related options auto table_options = options.table_factory->GetOptions(); if (table_options != nullptr) { - if (FLAGS_cache_size) { + if (FLAGS_cache_size > 0) { + // This violates this function's rules on when to set options. But we + // have to do it because the case of unconfigured block cache in OPTIONS + // file is indistinguishable (it is sanitized to 8MB by this point, not + // nullptr), and our regression tests assume this will be the shared + // block cache, even with OPTIONS file provided. table_options->block_cache = cache_; } - if (FLAGS_bloom_bits < 0) { - table_options->filter_policy = BlockBasedTableOptions().filter_policy; - } else if (FLAGS_bloom_bits == 0) { - table_options->filter_policy.reset(); - } else if (FLAGS_use_block_based_filter) { - // Use back-door way of enabling obsolete block-based Bloom - Status s = FilterPolicy::CreateFromString( - ConfigOptions(), - "rocksdb.internal.DeprecatedBlockBasedBloomFilter:" + - ROCKSDB_NAMESPACE::ToString(FLAGS_bloom_bits), - &table_options->filter_policy); - if (!s.ok()) { - fprintf(stderr, "failure creating obsolete block-based filter: %s\n", - s.ToString().c_str()); - exit(1); + if (table_options->filter_policy == nullptr) { + if (FLAGS_bloom_bits < 0) { + table_options->filter_policy = BlockBasedTableOptions().filter_policy; + } else if (FLAGS_bloom_bits == 0) { + table_options->filter_policy.reset(); + } else if (FLAGS_use_block_based_filter) { + // Use back-door way of enabling obsolete block-based Bloom + Status s = FilterPolicy::CreateFromString( + ConfigOptions(), + "rocksdb.internal.DeprecatedBlockBasedBloomFilter:" + + ROCKSDB_NAMESPACE::ToString(FLAGS_bloom_bits), + &table_options->filter_policy); + if (!s.ok()) { + fprintf(stderr, + "failure creating obsolete block-based filter: %s\n", + s.ToString().c_str()); + exit(1); + } + } else { + table_options->filter_policy.reset( + FLAGS_use_ribbon_filter ? NewRibbonFilterPolicy(FLAGS_bloom_bits) + : NewBloomFilterPolicy(FLAGS_bloom_bits)); } - } else { - table_options->filter_policy.reset( - FLAGS_use_ribbon_filter ? NewRibbonFilterPolicy(FLAGS_bloom_bits) - : NewBloomFilterPolicy(FLAGS_bloom_bits)); } } - if (FLAGS_row_cache_size) { - if (FLAGS_cache_numshardbits >= 1) { - options.row_cache = - NewLRUCache(FLAGS_row_cache_size, FLAGS_cache_numshardbits); - } else { - options.row_cache = NewLRUCache(FLAGS_row_cache_size); + + if (options.row_cache == nullptr) { + if (FLAGS_row_cache_size) { + if (FLAGS_cache_numshardbits >= 1) { + options.row_cache = + NewLRUCache(FLAGS_row_cache_size, FLAGS_cache_numshardbits); + } else { + options.row_cache = NewLRUCache(FLAGS_row_cache_size); + } } } + + if (options.env == Env::Default()) { + options.env = FLAGS_env; + } if (FLAGS_enable_io_prio) { - FLAGS_env->LowerThreadPoolIOPriority(Env::LOW); - FLAGS_env->LowerThreadPoolIOPriority(Env::HIGH); + options.env->LowerThreadPoolIOPriority(Env::LOW); + options.env->LowerThreadPoolIOPriority(Env::HIGH); } if (FLAGS_enable_cpu_prio) { - FLAGS_env->LowerThreadPoolCPUPriority(Env::LOW); - FLAGS_env->LowerThreadPoolCPUPriority(Env::HIGH); + options.env->LowerThreadPoolCPUPriority(Env::LOW); + options.env->LowerThreadPoolCPUPriority(Env::HIGH); } - options.env = FLAGS_env; + if (FLAGS_sine_write_rate) { FLAGS_benchmark_write_rate_limit = static_cast(SineRate(0)); } - if (FLAGS_rate_limiter_bytes_per_sec > 0) { - options.rate_limiter.reset(NewGenericRateLimiter( - FLAGS_rate_limiter_bytes_per_sec, FLAGS_rate_limiter_refill_period_us, - 10 /* fairness */, - FLAGS_rate_limit_bg_reads ? RateLimiter::Mode::kReadsOnly - : RateLimiter::Mode::kWritesOnly, - FLAGS_rate_limiter_auto_tuned)); + if (options.rate_limiter == nullptr) { + if (FLAGS_rate_limiter_bytes_per_sec > 0) { + options.rate_limiter.reset(NewGenericRateLimiter( + FLAGS_rate_limiter_bytes_per_sec, + FLAGS_rate_limiter_refill_period_us, 10 /* fairness */, + FLAGS_rate_limit_bg_reads ? RateLimiter::Mode::kReadsOnly + : RateLimiter::Mode::kWritesOnly, + FLAGS_rate_limiter_auto_tuned)); + } } options.listeners.emplace_back(listener_); - if (FLAGS_file_checksum) { - options.file_checksum_gen_factory.reset( - new FileChecksumGenCrc32cFactory()); + if (options.file_checksum_gen_factory == nullptr) { + if (FLAGS_file_checksum) { + options.file_checksum_gen_factory.reset( + new FileChecksumGenCrc32cFactory()); + } } if (FLAGS_num_multi_db <= 1) { @@ -4391,9 +4421,11 @@ class Benchmark { } // KeepFilter is a noop filter, this can be used to test compaction filter - if (FLAGS_use_keep_filter) { - options.compaction_filter = new KeepFilter(); - fprintf(stdout, "A noop compaction filter is used\n"); + if (options.compaction_filter == nullptr) { + if (FLAGS_use_keep_filter) { + options.compaction_filter = new KeepFilter(); + fprintf(stdout, "A noop compaction filter is used\n"); + } } if (FLAGS_use_existing_keys) {