From 019d7894eba8bb01cd4d82df1297f073dab937df Mon Sep 17 00:00:00 2001 From: Andrew Kryczka Date: Tue, 10 Apr 2018 18:48:49 -0700 Subject: [PATCH] fix calling SetOptions on deprecated options Summary: In `cf_options_type_info`, the deprecated options are all considered to have offset zero in the `MutableCFOptions` struct. Previously we weren't checking in `GetMutableOptionsFromStrings` whether the provided option was deprecated or not and simply writing the provided value to the offset specified by `cf_options_type_info`. That meant setting any deprecated option would overwrite the first element in the struct, which is `write_buffer_size`. `db_stress` hit this often since it calls `SetOptions` with `soft_rate_limit=0` and `hard_rate_limit=0`, which are both deprecated so cause `write_buffer_size` to be set to zero, which causes it to crash on the following assertion: ``` db_stress: db/memtable.cc:106: rocksdb::MemTable::MemTable(const rocksdb::InternalKeyComparator&, const rocksdb::ImmutableCFOptions&, const rocksdb::MutableCFOptions&, rocksdb::WriteBufferManager*, rocksdb::SequenceNumber, uint32_t): Assertion `!ShouldScheduleFlush()' failed. ``` We fix it by skipping deprecated options (and logging a warning) when users provide them to `SetOptions`. I didn't want to fail the call for compatibility reasons. Closes https://github.com/facebook/rocksdb/pull/3700 Differential Revision: D7572596 Pulled By: ajkr fbshipit-source-id: bd5d84e14c0c39f30c5d4c6df7c1503d2c28ecf1 --- db/column_family.cc | 5 +++-- options/options_helper.cc | 9 ++++++++- options/options_helper.h | 2 +- 3 files changed, 12 insertions(+), 4 deletions(-) diff --git a/db/column_family.cc b/db/column_family.cc index 5349b637a..9bb89ebac 100644 --- a/db/column_family.cc +++ b/db/column_family.cc @@ -1148,8 +1148,9 @@ void ColumnFamilyData::ResetThreadLocalSuperVersions() { Status ColumnFamilyData::SetOptions( const std::unordered_map& options_map) { MutableCFOptions new_mutable_cf_options; - Status s = GetMutableOptionsFromStrings(mutable_cf_options_, options_map, - &new_mutable_cf_options); + Status s = + GetMutableOptionsFromStrings(mutable_cf_options_, options_map, + ioptions_.info_log, &new_mutable_cf_options); if (s.ok()) { mutable_cf_options_ = new_mutable_cf_options; mutable_cf_options_.RefreshDerivedOptions(ioptions_); diff --git a/options/options_helper.cc b/options/options_helper.cc index 6f6c8e5ed..adf99e9b4 100644 --- a/options/options_helper.cc +++ b/options/options_helper.cc @@ -704,7 +704,7 @@ bool SerializeSingleOptionHelper(const char* opt_address, Status GetMutableOptionsFromStrings( const MutableCFOptions& base_options, const std::unordered_map& options_map, - MutableCFOptions* new_options) { + Logger* info_log, MutableCFOptions* new_options) { assert(new_options); *new_options = base_options; for (const auto& o : options_map) { @@ -717,6 +717,13 @@ Status GetMutableOptionsFromStrings( if (!opt_info.is_mutable) { return Status::InvalidArgument("Option not changeable: " + o.first); } + if (opt_info.verification == OptionVerificationType::kDeprecated) { + // log warning when user tries to set a deprecated option but don't fail + // the call for compatibility. + ROCKS_LOG_WARN(info_log, "%s is a deprecated option and cannot be set", + o.first.c_str()); + continue; + } bool is_ok = ParseOptionHelper( reinterpret_cast(new_options) + opt_info.mutable_offset, opt_info.type, o.second); diff --git a/options/options_helper.h b/options/options_helper.h index 2e46877db..ab91109be 100644 --- a/options/options_helper.h +++ b/options/options_helper.h @@ -31,7 +31,7 @@ ColumnFamilyOptions BuildColumnFamilyOptions( Status GetMutableOptionsFromStrings( const MutableCFOptions& base_options, const std::unordered_map& options_map, - MutableCFOptions* new_options); + Logger* info_log, MutableCFOptions* new_options); Status GetMutableDBOptionsFromStrings( const MutableDBOptions& base_options,