Skip to content

Commit

Permalink
Fix some minor issues in the Customizable infrastructure (facebook#8566)
Browse files Browse the repository at this point in the history 
Summary:
- Fix issue with OptionType::Vector when the nested item is a Customizable with no names
- Fix issue with OptionType::Vector to appropriately wrap the elements in a Vector;
- Fix an issue with nested Customizable object with a null immutable object still appearing in the mutable options;
- Fix/Add tests for null/empty customizable objects
- Move the RegisterTestObjects from customizable_test into testutil.

Pull Request resolved: facebook#8566

Reviewed By: zhichao-cao

Differential Revision: D30303724

Pulled By: mrambacher

fbshipit-source-id: 33fa8ea2a3b663210cb356da05e64aab7585b1b5
  • Loading branch information
@mrambacher @facebook-github-bot
mrambacher authored and facebook-github-bot committed Aug 19, 2021
1 parent c625b8d commit 9eb002f
Show file tree
Hide file tree
Showing 13 changed files with 534 additions and 306 deletions.
1 change: 1 addition & 0 deletions HISTORY.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@

## Public API change
* Added APIs to decode and replay trace file via Replayer class. Added `DB::NewDefaultReplayer()` to create a default Replayer instance. Added `TraceReader::Reset()` to restart reading a trace file. Created trace_record.h, trace_record_result.h and utilities/replayer.h files to access the decoded Trace records, replay them, and query the actual operation results.
* Added Configurable::GetOptionsMap to the public API for use in creating new Customizable classes.

### Performance Improvements
* Try to avoid updating DBOptions if `SetDBOptions()` does not change any option value.
Expand Down
18 changes: 18 additions & 0 deletions include/rocksdb/configurable.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -267,6 +267,24 @@ class Configurable {
// changed.
virtual bool IsPrepared() const { return is_prepared_; }

// Splits the input opt_value into the ID field and the remaining options.
// The input opt_value can be in the form of "name" or "name=value
// [;name=value]". The first form uses the "name" as an id with no options The
// latter form converts the input into a map of name=value pairs and sets "id"
// to the "id" value from the map.
// @param opt_value The value to split into id and options
// @param id The id field from the opt_value
// @param options The remaining name/value pairs from the opt_value
// @param default_id If specified and there is no id field in the map, this
// value is returned as the ID
// @return OK if the value was converted to a map successfully and an ID was
// found.
// @return InvalidArgument if the value could not be converted to a map or
// there was or there is no id property in the map.
static Status GetOptionsMap(
const std::string& opt_value, const std::string& default_id,
std::string* id, std::unordered_map<std::string, std::string>* options);

protected:
// True once the object is prepared. Once the object is prepared, only
// mutable options can be configured.
Expand Down
78 changes: 35 additions & 43 deletions include/rocksdb/utilities/customizable_util.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -58,24 +58,26 @@ static Status NewSharedObject(
const ConfigOptions& config_options, const std::string& id,
const std::unordered_map<std::string, std::string>& opt_map,
std::shared_ptr<T>* result) {
Status status;
if (!id.empty()) {
Status status;
#ifndef ROCKSDB_LITE
status = config_options.registry->NewSharedObject(id, result);
#else
status = Status::NotSupported("Cannot load object in LITE mode ", id);
#endif // ROCKSDB_LITE
if (config_options.ignore_unsupported_options && status.IsNotSupported()) {
return Status::OK();
status = Status::OK();
} else if (status.ok()) {
status = Customizable::ConfigureNewObject(config_options, result->get(),
opt_map);
}
} else {
status = Status::NotSupported("Cannot reset object ");
}
if (!status.ok()) {
return status;
} else if (opt_map.empty()) {
// There was no ID and no map (everything empty), so reset/clear the result
result->reset();
return Status::OK();
} else {
return Customizable::ConfigureNewObject(config_options, result->get(),
opt_map);
return Status::NotSupported("Cannot reset object ");
}
}

Expand Down Expand Up @@ -119,12 +121,7 @@ static Status LoadSharedObject(const ConfigOptions& config_options,
return status;
} else if (func == nullptr ||
!func(id, result)) { // No factory, or it failed
if (value.empty()) { // No Id and no options. Clear the object
*result = nullptr;
return Status::OK();
} else {
return NewSharedObject(config_options, id, opt_map, result);
}
return NewSharedObject(config_options, id, opt_map, result);
} else {
return Customizable::ConfigureNewObject(config_options, result->get(),
opt_map);
Expand All @@ -149,24 +146,26 @@ static Status NewUniqueObject(
const ConfigOptions& config_options, const std::string& id,
const std::unordered_map<std::string, std::string>& opt_map,
std::unique_ptr<T>* result) {
Status status;
if (id.empty()) {
status = Status::NotSupported("Cannot reset object ");
} else {
if (!id.empty()) {
Status status;
#ifndef ROCKSDB_LITE
status = config_options.registry->NewUniqueObject(id, result);
#else
status = Status::NotSupported("Cannot load object in LITE mode ", id);
#endif // ROCKSDB_LITE
if (config_options.ignore_unsupported_options && status.IsNotSupported()) {
return Status::OK();
status = Status::OK();
} else if (status.ok()) {
status = Customizable::ConfigureNewObject(config_options, result->get(),
opt_map);
}
}
if (!status.ok()) {
return status;
} else if (opt_map.empty()) {
// There was no ID and no map (everything empty), so reset/clear the result
result->reset();
return Status::OK();
} else {
return Customizable::ConfigureNewObject(config_options, result->get(),
opt_map);
return Status::NotSupported("Cannot reset object ");
}
}

Expand Down Expand Up @@ -194,12 +193,7 @@ static Status LoadUniqueObject(const ConfigOptions& config_options,
return status;
} else if (func == nullptr ||
!func(id, result)) { // No factory, or it failed
if (value.empty()) { // No Id and no options. Clear the object
*result = nullptr;
return Status::OK();
} else {
return NewUniqueObject(config_options, id, opt_map, result);
}
return NewUniqueObject(config_options, id, opt_map, result);
} else {
return Customizable::ConfigureNewObject(config_options, result->get(),
opt_map);
Expand All @@ -223,23 +217,26 @@ template <typename T>
static Status NewStaticObject(
const ConfigOptions& config_options, const std::string& id,
const std::unordered_map<std::string, std::string>& opt_map, T** result) {
Status status;
if (id.empty()) {
status = Status::NotSupported("Cannot reset object ");
} else {
if (!id.empty()) {
Status status;
#ifndef ROCKSDB_LITE
status = config_options.registry->NewStaticObject(id, result);
#else
status = Status::NotSupported("Cannot load object in LITE mode ", id);
#endif // ROCKSDB_LITE
if (config_options.ignore_unsupported_options && status.IsNotSupported()) {
return Status::OK();
status = Status::OK();
} else if (status.ok()) {
status =
Customizable::ConfigureNewObject(config_options, *result, opt_map);
}
}
if (!status.ok()) {
return status;
} else if (opt_map.empty()) {
// There was no ID and no map (everything empty), so reset/clear the result
*result = nullptr;
return Status::OK();
} else {
return Customizable::ConfigureNewObject(config_options, *result, opt_map);
return Status::NotSupported("Cannot reset object ");
}
}

Expand All @@ -266,12 +263,7 @@ static Status LoadStaticObject(const ConfigOptions& config_options,
return status;
} else if (func == nullptr ||
!func(id, result)) { // No factory, or it failed
if (value.empty()) { // No Id and no options. Clear the object
*result = nullptr;
return Status::OK();
} else {
return NewStaticObject(config_options, id, opt_map, result);
}
return NewStaticObject(config_options, id, opt_map, result);
} else {
return Customizable::ConfigureNewObject(config_options, *result, opt_map);
}
Expand Down
44 changes: 32 additions & 12 deletions include/rocksdb/utilities/options_type.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -430,10 +430,15 @@ class OptionTypeInfo {
return OptionTypeInfo(
offset, OptionType::kCustomizable, ovt,
flags | OptionTypeFlags::kShared,
[](const ConfigOptions& opts, const std::string&,
[](const ConfigOptions& opts, const std::string& name,
const std::string& value, void* addr) {
auto* shared = static_cast<std::shared_ptr<T>*>(addr);
return T::CreateFromString(opts, value, shared);
if (name == kIdPropName() && value.empty()) {
shared->reset();
return Status::OK();
} else {
return T::CreateFromString(opts, value, shared);
}
},
serialize_func, equals_func);
}
Expand Down Expand Up @@ -463,10 +468,15 @@ class OptionTypeInfo {
return OptionTypeInfo(
offset, OptionType::kCustomizable, ovt,
flags | OptionTypeFlags::kUnique,
[](const ConfigOptions& opts, const std::string&,
[](const ConfigOptions& opts, const std::string& name,
const std::string& value, void* addr) {
auto* unique = static_cast<std::unique_ptr<T>*>(addr);
return T::CreateFromString(opts, value, unique);
if (name == kIdPropName() && value.empty()) {
unique->reset();
return Status::OK();
} else {
return T::CreateFromString(opts, value, unique);
}
},
serialize_func, equals_func);
}
Expand Down Expand Up @@ -494,10 +504,15 @@ class OptionTypeInfo {
return OptionTypeInfo(
offset, OptionType::kCustomizable, ovt,
flags | OptionTypeFlags::kRawPointer,
[](const ConfigOptions& opts, const std::string&,
[](const ConfigOptions& opts, const std::string& name,
const std::string& value, void* addr) {
auto** pointer = static_cast<T**>(addr);
return T::CreateFromString(opts, value, pointer);
if (name == kIdPropName() && value.empty()) {
*pointer = nullptr;
return Status::OK();
} else {
return T::CreateFromString(opts, value, pointer);
}
},
serialize_func, equals_func);
}
Expand Down Expand Up @@ -773,6 +788,9 @@ class OptionTypeInfo {
static Status NextToken(const std::string& opts, char delimiter, size_t start,
size_t* end, std::string* token);

constexpr static const char* kIdPropName() { return "id"; }
constexpr static const char* kIdPropSuffix() { return ".id"; }

private:
int offset_;

Expand Down Expand Up @@ -867,18 +885,18 @@ Status SerializeVector(const ConfigOptions& config_options,
std::string result;
ConfigOptions embedded = config_options;
embedded.delimiter = ";";
for (size_t i = 0; i < vec.size(); ++i) {
int printed = 0;
for (const auto& elem : vec) {
std::string elem_str;
Status s = elem_info.Serialize(
embedded, name, reinterpret_cast<const char*>(&vec[i]), &elem_str);
Status s = elem_info.Serialize(embedded, name, &elem, &elem_str);
if (!s.ok()) {
return s;
} else {
if (i > 0) {
} else if (!elem_str.empty()) {
if (printed++ > 0) {
result += separator;
}
// If the element contains embedded separators, put it inside of brackets
if (result.find(separator) != std::string::npos) {
if (elem_str.find(separator) != std::string::npos) {
result += "{" + elem_str + "}";
} else {
result += elem_str;
Expand All @@ -887,6 +905,8 @@ Status SerializeVector(const ConfigOptions& config_options,
}
if (result.find("=") != std::string::npos) {
*value = "{" + result + "}";
} else if (printed > 1 && result.at(0) == '{') {
*value = "{" + result + "}";
} else {
*value = result;
}
Expand Down
18 changes: 11 additions & 7 deletions options/configurable.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -415,8 +415,8 @@ Status ConfigurableHelper::ConfigureCustomizableOption(

if (opt_info.IsMutable() || !config_options.mutable_options_only) {
// Either the option is mutable, or we are processing all of the options
if (opt_name == name || name == ConfigurableHelper::kIdPropName ||
EndsWith(opt_name, ConfigurableHelper::kIdPropSuffix)) {
if (opt_name == name || name == OptionTypeInfo::kIdPropName() ||
EndsWith(opt_name, OptionTypeInfo::kIdPropSuffix())) {
return configurable.ParseOption(copy, opt_info, name, value, opt_ptr);
} else if (value.empty()) {
return Status::OK();
Expand All @@ -439,8 +439,8 @@ Status ConfigurableHelper::ConfigureCustomizableOption(
} else {
return Status::InvalidArgument("Option not changeable: " + opt_name);
}
} else if (EndsWith(opt_name, ConfigurableHelper::kIdPropSuffix) ||
name == ConfigurableHelper::kIdPropName) {
} else if (EndsWith(opt_name, OptionTypeInfo::kIdPropSuffix()) ||
name == OptionTypeInfo::kIdPropName()) {
// We have a property of the form "id=value" or "table.id=value"
// This is OK if we ID/value matches the current customizable object
if (custom->GetId() == value) {
Expand All @@ -459,7 +459,8 @@ Status ConfigurableHelper::ConfigureCustomizableOption(
// map
std::unordered_map<std::string, std::string> props;
std::string id;
Status s = GetOptionsMap(value, custom->GetId(), &id, &props);
Status s =
Configurable::GetOptionsMap(value, custom->GetId(), &id, &props);
if (!s.ok()) {
return s;
} else if (custom->GetId() != id) {
Expand Down Expand Up @@ -734,7 +735,7 @@ bool ConfigurableHelper::AreEquivalent(const ConfigOptions& config_options,
}
#endif // ROCKSDB_LITE

Status ConfigurableHelper::GetOptionsMap(
Status Configurable::GetOptionsMap(
const std::string& value, const std::string& default_id, std::string* id,
std::unordered_map<std::string, std::string>* props) {
assert(id);
Expand All @@ -752,10 +753,13 @@ Status ConfigurableHelper::GetOptionsMap(
props->clear(); // Clear the properties
status = Status::OK(); // and ignore the error
} else {
auto iter = props->find(ConfigurableHelper::kIdPropName);
auto iter = props->find(OptionTypeInfo::kIdPropName());
if (iter != props->end()) {
*id = iter->second;
props->erase(iter);
if (*id == kNullptrString) {
id->clear();
}
} else if (!default_id.empty()) {
*id = default_id;
} else { // No id property and no default
Expand Down
20 changes: 0 additions & 20 deletions options/configurable_helper.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,6 @@ namespace ROCKSDB_NAMESPACE {
// of configuring the objects.
class ConfigurableHelper {
public:
constexpr static const char* kIdPropName = "id";
constexpr static const char* kIdPropSuffix = ".id";
// Configures the input Configurable object based on the parameters.
// On successful completion, the Configurable is updated with the settings
// from the opt_map.
Expand All @@ -48,24 +46,6 @@ class ConfigurableHelper {
const std::unordered_map<std::string, std::string>& options,
std::unordered_map<std::string, std::string>* unused);

// Splits the input opt_value into the ID field and the remaining options.
// The input opt_value can be in the form of "name" or "name=value
// [;name=value]". The first form uses the "name" as an id with no options The
// latter form converts the input into a map of name=value pairs and sets "id"
// to the "id" value from the map.
// @param opt_value The value to split into id and options
// @param id The id field from the opt_value
// @param options The remaining name/value pairs from the opt_value
// @param default_id If specified and there is no id field in the map, this
// value is returned as the ID
// @return OK if the value was converted to a map succesfully and an ID was
// found.
// @return InvalidArgument if the value could not be converted to a map or
// there was or there is no id property in the map.
static Status GetOptionsMap(
const std::string& opt_value, const std::string& default_id,
std::string* id, std::unordered_map<std::string, std::string>* options);

#ifndef ROCKSDB_LITE
// Internal method to configure a set of options for this object.
// Classes may override this value to change its behavior.
Expand Down
Loading

0 comments on commit 9eb002f

Please sign in to comment.