Skip to content

Commit

Permalink
fix calling SetOptions on deprecated options
Browse files Browse the repository at this point in the history
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 facebook#3700

Differential Revision: D7572596

Pulled By: ajkr

fbshipit-source-id: bd5d84e14c0c39f30c5d4c6df7c1503d2c28ecf1
  • Loading branch information
ajkr authored and facebook-github-bot committed Apr 11, 2018
1 parent d95014b commit 019d789
Show file tree
Hide file tree
Showing 3 changed files with 12 additions and 4 deletions.
5 changes: 3 additions & 2 deletions db/column_family.cc
Original file line number Diff line number Diff line change
Expand Up @@ -1148,8 +1148,9 @@ void ColumnFamilyData::ResetThreadLocalSuperVersions() {
Status ColumnFamilyData::SetOptions(
const std::unordered_map<std::string, std::string>& 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_);
Expand Down
9 changes: 8 additions & 1 deletion options/options_helper.cc
Original file line number Diff line number Diff line change
Expand Up @@ -704,7 +704,7 @@ bool SerializeSingleOptionHelper(const char* opt_address,
Status GetMutableOptionsFromStrings(
const MutableCFOptions& base_options,
const std::unordered_map<std::string, std::string>& options_map,
MutableCFOptions* new_options) {
Logger* info_log, MutableCFOptions* new_options) {
assert(new_options);
*new_options = base_options;
for (const auto& o : options_map) {
Expand All @@ -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<char*>(new_options) + opt_info.mutable_offset,
opt_info.type, o.second);
Expand Down
2 changes: 1 addition & 1 deletion options/options_helper.h
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ ColumnFamilyOptions BuildColumnFamilyOptions(
Status GetMutableOptionsFromStrings(
const MutableCFOptions& base_options,
const std::unordered_map<std::string, std::string>& options_map,
MutableCFOptions* new_options);
Logger* info_log, MutableCFOptions* new_options);

Status GetMutableDBOptionsFromStrings(
const MutableDBOptions& base_options,
Expand Down

0 comments on commit 019d789

Please sign in to comment.