From c92e7a5ba71109e1b4dce6c754bbb487bbb2eb4f Mon Sep 17 00:00:00 2001 From: mrambacher Date: Tue, 13 Dec 2022 10:40:38 -0500 Subject: [PATCH 01/10] Add RefreshOptions method Allows for configuration changes to be updated by reading an Options file periodically. --- db/db_impl/db_impl.cc | 102 +++++++++++++++++++++++++++++++++- db/db_impl/db_impl.h | 11 +++- db/periodic_task_scheduler.cc | 2 + db/periodic_task_scheduler.h | 1 + include/rocksdb/options.h | 7 +++ options/db_options.cc | 17 ++++++ options/db_options.h | 2 + 7 files changed, 140 insertions(+), 2 deletions(-) diff --git a/db/db_impl/db_impl.cc b/db/db_impl/db_impl.cc index 9fcf3c9a5f..5735f78464 100644 --- a/db/db_impl/db_impl.cc +++ b/db/db_impl/db_impl.cc @@ -276,6 +276,8 @@ DBImpl::DBImpl(const DBOptions& options, const std::string& dbname, periodic_task_functions_.emplace( PeriodicTaskType::kRecordSeqnoTime, [this]() { this->RecordSeqnoToTimeMapping(); }); + periodic_task_functions_.emplace(PeriodicTaskType::kRefreshOptions, + [this]() { this->RefreshOptions(); }); #endif // ROCKSDB_LITE versions_.reset(new VersionSet(dbname_, &immutable_db_options_, file_options_, @@ -829,6 +831,15 @@ Status DBImpl::StartPeriodicTaskScheduler() { return s; } } + if (mutable_db_options_.refresh_options_sec > 0) { + Status s = periodic_task_scheduler_.Register( + PeriodicTaskType::kRefreshOptions, + periodic_task_functions_.at(PeriodicTaskType::kRefreshOptions), + mutable_db_options_.refresh_options_sec); + if (!s.ok()) { + return s; + } + } Status s = periodic_task_scheduler_.Register( PeriodicTaskType::kFlushInfoLog, @@ -1127,6 +1138,76 @@ void DBImpl::FlushInfoLog() { LogFlush(immutable_db_options_.info_log); } +#ifndef ROCKSDB_LITE +// Periodically checks to see if the new options should be loaded into the +// process. log. +void DBImpl::RefreshOptions() { + if (shutdown_initiated_) { + return; + } + std::string new_options_file = mutable_db_options_.refresh_options_file; + if (new_options_file.empty()) { + new_options_file = "Options.new"; + } + if (new_options_file[0] != kFilePathSeparator) { + new_options_file = NormalizePath(immutable_db_options_.db_paths[0].path + + kFilePathSeparator + new_options_file); + } + TEST_SYNC_POINT("DBImpl::RefreshOptions::Start"); + Status s = fs_->FileExists(new_options_file, IOOptions(), nullptr); + if (!s.ok()) { + return; + } + ROCKS_LOG_INFO(immutable_db_options_.info_log, + "Refreshing Options from file: %s\n", + new_options_file.c_str()); + + ConfigOptions cfg_opts; + cfg_opts.ignore_unknown_options = true; + cfg_opts.mutable_options_only = true; + RocksDBOptionsParser op; + s = op.Parse(cfg_opts, new_options_file, fs_.get()); + if (!s.ok()) { + ROCKS_LOG_WARN(immutable_db_options_.info_log, + "Failed to parse Options file (%s): %s\n", + new_options_file.c_str(), s.ToString().c_str()); + } else if (!op.db_opt_map()->empty()) { + s = SetDBOptions(*(op.db_opt_map())); + if (!s.ok()) { + ROCKS_LOG_WARN(immutable_db_options_.info_log, + "Failed to refresh DBOptions, Aborting: %s\n", + s.ToString().c_str()); + } + } + if (s.ok()) { + int idx = 0; + for (const auto& cf_opt_map : *(op.cf_opt_maps())) { + if (!cf_opt_map.empty()) { + const auto& cf_name = (*op.cf_names())[idx]; + auto cfd = versions_->GetColumnFamilySet()->GetColumnFamily(cf_name); + if (cfd == nullptr) { + ROCKS_LOG_WARN(immutable_db_options_.info_log, + "RefreshOptions failed locating CF: %s\n", + cf_name.c_str()); + } else if (!cfd->IsDropped()) { + s = SetCFOptionsImpl(cfd, cf_opt_map); + if (!s.ok()) { + ROCKS_LOG_WARN(immutable_db_options_.info_log, + "Failed to refresh CFOptions for CF %s: %s\n", + cf_name.c_str(), s.ToString().c_str()); + } + } + } + idx++; + } + } + s = fs_->DeleteFile(new_options_file, IOOptions(), nullptr); + ROCKS_LOG_INFO(immutable_db_options_.info_log, + "RefreshOptions Complete, deleting options file %s: %s\n", + new_options_file.c_str(), s.ToString().c_str()); +} +#endif // ROCKSDB_LITE + Status DBImpl::TablesRangeTombstoneSummary(ColumnFamilyHandle* column_family, int max_entries_to_print, std::string* out_str) { @@ -1178,7 +1259,14 @@ Status DBImpl::SetOptions( cfd->GetName().c_str()); return Status::InvalidArgument("empty input"); } + return SetCFOptionsImpl(cfd, options_map); +#endif // ROCKSDB_LITE +} +#ifndef ROCKSDB_LITE +Status DBImpl::SetCFOptionsImpl( + ColumnFamilyData* cfd, + const std::unordered_map& options_map) { MutableCFOptions new_options; Status s; Status persist_options_status; @@ -1227,8 +1315,8 @@ Status DBImpl::SetOptions( } LogFlush(immutable_db_options_.info_log); return s; -#endif // ROCKSDB_LITE } +#endif // ROCKSDB_LITE Status DBImpl::SetDBOptions( const std::unordered_map& options_map) { @@ -1336,6 +1424,18 @@ Status DBImpl::SetDBOptions( new_options.stats_persist_period_sec); } } + if (s.ok()) { + if (new_options.refresh_options_sec == 0) { + s = periodic_task_scheduler_.Unregister( + PeriodicTaskType::kRefreshOptions); + } else { + s = periodic_task_scheduler_.Register( + PeriodicTaskType::kRefreshOptions, + periodic_task_functions_.at(PeriodicTaskType::kRefreshOptions), + new_options.refresh_options_sec); + } + } + mutex_.Lock(); if (!s.ok()) { return s; diff --git a/db/db_impl/db_impl.h b/db/db_impl/db_impl.h index 03418c1d50..71796bcd67 100644 --- a/db/db_impl/db_impl.h +++ b/db/db_impl/db_impl.h @@ -1183,6 +1183,11 @@ class DBImpl : public DB { // record current sequence number to time mapping void RecordSeqnoToTimeMapping(); +#ifndef ROCKSDB_LITE + // Checks if the options should be updated + void RefreshOptions(); +#endif // ROCKSDB_LITE + // Interface to block and signal the DB in case of stalling writes by // WriteBufferManager. Each DBImpl object contains ptr to WBMStallInterface. // When DB needs to be blocked or signalled by WriteBufferManager, @@ -1774,7 +1779,11 @@ class DBImpl : public DB { ColumnFamilyHandle** handle); Status DropColumnFamilyImpl(ColumnFamilyHandle* column_family); - +#ifndef ROCKSDB_LITE + Status SetCFOptionsImpl( + ColumnFamilyData* cfd, + const std::unordered_map& options_map); +#endif // ROCKSDB_LITE // Delete any unneeded files and stale in-memory entries. void DeleteObsoleteFiles(); // Delete obsolete files and log status and information of file deletion diff --git a/db/periodic_task_scheduler.cc b/db/periodic_task_scheduler.cc index 2024510dd6..90d1e4967e 100644 --- a/db/periodic_task_scheduler.cc +++ b/db/periodic_task_scheduler.cc @@ -27,6 +27,7 @@ static const std::map kDefaultPeriodSeconds = { {PeriodicTaskType::kPersistStats, kInvalidPeriodSec}, {PeriodicTaskType::kFlushInfoLog, 10}, {PeriodicTaskType::kRecordSeqnoTime, kInvalidPeriodSec}, + {PeriodicTaskType::kRefreshOptions, kInvalidPeriodSec}, }; static const std::map kPeriodicTaskTypeNames = { @@ -34,6 +35,7 @@ static const std::map kPeriodicTaskTypeNames = { {PeriodicTaskType::kPersistStats, "pst_st"}, {PeriodicTaskType::kFlushInfoLog, "flush_info_log"}, {PeriodicTaskType::kRecordSeqnoTime, "record_seq_time"}, + {PeriodicTaskType::kRefreshOptions, "refresh_options"}, }; Status PeriodicTaskScheduler::Register(PeriodicTaskType task_type, diff --git a/db/periodic_task_scheduler.h b/db/periodic_task_scheduler.h index f45b80c4d8..d5c5eec53b 100644 --- a/db/periodic_task_scheduler.h +++ b/db/periodic_task_scheduler.h @@ -23,6 +23,7 @@ enum class PeriodicTaskType : uint8_t { kPersistStats, kFlushInfoLog, kRecordSeqnoTime, + kRefreshOptions, kMax, }; diff --git a/include/rocksdb/options.h b/include/rocksdb/options.h index acbc991677..eef770660a 100644 --- a/include/rocksdb/options.h +++ b/include/rocksdb/options.h @@ -1394,6 +1394,13 @@ struct DBOptions { // of the contract leads to undefined behaviors with high possibility of data // inconsistency, e.g. deleted old data become visible again, etc. bool enforce_single_del_contracts = true; + + // If non-zero, a task will be started to check for a new + // "refresh_options_file" If found, the refresh task will update the mutable + // options from the settings in this file + // Not supported in ROCKSDB_LITE mode! + unsigned int refresh_options_sec = 0; + std::string refresh_options_file; }; // Options to control the behavior of a database (passed to DB::Open) diff --git a/options/db_options.cc b/options/db_options.cc index 840ade7ea7..cb15d7f104 100644 --- a/options/db_options.cc +++ b/options/db_options.cc @@ -102,6 +102,14 @@ static std::unordered_map {offsetof(struct MutableDBOptions, stats_persist_period_sec), OptionType::kUInt, OptionVerificationType::kNormal, OptionTypeFlags::kMutable}}, + {"refresh_options_sec", + {offsetof(struct MutableDBOptions, refresh_options_sec), + OptionType::kUInt, OptionVerificationType::kNormal, + OptionTypeFlags::kMutable}}, + {"refresh_options_file", + {offsetof(struct MutableDBOptions, refresh_options_file), + OptionType::kString, OptionVerificationType::kNormal, + OptionTypeFlags::kMutable}}, {"stats_history_buffer_size", {offsetof(struct MutableDBOptions, stats_history_buffer_size), OptionType::kSizeT, OptionVerificationType::kNormal, @@ -982,6 +990,7 @@ MutableDBOptions::MutableDBOptions() delete_obsolete_files_period_micros(6ULL * 60 * 60 * 1000000), stats_dump_period_sec(600), stats_persist_period_sec(600), + refresh_options_sec(0), stats_history_buffer_size(1024 * 1024), max_open_files(-1), bytes_per_sync(0), @@ -1002,6 +1011,8 @@ MutableDBOptions::MutableDBOptions(const DBOptions& options) options.delete_obsolete_files_period_micros), stats_dump_period_sec(options.stats_dump_period_sec), stats_persist_period_sec(options.stats_persist_period_sec), + refresh_options_sec(options.refresh_options_sec), + refresh_options_file(options.refresh_options_file), stats_history_buffer_size(options.stats_history_buffer_size), max_open_files(options.max_open_files), bytes_per_sync(options.bytes_per_sync), @@ -1033,6 +1044,12 @@ void MutableDBOptions::Dump(Logger* log) const { stats_dump_period_sec); ROCKS_LOG_HEADER(log, " Options.stats_persist_period_sec: %d", stats_persist_period_sec); + ROCKS_LOG_HEADER(log, " Options.refresh_options_sec: %d", + refresh_options_sec); + if (refresh_options_sec > 0 && !refresh_options_file.empty()) { + ROCKS_LOG_HEADER(log, " Options.refresh_options_file: %s", + refresh_options_file.c_str()); + } ROCKS_LOG_HEADER( log, " Options.stats_history_buffer_size: %" ROCKSDB_PRIszt, diff --git a/options/db_options.h b/options/db_options.h index 8946f60ff4..d336f2679a 100644 --- a/options/db_options.h +++ b/options/db_options.h @@ -130,6 +130,8 @@ struct MutableDBOptions { uint64_t delete_obsolete_files_period_micros; unsigned int stats_dump_period_sec; unsigned int stats_persist_period_sec; + unsigned int refresh_options_sec; + std::string refresh_options_file; size_t stats_history_buffer_size; int max_open_files; uint64_t bytes_per_sync; From 1bffd395ffd891307ebc260987b1941f49613353 Mon Sep 17 00:00:00 2001 From: mrambacher Date: Mon, 19 Dec 2022 14:55:51 -0500 Subject: [PATCH 02/10] Address PR Comments - Add command line flag to db_stress_tool and db_bench - Set the default to check for options updates every hour (3600 seconds). --- db_stress_tool/db_stress_common.h | 2 ++ db_stress_tool/db_stress_gflags.cc | 6 ++++++ db_stress_tool/db_stress_test_base.cc | 4 ++++ include/rocksdb/options.h | 3 ++- tools/db_bench_tool.cc | 8 ++++++++ 5 files changed, 22 insertions(+), 1 deletion(-) diff --git a/db_stress_tool/db_stress_common.h b/db_stress_tool/db_stress_common.h index 2ef3adc52c..e91e47eb8e 100644 --- a/db_stress_tool/db_stress_common.h +++ b/db_stress_tool/db_stress_common.h @@ -302,6 +302,8 @@ DECLARE_bool(two_write_queues); DECLARE_bool(use_only_the_last_commit_time_batch_for_recovery); DECLARE_uint64(wp_snapshot_cache_bits); DECLARE_uint64(wp_commit_cache_bits); +DECLARE_int32(refresh_options_sec); +DECLARE_string(refresh_options_file); #endif // !ROCKSDB_LITE DECLARE_bool(adaptive_readahead); diff --git a/db_stress_tool/db_stress_gflags.cc b/db_stress_tool/db_stress_gflags.cc index 9a4aad2ad2..0f71f6a76e 100644 --- a/db_stress_tool/db_stress_gflags.cc +++ b/db_stress_tool/db_stress_gflags.cc @@ -1010,6 +1010,12 @@ DEFINE_uint64( DEFINE_uint64(wp_commit_cache_bits, 23ull, "Number of bits to represent write-prepared transaction db's " "commit cache. Default: 23 (8M entries)"); +DEFINE_int32( + refresh_options_sec, 0, + "Frequency (in secs) to look for a new options file (off by default)"); +DEFINE_string(refresh_options_file, "", + "File in which to look for new options"); + #endif // !ROCKSDB_LITE DEFINE_bool(adaptive_readahead, false, diff --git a/db_stress_tool/db_stress_test_base.cc b/db_stress_tool/db_stress_test_base.cc index 3153658945..f0379d10e1 100644 --- a/db_stress_tool/db_stress_test_base.cc +++ b/db_stress_tool/db_stress_test_base.cc @@ -3126,6 +3126,10 @@ void InitializeOptionsFromFlags( FLAGS_verify_sst_unique_id_in_manifest; options.memtable_protection_bytes_per_key = FLAGS_memtable_protection_bytes_per_key; +#ifndef ROCKSDB_LITE + options.refresh_options_sec = FLAGS_refresh_options_sec; + options.refresh_options_file = FLAGS_refresh_options_file; +#endif // ROCKSDB_LITE // Integrated BlobDB options.enable_blob_files = FLAGS_enable_blob_files; diff --git a/include/rocksdb/options.h b/include/rocksdb/options.h index eef770660a..b1a7fe796c 100644 --- a/include/rocksdb/options.h +++ b/include/rocksdb/options.h @@ -1398,8 +1398,9 @@ struct DBOptions { // If non-zero, a task will be started to check for a new // "refresh_options_file" If found, the refresh task will update the mutable // options from the settings in this file + // Defaults to check once per hour. Set to 0 to disable the task. // Not supported in ROCKSDB_LITE mode! - unsigned int refresh_options_sec = 0; + unsigned int refresh_options_sec = 60 * 60; std::string refresh_options_file; }; diff --git a/tools/db_bench_tool.cc b/tools/db_bench_tool.cc index fe8f526aaf..9900fff2a2 100644 --- a/tools/db_bench_tool.cc +++ b/tools/db_bench_tool.cc @@ -1460,6 +1460,12 @@ DEFINE_int32(table_cache_numshardbits, 4, ""); DEFINE_string(filter_uri, "", "URI for registry FilterPolicy"); #ifndef ROCKSDB_LITE +DEFINE_int32( + refresh_options_sec, 0, + "Frequency (in secs) to look for a new options file (off by default)"); +DEFINE_string(refresh_options_file, "", + "File in which to look for new options"); + DEFINE_string(env_uri, "", "URI for registry Env lookup. Mutually exclusive with --fs_uri"); DEFINE_string(fs_uri, "", @@ -4439,6 +4445,8 @@ class Benchmark { options.manual_wal_flush = FLAGS_manual_wal_flush; options.wal_compression = FLAGS_wal_compression_e; #ifndef ROCKSDB_LITE + options.refresh_options_sec = FLAGS_refresh_options_sec; + options.refresh_options_file = FLAGS_refresh_options_file; options.ttl = FLAGS_fifo_compaction_ttl; options.compaction_options_fifo = CompactionOptionsFIFO( FLAGS_fifo_compaction_max_table_files_size_mb * 1024 * 1024, From 6b648d4db0cd4d02b0982a6926938e5b2dcd0b10 Mon Sep 17 00:00:00 2001 From: mrambacher Date: Tue, 20 Dec 2022 09:59:15 -0500 Subject: [PATCH 03/10] First round of unit tests... --- db/db_impl/db_impl.cc | 6 ++++++ db/db_options_test.cc | 36 ++++++++++++++++++++++++++++++++++++ options/options_helper.cc | 2 ++ 3 files changed, 44 insertions(+) diff --git a/db/db_impl/db_impl.cc b/db/db_impl/db_impl.cc index 5735f78464..25aee198fb 100644 --- a/db/db_impl/db_impl.cc +++ b/db/db_impl/db_impl.cc @@ -1155,6 +1155,7 @@ void DBImpl::RefreshOptions() { } TEST_SYNC_POINT("DBImpl::RefreshOptions::Start"); Status s = fs_->FileExists(new_options_file, IOOptions(), nullptr); + TEST_SYNC_POINT_CALLBACK("DBImpl::RefreshOptions::FileExists", &s); if (!s.ok()) { return; } @@ -1167,12 +1168,14 @@ void DBImpl::RefreshOptions() { cfg_opts.mutable_options_only = true; RocksDBOptionsParser op; s = op.Parse(cfg_opts, new_options_file, fs_.get()); + TEST_SYNC_POINT_CALLBACK("DBImpl::RefreshOptions::Parse", &s); if (!s.ok()) { ROCKS_LOG_WARN(immutable_db_options_.info_log, "Failed to parse Options file (%s): %s\n", new_options_file.c_str(), s.ToString().c_str()); } else if (!op.db_opt_map()->empty()) { s = SetDBOptions(*(op.db_opt_map())); + TEST_SYNC_POINT_CALLBACK("DBImpl::RefreshOptions::SetDBOptions", &s); if (!s.ok()) { ROCKS_LOG_WARN(immutable_db_options_.info_log, "Failed to refresh DBOptions, Aborting: %s\n", @@ -1191,6 +1194,7 @@ void DBImpl::RefreshOptions() { cf_name.c_str()); } else if (!cfd->IsDropped()) { s = SetCFOptionsImpl(cfd, cf_opt_map); + TEST_SYNC_POINT_CALLBACK("DBImpl::RefreshOptions::SetCFOptions", &s); if (!s.ok()) { ROCKS_LOG_WARN(immutable_db_options_.info_log, "Failed to refresh CFOptions for CF %s: %s\n", @@ -1202,9 +1206,11 @@ void DBImpl::RefreshOptions() { } } s = fs_->DeleteFile(new_options_file, IOOptions(), nullptr); + TEST_SYNC_POINT_CALLBACK("DBImpl::RefreshOptions::DeleteFile", &s); ROCKS_LOG_INFO(immutable_db_options_.info_log, "RefreshOptions Complete, deleting options file %s: %s\n", new_options_file.c_str(), s.ToString().c_str()); + TEST_SYNC_POINT("DBImpl::RefreshOptions::Complete"); } #endif // ROCKSDB_LITE diff --git a/db/db_options_test.cc b/db/db_options_test.cc index 212b8e8124..09ddcc32f6 100644 --- a/db/db_options_test.cc +++ b/db/db_options_test.cc @@ -14,6 +14,7 @@ #include "db/db_impl/db_impl.h" #include "db/db_test_util.h" #include "options/options_helper.h" +#include "options/options_parser.h" #include "port/stack_trace.h" #include "rocksdb/cache.h" #include "rocksdb/convenience.h" @@ -1151,6 +1152,41 @@ TEST_F(DBOptionsTest, ChangeCompression) { SyncPoint::GetInstance()->DisableProcessing(); } +TEST_F(DBOptionsTest, RefreshOptions) { + std::string test_path; + Options options = CurrentOptions(); + auto fs = options.env->GetFileSystem(); + options.create_if_missing = true; + options.refresh_options_sec = 1; + options.refresh_options_file = dbname_ + "/Options.New"; + options.max_background_jobs = 1; + options.max_background_compactions = 2; + options.periodic_compaction_seconds = 100; + ASSERT_OK(TryReopen(options)); + SyncPoint::GetInstance()->LoadDependency( + {{"DBImpl::RefreshOptions::Complete", "DBOptionsTest::WaitForUpdates"}}); + SyncPoint::GetInstance()->EnableProcessing(); + ConfigOptions config_options; + config_options.mutable_options_only = true; + options.max_background_jobs = 10; + options.max_background_compactions = 20; + options.periodic_compaction_seconds = 200; + ASSERT_OK(PersistRocksDBOptions(config_options, options, {"default"}, + {options}, options.refresh_options_file, + fs.get())); + + TEST_SYNC_POINT("DBOptionsTest::WaitForUpdates"); + DBOptions new_db_opts = db_->GetDBOptions(); + ASSERT_EQ(new_db_opts.max_background_jobs, 10); + ASSERT_EQ(new_db_opts.max_background_compactions, 20); + auto dcfh = db_->DefaultColumnFamily(); + ColumnFamilyDescriptor dcd; + ASSERT_OK(dcfh->GetDescriptor(&dcd)); + ASSERT_EQ(dcd.options.periodic_compaction_seconds, 200); + + SyncPoint::GetInstance()->DisableProcessing(); +} + #endif // ROCKSDB_LITE TEST_F(DBOptionsTest, BottommostCompressionOptsWithFallbackType) { diff --git a/options/options_helper.cc b/options/options_helper.cc index efb1d382e8..60cc6bda1e 100644 --- a/options/options_helper.cc +++ b/options/options_helper.cc @@ -186,6 +186,8 @@ DBOptions BuildDBOptions(const ImmutableDBOptions& immutable_db_options, options.lowest_used_cache_tier = immutable_db_options.lowest_used_cache_tier; options.enforce_single_del_contracts = immutable_db_options.enforce_single_del_contracts; + options.refresh_options_sec = mutable_db_options.refresh_options_sec; + options.refresh_options_file = mutable_db_options.refresh_options_file; return options; } From 916c5a2ea10c9d296e7fdc59a1e72e2147b8b269 Mon Sep 17 00:00:00 2001 From: mrambacher Date: Wed, 21 Dec 2022 15:55:25 -0500 Subject: [PATCH 04/10] Add more unit tests --- db/db_options_test.cc | 145 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 145 insertions(+) diff --git a/db/db_options_test.cc b/db/db_options_test.cc index e0e975857f..31f1a297e4 100644 --- a/db/db_options_test.cc +++ b/db/db_options_test.cc @@ -13,6 +13,7 @@ #include "db/column_family.h" #include "db/db_impl/db_impl.h" #include "db/db_test_util.h" +#include "env/mock_env.h" #include "options/options_helper.h" #include "options/options_parser.h" #include "port/stack_trace.h" @@ -1177,6 +1178,11 @@ TEST_F(DBOptionsTest, RefreshOptions) { fs.get())); TEST_SYNC_POINT("DBOptionsTest::WaitForUpdates"); + SyncPoint::GetInstance()->DisableProcessing(); + SyncPoint::GetInstance()->LoadDependency( + {{"DBImpl::RefreshOptions::Complete", "DBOptionsTest::WaitForUpdates"}}); + SyncPoint::GetInstance()->EnableProcessing(); + DBOptions new_db_opts = db_->GetDBOptions(); ASSERT_EQ(new_db_opts.max_background_jobs, 10); ASSERT_EQ(new_db_opts.max_background_compactions, 20); @@ -1188,6 +1194,145 @@ TEST_F(DBOptionsTest, RefreshOptions) { SyncPoint::GetInstance()->DisableProcessing(); } +TEST_F(DBOptionsTest, RefreshOptionsImmutable) { + std::string test_path; + Options options = CurrentOptions(); + auto fs = options.env->GetFileSystem(); + options.create_if_missing = true; + options.refresh_options_sec = 1; + options.refresh_options_file = dbname_ + "/Options.New"; + ASSERT_OK(TryReopen(options)); + SyncPoint::GetInstance()->LoadDependency( + {{"DBImpl::RefreshOptions::Complete", "DBOptionsTest::WaitForUpdates"}}); + SyncPoint::GetInstance()->EnableProcessing(); + ConfigOptions config_options; + + // Test setting an immutable DBOption and see the value + // did not change + std::unique_ptr mock(MockEnv::Create(options.env)); + options.env = mock.get(); + ASSERT_OK(PersistRocksDBOptions(config_options, options, {"default"}, + {options}, options.refresh_options_file, + fs.get())); + TEST_SYNC_POINT("DBOptionsTest::WaitForUpdates"); + + SyncPoint::GetInstance()->DisableProcessing(); + SyncPoint::GetInstance()->LoadDependency( + {{"DBImpl::RefreshOptions::Complete", "DBOptionsTest::WaitForUpdates"}}); + SyncPoint::GetInstance()->EnableProcessing(); + + // Test setting an immutable ColumnFamilyOption and see the value + // did not change + options = CurrentOptions(); + options.comparator = ReverseBytewiseComparator(); + options.refresh_options_sec = 1; + options.refresh_options_file = dbname_ + "/Options.New"; + ASSERT_OK(PersistRocksDBOptions(config_options, options, {"default"}, + {options}, options.refresh_options_file, + fs.get())); + TEST_SYNC_POINT("DBOptionsTest::WaitForUpdates"); + auto dcfh = db_->DefaultColumnFamily(); + ColumnFamilyDescriptor dcd; + ASSERT_OK(dcfh->GetDescriptor(&dcd)); + ASSERT_EQ(dcd.options.comparator, BytewiseComparator()); + + SyncPoint::GetInstance()->DisableProcessing(); +} + +TEST_F(DBOptionsTest, RefreshOptionsBadFile) { + std::string test_path; + Options options = CurrentOptions(); + auto fs = options.env->GetFileSystem(); + options.create_if_missing = true; + options.refresh_options_sec = 1; + options.refresh_options_file = dbname_ + "/Options.New"; + ASSERT_OK(TryReopen(options)); + + SyncPoint::GetInstance()->LoadDependency( + {{"DBImpl::RefreshOptions::Complete", "DBOptionsTest::WaitForUpdates"}}); + SyncPoint::GetInstance()->SetCallBack("DBImpl::RefreshOptions::Parse", + [&](void* arg) { + auto s = static_cast(arg); + ASSERT_NOK(*s); + }); + SyncPoint::GetInstance()->EnableProcessing(); + // Test with a file that is not an options file + ASSERT_OK(CreateFile(fs, options.refresh_options_file, "fred", false)); + TEST_SYNC_POINT("DBOptionsTest::WaitForUpdates"); + SyncPoint::GetInstance()->DisableProcessing(); + SyncPoint::GetInstance()->LoadDependency( + {{"DBImpl::RefreshOptions::Complete", "DBOptionsTest::WaitForUpdates"}}); + SyncPoint::GetInstance()->EnableProcessing(); + + // Test with a file that contains no DBOptions section + ASSERT_OK(CreateFile(fs, options.refresh_options_file, + "[CFOptions \"default\"]\n", false)); + TEST_SYNC_POINT("DBOptionsTest::WaitForUpdates"); + SyncPoint::GetInstance()->DisableProcessing(); + SyncPoint::GetInstance()->LoadDependency( + {{"DBImpl::RefreshOptions::Complete", "DBOptionsTest::WaitForUpdates"}}); + SyncPoint::GetInstance()->EnableProcessing(); + + // Test with a file that contains no ColumnFamilyOptions section + ASSERT_OK( + CreateFile(fs, options.refresh_options_file, "[DBOptions]\n", false)); + TEST_SYNC_POINT("DBOptionsTest::WaitForUpdates"); + SyncPoint::GetInstance()->DisableProcessing(); + SyncPoint::GetInstance()->LoadDependency( + {{"DBImpl::RefreshOptions::Complete", "DBOptionsTest::WaitForUpdates"}}); + SyncPoint::GetInstance()->EnableProcessing(); + + // Test with a file that contains no default ColumnFamilyOptions section + ASSERT_OK(CreateFile(fs, options.refresh_options_file, + "[DBOptions]\n" + "[CFOptions \"unknown\"]\n", + false)); + TEST_SYNC_POINT("DBOptionsTest::WaitForUpdates"); + + SyncPoint::GetInstance()->DisableProcessing(); +} + +TEST_F(DBOptionsTest, RefreshOptionsUnknown) { + std::string test_path; + Options options = CurrentOptions(); + auto fs = options.env->GetFileSystem(); + options.create_if_missing = true; + options.refresh_options_sec = 1; + options.refresh_options_file = dbname_ + "/Options.New"; + ASSERT_OK(TryReopen(options)); + SyncPoint::GetInstance()->LoadDependency( + {{"DBImpl::RefreshOptions::Complete", "DBOptionsTest::WaitForUpdates"}}); + SyncPoint::GetInstance()->SetCallBack("DBImpl::RefreshOptions::SetDBOptions", + [&](void* arg) { + auto s = static_cast(arg); + ASSERT_NOK(*s); + }); + SyncPoint::GetInstance()->SetCallBack("DBImpl::RefreshOptions::SetCFOptions", + [&](void* arg) { + auto s = static_cast(arg); + ASSERT_NOK(*s); + }); + SyncPoint::GetInstance()->EnableProcessing(); + // Test with a file that contains a bad DBOptions value + + ASSERT_OK(CreateFile(fs, options.refresh_options_file, + "[DBOptions]\n" + "unknown = value\n" + "[CFOptions \"default\"]\n", + false)); + TEST_SYNC_POINT("DBOptionsTest::WaitForUpdates"); + + // Test with a file with a bad ColumnFamilyOptions + ASSERT_OK(CreateFile(fs, options.refresh_options_file, + "[DBOptions]\n" + "[CFOptions \"default\"]\n" + "unknown = value\n", + false)); + TEST_SYNC_POINT("DBOptionsTest::WaitForUpdates"); + + SyncPoint::GetInstance()->DisableProcessing(); +} + #endif // ROCKSDB_LITE TEST_F(DBOptionsTest, BottommostCompressionOptsWithFallbackType) { From 76bebd64b4259fc3bfefc12770ad16c2a617aa69 Mon Sep 17 00:00:00 2001 From: mrambacher Date: Wed, 4 Jan 2023 20:11:31 -0500 Subject: [PATCH 05/10] Update Unit Tests - Add tests for different/default refresh options file; - Added check that options file is deleted on completion - Add tests with multiple refreshes of the same file --- db/db_options_test.cc | 180 +++++++++++++++++++++++++++++++----------- 1 file changed, 133 insertions(+), 47 deletions(-) diff --git a/db/db_options_test.cc b/db/db_options_test.cc index 31f1a297e4..8f4a96cfe1 100644 --- a/db/db_options_test.cc +++ b/db/db_options_test.cc @@ -31,6 +31,11 @@ class DBOptionsTest : public DBTestBase { public: DBOptionsTest() : DBTestBase("db_options_test", /*env_do_fsync=*/true) {} + ~DBOptionsTest() override { + SyncPoint::GetInstance()->DisableProcessing(); + SyncPoint::GetInstance()->ClearAllCallBacks(); + } + #ifndef ROCKSDB_LITE std::unordered_map GetMutableDBOptionsMap( const DBOptions& options) { @@ -1154,13 +1159,23 @@ TEST_F(DBOptionsTest, ChangeCompression) { SyncPoint::GetInstance()->DisableProcessing(); } +namespace { +Status WaitForOptionsUpdate(const std::shared_ptr& fs, + const std::string& options_file) { + TEST_SYNC_POINT("DBOptionsTest::WaitForUpdates"); + auto s = fs->FileExists(options_file, IOOptions(), nullptr); + SyncPoint::GetInstance()->LoadDependency( + {{"DBImpl::RefreshOptions::Complete", "DBOptionsTest::WaitForUpdates"}}); + return s; +} +} // namespace + TEST_F(DBOptionsTest, RefreshOptions) { - std::string test_path; Options options = CurrentOptions(); auto fs = options.env->GetFileSystem(); options.create_if_missing = true; options.refresh_options_sec = 1; - options.refresh_options_file = dbname_ + "/Options.New"; + options.refresh_options_file = dbname_ + "/Options.new"; options.max_background_jobs = 1; options.max_background_compactions = 2; options.periodic_compaction_seconds = 100; @@ -1176,31 +1191,127 @@ TEST_F(DBOptionsTest, RefreshOptions) { ASSERT_OK(PersistRocksDBOptions(config_options, options, {"default"}, {options}, options.refresh_options_file, fs.get())); + ASSERT_NOK(WaitForOptionsUpdate(fs, options.refresh_options_file)); + + DBOptions new_db_opts = db_->GetDBOptions(); + ASSERT_EQ(new_db_opts.max_background_jobs, 10); + ASSERT_EQ(new_db_opts.max_background_compactions, 20); + auto dcfh = db_->DefaultColumnFamily(); + ColumnFamilyDescriptor dcd; + ASSERT_OK(dcfh->GetDescriptor(&dcd)); + ASSERT_EQ(dcd.options.periodic_compaction_seconds, 200); +} + +TEST_F(DBOptionsTest, RefreshSimpleOptions) { + Options options = CurrentOptions(); + auto fs = options.env->GetFileSystem(); + options.create_if_missing = true; + options.max_background_compactions = 11; + options.refresh_options_sec = 1; + options.refresh_options_file = dbname_ + "/Options.new"; + options.enable_blob_files = false; + ASSERT_OK(TryReopen(options)); - TEST_SYNC_POINT("DBOptionsTest::WaitForUpdates"); - SyncPoint::GetInstance()->DisableProcessing(); SyncPoint::GetInstance()->LoadDependency( {{"DBImpl::RefreshOptions::Complete", "DBOptionsTest::WaitForUpdates"}}); SyncPoint::GetInstance()->EnableProcessing(); + // Test with a file that contains only DBOptions + ASSERT_OK(CreateFile(fs, options.refresh_options_file, + "[DBOptions]\n" + "max_background_compactions = 22\n" + "[CFOptions \"default\"]\n", + false)); + ASSERT_NOK(WaitForOptionsUpdate(fs, options.refresh_options_file)); DBOptions new_db_opts = db_->GetDBOptions(); - ASSERT_EQ(new_db_opts.max_background_jobs, 10); - ASSERT_EQ(new_db_opts.max_background_compactions, 20); + ASSERT_EQ(new_db_opts.max_background_compactions, 22); + + // Test with a file that contains only ColumnFamilyOptions + ASSERT_OK(CreateFile(fs, options.refresh_options_file, + "[DBOptions]\n" + "[CFOptions \"default\"]\n" + "enable_blob_files = true\n", + false)); + ASSERT_NOK(WaitForOptionsUpdate(fs, options.refresh_options_file)); auto dcfh = db_->DefaultColumnFamily(); ColumnFamilyDescriptor dcd; ASSERT_OK(dcfh->GetDescriptor(&dcd)); - ASSERT_EQ(dcd.options.periodic_compaction_seconds, 200); + ASSERT_EQ(dcd.options.enable_blob_files, true); - SyncPoint::GetInstance()->DisableProcessing(); + // Test with a file that contains a table factory options + ASSERT_OK(CreateFile(fs, options.refresh_options_file, + "[DBOptions]\n" + "[CFOptions \"default\"]\n" + "table_factory.block_size = 32768\n", + false)); + ASSERT_NOK(WaitForOptionsUpdate(fs, options.refresh_options_file)); + + ASSERT_OK(dcfh->GetDescriptor(&dcd)); + auto bbto = dcd.options.table_factory->GetOptions(); + ASSERT_NE(bbto, nullptr); + ASSERT_EQ(bbto->block_size, 32768); +} + +TEST_F(DBOptionsTest, DifferentOptionsFile) { + Options options = CurrentOptions(); + auto fs = options.env->GetFileSystem(); + options.create_if_missing = true; + options.refresh_options_sec = 1; + options.refresh_options_file = ""; + options.max_background_jobs = 1; + options.max_background_compactions = 2; + options.periodic_compaction_seconds = 100; + ASSERT_OK(TryReopen(options)); + SyncPoint::GetInstance()->LoadDependency( + {{"DBImpl::RefreshOptions::Complete", "DBOptionsTest::WaitForUpdates"}}); + SyncPoint::GetInstance()->EnableProcessing(); + ConfigOptions config_options; + config_options.mutable_options_only = true; + options.refresh_options_file = "Options.tmp.1"; + ASSERT_OK(PersistRocksDBOptions(config_options, options, {"default"}, + {options}, dbname_ + "/Options.new", + fs.get())); + ASSERT_NOK(WaitForOptionsUpdate(fs, dbname_ + "/Options.new")); + + DBOptions new_db_opts = db_->GetDBOptions(); + ASSERT_EQ(new_db_opts.refresh_options_file, "Options.tmp.1"); + + options.refresh_options_file = "Options.tmp.2"; + ASSERT_OK(PersistRocksDBOptions(config_options, options, {"default"}, + {options}, dbname_ + "/Options.tmp.1", + fs.get())); + ASSERT_NOK(WaitForOptionsUpdate(fs, dbname_ + "/Options.tmp.1")); + new_db_opts = db_->GetDBOptions(); + ASSERT_EQ(new_db_opts.refresh_options_file, "Options.tmp.2"); + + ASSERT_OK(fs->CreateDir(dbname_ + "/Options.tmp", IOOptions(), nullptr)); + options.refresh_options_file = dbname_ + "/Options.tmp/Options.new"; + ASSERT_OK(PersistRocksDBOptions(config_options, options, {"default"}, + {options}, dbname_ + "/Options.tmp.2", + fs.get())); + + ASSERT_NOK(WaitForOptionsUpdate(fs, dbname_ + "/Options.tmp.2")); + + new_db_opts = db_->GetDBOptions(); + ASSERT_EQ(new_db_opts.refresh_options_file, + dbname_ + "/Options.tmp/Options.new"); + + options.max_background_compactions = 4; + ASSERT_OK( + PersistRocksDBOptions(config_options, options, {"default"}, {options}, + dbname_ + "/Options.tmp/Options.new", fs.get())); + ASSERT_NOK(WaitForOptionsUpdate(fs, dbname_ + "/Options.tmp/Options.new")); + new_db_opts = db_->GetDBOptions(); + ASSERT_EQ(new_db_opts.max_background_compactions, 4); + ASSERT_OK(fs->DeleteDir(dbname_ + "/Options.tmp", IOOptions(), nullptr)); } TEST_F(DBOptionsTest, RefreshOptionsImmutable) { - std::string test_path; Options options = CurrentOptions(); auto fs = options.env->GetFileSystem(); options.create_if_missing = true; options.refresh_options_sec = 1; - options.refresh_options_file = dbname_ + "/Options.New"; + options.refresh_options_file = dbname_ + "/Options.new"; ASSERT_OK(TryReopen(options)); SyncPoint::GetInstance()->LoadDependency( {{"DBImpl::RefreshOptions::Complete", "DBOptionsTest::WaitForUpdates"}}); @@ -1214,38 +1325,31 @@ TEST_F(DBOptionsTest, RefreshOptionsImmutable) { ASSERT_OK(PersistRocksDBOptions(config_options, options, {"default"}, {options}, options.refresh_options_file, fs.get())); - TEST_SYNC_POINT("DBOptionsTest::WaitForUpdates"); - - SyncPoint::GetInstance()->DisableProcessing(); - SyncPoint::GetInstance()->LoadDependency( - {{"DBImpl::RefreshOptions::Complete", "DBOptionsTest::WaitForUpdates"}}); - SyncPoint::GetInstance()->EnableProcessing(); + ASSERT_NOK(WaitForOptionsUpdate(fs, options.refresh_options_file)); // Test setting an immutable ColumnFamilyOption and see the value // did not change options = CurrentOptions(); options.comparator = ReverseBytewiseComparator(); options.refresh_options_sec = 1; - options.refresh_options_file = dbname_ + "/Options.New"; + options.refresh_options_file = dbname_ + "/Options.new"; ASSERT_OK(PersistRocksDBOptions(config_options, options, {"default"}, {options}, options.refresh_options_file, fs.get())); - TEST_SYNC_POINT("DBOptionsTest::WaitForUpdates"); + ASSERT_NOK(WaitForOptionsUpdate(fs, options.refresh_options_file)); + auto dcfh = db_->DefaultColumnFamily(); ColumnFamilyDescriptor dcd; ASSERT_OK(dcfh->GetDescriptor(&dcd)); ASSERT_EQ(dcd.options.comparator, BytewiseComparator()); - - SyncPoint::GetInstance()->DisableProcessing(); } TEST_F(DBOptionsTest, RefreshOptionsBadFile) { - std::string test_path; Options options = CurrentOptions(); auto fs = options.env->GetFileSystem(); options.create_if_missing = true; options.refresh_options_sec = 1; - options.refresh_options_file = dbname_ + "/Options.New"; + options.refresh_options_file = dbname_ + "/Options.new"; ASSERT_OK(TryReopen(options)); SyncPoint::GetInstance()->LoadDependency( @@ -1258,47 +1362,32 @@ TEST_F(DBOptionsTest, RefreshOptionsBadFile) { SyncPoint::GetInstance()->EnableProcessing(); // Test with a file that is not an options file ASSERT_OK(CreateFile(fs, options.refresh_options_file, "fred", false)); - TEST_SYNC_POINT("DBOptionsTest::WaitForUpdates"); - SyncPoint::GetInstance()->DisableProcessing(); - SyncPoint::GetInstance()->LoadDependency( - {{"DBImpl::RefreshOptions::Complete", "DBOptionsTest::WaitForUpdates"}}); - SyncPoint::GetInstance()->EnableProcessing(); + ASSERT_NOK(WaitForOptionsUpdate(fs, options.refresh_options_file)); // Test with a file that contains no DBOptions section ASSERT_OK(CreateFile(fs, options.refresh_options_file, "[CFOptions \"default\"]\n", false)); - TEST_SYNC_POINT("DBOptionsTest::WaitForUpdates"); - SyncPoint::GetInstance()->DisableProcessing(); - SyncPoint::GetInstance()->LoadDependency( - {{"DBImpl::RefreshOptions::Complete", "DBOptionsTest::WaitForUpdates"}}); - SyncPoint::GetInstance()->EnableProcessing(); + ASSERT_NOK(WaitForOptionsUpdate(fs, options.refresh_options_file)); // Test with a file that contains no ColumnFamilyOptions section ASSERT_OK( CreateFile(fs, options.refresh_options_file, "[DBOptions]\n", false)); - TEST_SYNC_POINT("DBOptionsTest::WaitForUpdates"); - SyncPoint::GetInstance()->DisableProcessing(); - SyncPoint::GetInstance()->LoadDependency( - {{"DBImpl::RefreshOptions::Complete", "DBOptionsTest::WaitForUpdates"}}); - SyncPoint::GetInstance()->EnableProcessing(); + ASSERT_NOK(WaitForOptionsUpdate(fs, options.refresh_options_file)); // Test with a file that contains no default ColumnFamilyOptions section ASSERT_OK(CreateFile(fs, options.refresh_options_file, "[DBOptions]\n" "[CFOptions \"unknown\"]\n", false)); - TEST_SYNC_POINT("DBOptionsTest::WaitForUpdates"); - - SyncPoint::GetInstance()->DisableProcessing(); + ASSERT_NOK(WaitForOptionsUpdate(fs, options.refresh_options_file)); } TEST_F(DBOptionsTest, RefreshOptionsUnknown) { - std::string test_path; Options options = CurrentOptions(); auto fs = options.env->GetFileSystem(); options.create_if_missing = true; options.refresh_options_sec = 1; - options.refresh_options_file = dbname_ + "/Options.New"; + options.refresh_options_file = dbname_ + "/Options.new"; ASSERT_OK(TryReopen(options)); SyncPoint::GetInstance()->LoadDependency( {{"DBImpl::RefreshOptions::Complete", "DBOptionsTest::WaitForUpdates"}}); @@ -1314,13 +1403,12 @@ TEST_F(DBOptionsTest, RefreshOptionsUnknown) { }); SyncPoint::GetInstance()->EnableProcessing(); // Test with a file that contains a bad DBOptions value - ASSERT_OK(CreateFile(fs, options.refresh_options_file, "[DBOptions]\n" "unknown = value\n" "[CFOptions \"default\"]\n", false)); - TEST_SYNC_POINT("DBOptionsTest::WaitForUpdates"); + ASSERT_NOK(WaitForOptionsUpdate(fs, options.refresh_options_file)); // Test with a file with a bad ColumnFamilyOptions ASSERT_OK(CreateFile(fs, options.refresh_options_file, @@ -1328,9 +1416,7 @@ TEST_F(DBOptionsTest, RefreshOptionsUnknown) { "[CFOptions \"default\"]\n" "unknown = value\n", false)); - TEST_SYNC_POINT("DBOptionsTest::WaitForUpdates"); - - SyncPoint::GetInstance()->DisableProcessing(); + ASSERT_NOK(WaitForOptionsUpdate(fs, options.refresh_options_file)); } #endif // ROCKSDB_LITE From ed45a22ef5eedfd470c78e8d4b7d0ff6a3823edc Mon Sep 17 00:00:00 2001 From: mrambacher Date: Fri, 6 Jan 2023 17:11:12 -0500 Subject: [PATCH 06/10] Add test if refresh_options_file is dir, not file --- db/db_options_test.cc | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/db/db_options_test.cc b/db/db_options_test.cc index 8f4a96cfe1..7412a27cc5 100644 --- a/db/db_options_test.cc +++ b/db/db_options_test.cc @@ -1380,6 +1380,17 @@ TEST_F(DBOptionsTest, RefreshOptionsBadFile) { "[CFOptions \"unknown\"]\n", false)); ASSERT_NOK(WaitForOptionsUpdate(fs, options.refresh_options_file)); + + // Test what happens if the refresh_options_file is a directory, not a file + bool exists = false; + SyncPoint::GetInstance()->SetCallBack("DBImpl::RefreshOptions::FileExists", + [&](void* /*arg*/) { exists = true; }); + + ASSERT_OK(fs->CreateDir(options.refresh_options_file, IOOptions(), nullptr)); + TEST_SYNC_POINT("DBOptionsTest::WaitForUpdates"); + ASSERT_TRUE(exists); + ASSERT_OK(fs->FileExists(options.refresh_options_file, IOOptions(), nullptr)); + ASSERT_OK(fs->DeleteDir(options.refresh_options_file, IOOptions(), nullptr)); } TEST_F(DBOptionsTest, RefreshOptionsUnknown) { From 86d6fe9473d018e5e6602b525a1430a955e73af7 Mon Sep 17 00:00:00 2001 From: mrambacher Date: Thu, 19 Jan 2023 20:09:18 -0500 Subject: [PATCH 07/10] Attempted fix for compilation error --- db/db_options_test.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/db/db_options_test.cc b/db/db_options_test.cc index 7412a27cc5..a4920f183e 100644 --- a/db/db_options_test.cc +++ b/db/db_options_test.cc @@ -1160,7 +1160,7 @@ TEST_F(DBOptionsTest, ChangeCompression) { } namespace { -Status WaitForOptionsUpdate(const std::shared_ptr& fs, +IOStatus WaitForOptionsUpdate(const std::shared_ptr& fs, const std::string& options_file) { TEST_SYNC_POINT("DBOptionsTest::WaitForUpdates"); auto s = fs->FileExists(options_file, IOOptions(), nullptr); From f6adecbf69f9cac04f87cf08d1b4d56bc6c945a6 Mon Sep 17 00:00:00 2001 From: mrambacher Date: Tue, 7 Feb 2023 07:37:00 -0500 Subject: [PATCH 08/10] make format --- db/db_options_test.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/db/db_options_test.cc b/db/db_options_test.cc index a4920f183e..93da51ef24 100644 --- a/db/db_options_test.cc +++ b/db/db_options_test.cc @@ -1161,7 +1161,7 @@ TEST_F(DBOptionsTest, ChangeCompression) { namespace { IOStatus WaitForOptionsUpdate(const std::shared_ptr& fs, - const std::string& options_file) { + const std::string& options_file) { TEST_SYNC_POINT("DBOptionsTest::WaitForUpdates"); auto s = fs->FileExists(options_file, IOOptions(), nullptr); SyncPoint::GetInstance()->LoadDependency( From 3866036f809ff77341c3a6624d8a9ccb90a88e2e Mon Sep 17 00:00:00 2001 From: mrambacher Date: Tue, 7 Feb 2023 17:20:32 -0500 Subject: [PATCH 09/10] Fix failing tests - Missed periodic task after changing the default - Options settable missed as does not run on the Mac - Fixed race condition on writing/checking files in db_options_test --- db/db_options_test.cc | 109 +++++++++++++++++------------ db/periodic_task_scheduler_test.cc | 2 + options/options_settable_test.cc | 2 + 3 files changed, 68 insertions(+), 45 deletions(-) diff --git a/db/db_options_test.cc b/db/db_options_test.cc index 93da51ef24..6cd36b0858 100644 --- a/db/db_options_test.cc +++ b/db/db_options_test.cc @@ -1161,9 +1161,14 @@ TEST_F(DBOptionsTest, ChangeCompression) { namespace { IOStatus WaitForOptionsUpdate(const std::shared_ptr& fs, - const std::string& options_file) { - TEST_SYNC_POINT("DBOptionsTest::WaitForUpdates"); - auto s = fs->FileExists(options_file, IOOptions(), nullptr); + const std::string& tmp_options_file, + const std::string& new_options_file) { + auto s = + fs->RenameFile(tmp_options_file, new_options_file, IOOptions(), nullptr); + if (s.ok()) { + TEST_SYNC_POINT("DBOptionsTest::WaitForUpdates"); + s = fs->FileExists(new_options_file, IOOptions(), nullptr); + } SyncPoint::GetInstance()->LoadDependency( {{"DBImpl::RefreshOptions::Complete", "DBOptionsTest::WaitForUpdates"}}); return s; @@ -1176,6 +1181,7 @@ TEST_F(DBOptionsTest, RefreshOptions) { options.create_if_missing = true; options.refresh_options_sec = 1; options.refresh_options_file = dbname_ + "/Options.new"; + std::string tmp_options_file = dbname_ + "/Options.tmp"; options.max_background_jobs = 1; options.max_background_compactions = 2; options.periodic_compaction_seconds = 100; @@ -1189,9 +1195,9 @@ TEST_F(DBOptionsTest, RefreshOptions) { options.max_background_compactions = 20; options.periodic_compaction_seconds = 200; ASSERT_OK(PersistRocksDBOptions(config_options, options, {"default"}, - {options}, options.refresh_options_file, - fs.get())); - ASSERT_NOK(WaitForOptionsUpdate(fs, options.refresh_options_file)); + {options}, tmp_options_file, fs.get())); + ASSERT_NOK( + WaitForOptionsUpdate(fs, tmp_options_file, options.refresh_options_file)); DBOptions new_db_opts = db_->GetDBOptions(); ASSERT_EQ(new_db_opts.max_background_jobs, 10); @@ -1209,6 +1215,7 @@ TEST_F(DBOptionsTest, RefreshSimpleOptions) { options.max_background_compactions = 11; options.refresh_options_sec = 1; options.refresh_options_file = dbname_ + "/Options.new"; + std::string tmp_options_file = dbname_ + "/Options.tmp"; options.enable_blob_files = false; ASSERT_OK(TryReopen(options)); @@ -1217,34 +1224,37 @@ TEST_F(DBOptionsTest, RefreshSimpleOptions) { SyncPoint::GetInstance()->EnableProcessing(); // Test with a file that contains only DBOptions - ASSERT_OK(CreateFile(fs, options.refresh_options_file, + ASSERT_OK(CreateFile(fs, tmp_options_file, "[DBOptions]\n" "max_background_compactions = 22\n" "[CFOptions \"default\"]\n", false)); - ASSERT_NOK(WaitForOptionsUpdate(fs, options.refresh_options_file)); + ASSERT_NOK( + WaitForOptionsUpdate(fs, tmp_options_file, options.refresh_options_file)); DBOptions new_db_opts = db_->GetDBOptions(); ASSERT_EQ(new_db_opts.max_background_compactions, 22); // Test with a file that contains only ColumnFamilyOptions - ASSERT_OK(CreateFile(fs, options.refresh_options_file, + ASSERT_OK(CreateFile(fs, tmp_options_file, "[DBOptions]\n" "[CFOptions \"default\"]\n" "enable_blob_files = true\n", false)); - ASSERT_NOK(WaitForOptionsUpdate(fs, options.refresh_options_file)); + ASSERT_NOK( + WaitForOptionsUpdate(fs, tmp_options_file, options.refresh_options_file)); auto dcfh = db_->DefaultColumnFamily(); ColumnFamilyDescriptor dcd; ASSERT_OK(dcfh->GetDescriptor(&dcd)); ASSERT_EQ(dcd.options.enable_blob_files, true); // Test with a file that contains a table factory options - ASSERT_OK(CreateFile(fs, options.refresh_options_file, + ASSERT_OK(CreateFile(fs, tmp_options_file, "[DBOptions]\n" "[CFOptions \"default\"]\n" "table_factory.block_size = 32768\n", false)); - ASSERT_NOK(WaitForOptionsUpdate(fs, options.refresh_options_file)); + ASSERT_NOK( + WaitForOptionsUpdate(fs, tmp_options_file, options.refresh_options_file)); ASSERT_OK(dcfh->GetDescriptor(&dcd)); auto bbto = dcd.options.table_factory->GetOptions(); @@ -1261,6 +1271,7 @@ TEST_F(DBOptionsTest, DifferentOptionsFile) { options.max_background_jobs = 1; options.max_background_compactions = 2; options.periodic_compaction_seconds = 100; + std::string tmp_options_file = dbname_ + "/Options.new.tmp"; ASSERT_OK(TryReopen(options)); SyncPoint::GetInstance()->LoadDependency( {{"DBImpl::RefreshOptions::Complete", "DBOptionsTest::WaitForUpdates"}}); @@ -1269,38 +1280,38 @@ TEST_F(DBOptionsTest, DifferentOptionsFile) { config_options.mutable_options_only = true; options.refresh_options_file = "Options.tmp.1"; ASSERT_OK(PersistRocksDBOptions(config_options, options, {"default"}, - {options}, dbname_ + "/Options.new", - fs.get())); - ASSERT_NOK(WaitForOptionsUpdate(fs, dbname_ + "/Options.new")); + {options}, tmp_options_file, fs.get())); + ASSERT_NOK( + WaitForOptionsUpdate(fs, tmp_options_file, dbname_ + "/Options.new")); DBOptions new_db_opts = db_->GetDBOptions(); ASSERT_EQ(new_db_opts.refresh_options_file, "Options.tmp.1"); options.refresh_options_file = "Options.tmp.2"; ASSERT_OK(PersistRocksDBOptions(config_options, options, {"default"}, - {options}, dbname_ + "/Options.tmp.1", - fs.get())); - ASSERT_NOK(WaitForOptionsUpdate(fs, dbname_ + "/Options.tmp.1")); + {options}, tmp_options_file, fs.get())); + ASSERT_NOK( + WaitForOptionsUpdate(fs, tmp_options_file, dbname_ + "/Options.tmp.1")); new_db_opts = db_->GetDBOptions(); ASSERT_EQ(new_db_opts.refresh_options_file, "Options.tmp.2"); ASSERT_OK(fs->CreateDir(dbname_ + "/Options.tmp", IOOptions(), nullptr)); options.refresh_options_file = dbname_ + "/Options.tmp/Options.new"; ASSERT_OK(PersistRocksDBOptions(config_options, options, {"default"}, - {options}, dbname_ + "/Options.tmp.2", - fs.get())); + {options}, tmp_options_file, fs.get())); - ASSERT_NOK(WaitForOptionsUpdate(fs, dbname_ + "/Options.tmp.2")); + ASSERT_NOK( + WaitForOptionsUpdate(fs, tmp_options_file, dbname_ + "/Options.tmp.2")); new_db_opts = db_->GetDBOptions(); ASSERT_EQ(new_db_opts.refresh_options_file, dbname_ + "/Options.tmp/Options.new"); options.max_background_compactions = 4; - ASSERT_OK( - PersistRocksDBOptions(config_options, options, {"default"}, {options}, - dbname_ + "/Options.tmp/Options.new", fs.get())); - ASSERT_NOK(WaitForOptionsUpdate(fs, dbname_ + "/Options.tmp/Options.new")); + ASSERT_OK(PersistRocksDBOptions(config_options, options, {"default"}, + {options}, tmp_options_file, fs.get())); + ASSERT_NOK(WaitForOptionsUpdate(fs, tmp_options_file, + dbname_ + "/Options.tmp/Options.new")); new_db_opts = db_->GetDBOptions(); ASSERT_EQ(new_db_opts.max_background_compactions, 4); ASSERT_OK(fs->DeleteDir(dbname_ + "/Options.tmp", IOOptions(), nullptr)); @@ -1312,6 +1323,7 @@ TEST_F(DBOptionsTest, RefreshOptionsImmutable) { options.create_if_missing = true; options.refresh_options_sec = 1; options.refresh_options_file = dbname_ + "/Options.new"; + std::string tmp_options_file = dbname_ + "/Options.tmp"; ASSERT_OK(TryReopen(options)); SyncPoint::GetInstance()->LoadDependency( {{"DBImpl::RefreshOptions::Complete", "DBOptionsTest::WaitForUpdates"}}); @@ -1323,9 +1335,9 @@ TEST_F(DBOptionsTest, RefreshOptionsImmutable) { std::unique_ptr mock(MockEnv::Create(options.env)); options.env = mock.get(); ASSERT_OK(PersistRocksDBOptions(config_options, options, {"default"}, - {options}, options.refresh_options_file, - fs.get())); - ASSERT_NOK(WaitForOptionsUpdate(fs, options.refresh_options_file)); + {options}, tmp_options_file, fs.get())); + ASSERT_NOK( + WaitForOptionsUpdate(fs, tmp_options_file, options.refresh_options_file)); // Test setting an immutable ColumnFamilyOption and see the value // did not change @@ -1334,9 +1346,9 @@ TEST_F(DBOptionsTest, RefreshOptionsImmutable) { options.refresh_options_sec = 1; options.refresh_options_file = dbname_ + "/Options.new"; ASSERT_OK(PersistRocksDBOptions(config_options, options, {"default"}, - {options}, options.refresh_options_file, - fs.get())); - ASSERT_NOK(WaitForOptionsUpdate(fs, options.refresh_options_file)); + {options}, tmp_options_file, fs.get())); + ASSERT_NOK( + WaitForOptionsUpdate(fs, tmp_options_file, options.refresh_options_file)); auto dcfh = db_->DefaultColumnFamily(); ColumnFamilyDescriptor dcd; @@ -1350,6 +1362,7 @@ TEST_F(DBOptionsTest, RefreshOptionsBadFile) { options.create_if_missing = true; options.refresh_options_sec = 1; options.refresh_options_file = dbname_ + "/Options.new"; + std::string tmp_options_file = dbname_ + "/Options.tmp"; ASSERT_OK(TryReopen(options)); SyncPoint::GetInstance()->LoadDependency( @@ -1361,25 +1374,28 @@ TEST_F(DBOptionsTest, RefreshOptionsBadFile) { }); SyncPoint::GetInstance()->EnableProcessing(); // Test with a file that is not an options file - ASSERT_OK(CreateFile(fs, options.refresh_options_file, "fred", false)); - ASSERT_NOK(WaitForOptionsUpdate(fs, options.refresh_options_file)); + ASSERT_OK(CreateFile(fs, tmp_options_file, "fred", false)); + ASSERT_NOK( + WaitForOptionsUpdate(fs, tmp_options_file, options.refresh_options_file)); // Test with a file that contains no DBOptions section - ASSERT_OK(CreateFile(fs, options.refresh_options_file, - "[CFOptions \"default\"]\n", false)); - ASSERT_NOK(WaitForOptionsUpdate(fs, options.refresh_options_file)); + ASSERT_OK( + CreateFile(fs, tmp_options_file, "[CFOptions \"default\"]\n", false)); + ASSERT_NOK( + WaitForOptionsUpdate(fs, tmp_options_file, options.refresh_options_file)); // Test with a file that contains no ColumnFamilyOptions section - ASSERT_OK( - CreateFile(fs, options.refresh_options_file, "[DBOptions]\n", false)); - ASSERT_NOK(WaitForOptionsUpdate(fs, options.refresh_options_file)); + ASSERT_OK(CreateFile(fs, tmp_options_file, "[DBOptions]\n", false)); + ASSERT_NOK( + WaitForOptionsUpdate(fs, tmp_options_file, options.refresh_options_file)); // Test with a file that contains no default ColumnFamilyOptions section - ASSERT_OK(CreateFile(fs, options.refresh_options_file, + ASSERT_OK(CreateFile(fs, tmp_options_file, "[DBOptions]\n" "[CFOptions \"unknown\"]\n", false)); - ASSERT_NOK(WaitForOptionsUpdate(fs, options.refresh_options_file)); + ASSERT_NOK( + WaitForOptionsUpdate(fs, tmp_options_file, options.refresh_options_file)); // Test what happens if the refresh_options_file is a directory, not a file bool exists = false; @@ -1399,6 +1415,7 @@ TEST_F(DBOptionsTest, RefreshOptionsUnknown) { options.create_if_missing = true; options.refresh_options_sec = 1; options.refresh_options_file = dbname_ + "/Options.new"; + std::string tmp_options_file = dbname_ + "/Options.tmp"; ASSERT_OK(TryReopen(options)); SyncPoint::GetInstance()->LoadDependency( {{"DBImpl::RefreshOptions::Complete", "DBOptionsTest::WaitForUpdates"}}); @@ -1414,20 +1431,22 @@ TEST_F(DBOptionsTest, RefreshOptionsUnknown) { }); SyncPoint::GetInstance()->EnableProcessing(); // Test with a file that contains a bad DBOptions value - ASSERT_OK(CreateFile(fs, options.refresh_options_file, + ASSERT_OK(CreateFile(fs, tmp_options_file, "[DBOptions]\n" "unknown = value\n" "[CFOptions \"default\"]\n", false)); - ASSERT_NOK(WaitForOptionsUpdate(fs, options.refresh_options_file)); + ASSERT_NOK( + WaitForOptionsUpdate(fs, tmp_options_file, options.refresh_options_file)); // Test with a file with a bad ColumnFamilyOptions - ASSERT_OK(CreateFile(fs, options.refresh_options_file, + ASSERT_OK(CreateFile(fs, tmp_options_file, "[DBOptions]\n" "[CFOptions \"default\"]\n" "unknown = value\n", false)); - ASSERT_NOK(WaitForOptionsUpdate(fs, options.refresh_options_file)); + ASSERT_NOK( + WaitForOptionsUpdate(fs, tmp_options_file, options.refresh_options_file)); } #endif // ROCKSDB_LITE diff --git a/db/periodic_task_scheduler_test.cc b/db/periodic_task_scheduler_test.cc index b0922c0f8c..d93323515a 100644 --- a/db/periodic_task_scheduler_test.cc +++ b/db/periodic_task_scheduler_test.cc @@ -42,6 +42,7 @@ TEST_F(PeriodicTaskSchedulerTest, Basic) { Options options; options.stats_dump_period_sec = kPeriodSec; options.stats_persist_period_sec = kPeriodSec; + options.refresh_options_sec = 0; options.create_if_missing = true; options.env = mock_env_.get(); @@ -130,6 +131,7 @@ TEST_F(PeriodicTaskSchedulerTest, MultiInstances) { Options options; options.stats_dump_period_sec = kPeriodSec; options.stats_persist_period_sec = kPeriodSec; + options.refresh_options_sec = 0; options.create_if_missing = true; options.env = mock_env_.get(); diff --git a/options/options_settable_test.cc b/options/options_settable_test.cc index f6502f12cb..7675059137 100644 --- a/options/options_settable_test.cc +++ b/options/options_settable_test.cc @@ -361,6 +361,8 @@ TEST_F(OptionsSettableTest, DBOptionsAllFieldsSettable) { "lowest_used_cache_tier=kNonVolatileBlockTier;" "allow_data_in_errors=false;" "enforce_single_del_contracts=false;" + "refresh_options_sec=0;" + "refresh_options_file=Options.new;" "use_dynamic_delay=true", new_options)); From 653860a4b8669ae82e90d3b89e683d2eb04670f2 Mon Sep 17 00:00:00 2001 From: mrambacher Date: Fri, 10 Feb 2023 10:26:44 -0500 Subject: [PATCH 10/10] Fix for OptionsSettableTest.DBOptionsAllFieldsSettable --- options/options_settable_test.cc | 1 + 1 file changed, 1 insertion(+) diff --git a/options/options_settable_test.cc b/options/options_settable_test.cc index 7675059137..d15f6fd8e9 100644 --- a/options/options_settable_test.cc +++ b/options/options_settable_test.cc @@ -251,6 +251,7 @@ TEST_F(OptionsSettableTest, DBOptionsAllFieldsSettable) { sizeof(FileTypeSet)}, {offsetof(struct DBOptions, compaction_service), sizeof(std::shared_ptr)}, + {offsetof(struct DBOptions, refresh_options_file), sizeof(std::string)}, }; char* options_ptr = new char[sizeof(DBOptions)];