andreacavalli/rocksdb
0
0
Fork 0
Code Issues Pull Requests Projects Releases Wiki Activity

Fix GetOptionsPtr for Wrapped Customizable; Allow null options map (#9213)

Browse Source 
Summary:
1.  Fix GetOptionsPtr for Wrapped (Inner() != nullptr) Customizable objects.  This allows the inner options to be returned via this method.

2.  Allow the option type map to be nullptr.  This allows objects to be registered as options (for GetOptionsPtr) but not be used by the configuration methods.

Added tests as appropriate.

Pull Request resolved: https://github.com/facebook/rocksdb/pull/9213

Reviewed By: zhichao-cao

Differential Revision: D32718882

Pulled By: mrambacher

fbshipit-source-id: 563203d1f006a2629060feb31c5dff9a233e1e83
This commit is contained in:
mrambacher 2021-11-30 13:22:27 -08:00 committed by Levi Tamasi
parent 8e588307fc
commit 1217630bf3
4 changed files with 191 additions and 95 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

14
include/rocksdb/customizable.h
View File

@ -101,6 +101,20 @@ class Customizable : public Configurable {
}
}
const void* GetOptionsPtr(const std::string& name) const override {
const void* ptr = Configurable::GetOptionsPtr(name);
if (ptr != nullptr) {
return ptr;
} else {
const auto inner = Inner();
if (inner != nullptr) {
return inner->GetOptionsPtr(name);
} else {
return nullptr;
}
}
}
// Returns the named instance of the Customizable as a T*, or nullptr if not
// found. This method uses IsInstanceOf/Inner to find the appropriate class
// instance and then casts it to the expected return type.

166
options/configurable.cc
View File

@ -43,21 +43,23 @@ Status Configurable::PrepareOptions(const ConfigOptions& opts) {
Status status = Status::OK();
#ifndef ROCKSDB_LITE
for (auto opt_iter : options_) {
for (auto map_iter : *(opt_iter.type_map)) {
auto& opt_info = map_iter.second;
if (!opt_info.IsDeprecated() && !opt_info.IsAlias() &&
opt_info.IsConfigurable()) {
if (!opt_info.IsEnabled(OptionTypeFlags::kDontPrepare)) {
Configurable* config =
opt_info.AsRawPointer<Configurable>(opt_iter.opt_ptr);
if (config != nullptr) {
status = config->PrepareOptions(opts);
if (!status.ok()) {
return status;
if (opt_iter.type_map != nullptr) {
for (auto map_iter : *(opt_iter.type_map)) {
auto& opt_info = map_iter.second;
if (!opt_info.IsDeprecated() && !opt_info.IsAlias() &&
opt_info.IsConfigurable()) {
if (!opt_info.IsEnabled(OptionTypeFlags::kDontPrepare)) {
Configurable* config =
opt_info.AsRawPointer<Configurable>(opt_iter.opt_ptr);
if (config != nullptr) {
status = config->PrepareOptions(opts);
if (!status.ok()) {
return status;
}
} else if (!opt_info.CanBeNull()) {
status = Status::NotFound("Missing configurable object",
map_iter.first);
}
} else if (!opt_info.CanBeNull()) {
status =
Status::NotFound("Missing configurable object", map_iter.first);
}
}
}
@ -74,20 +76,22 @@ Status Configurable::ValidateOptions(const DBOptions& db_opts,
Status status;
#ifndef ROCKSDB_LITE
for (auto opt_iter : options_) {
for (auto map_iter : *(opt_iter.type_map)) {
auto& opt_info = map_iter.second;
if (!opt_info.IsDeprecated() && !opt_info.IsAlias()) {
if (opt_info.IsConfigurable()) {
const Configurable* config =
opt_info.AsRawPointer<Configurable>(opt_iter.opt_ptr);
if (config != nullptr) {
status = config->ValidateOptions(db_opts, cf_opts);
} else if (!opt_info.CanBeNull()) {
status =
Status::NotFound("Missing configurable object", map_iter.first);
}
if (!status.ok()) {
return status;
if (opt_iter.type_map != nullptr) {
for (auto map_iter : *(opt_iter.type_map)) {
auto& opt_info = map_iter.second;
if (!opt_info.IsDeprecated() && !opt_info.IsAlias()) {
if (opt_info.IsConfigurable()) {
const Configurable* config =
opt_info.AsRawPointer<Configurable>(opt_iter.opt_ptr);
if (config != nullptr) {
status = config->ValidateOptions(db_opts, cf_opts);
} else if (!opt_info.CanBeNull()) {
status = Status::NotFound("Missing configurable object",
map_iter.first);
}
if (!status.ok()) {
return status;
}
}
}
}
@ -124,11 +128,13 @@ const OptionTypeInfo* ConfigurableHelper::FindOption(
const std::vector<Configurable::RegisteredOptions>& options,
const std::string& short_name, std::string* opt_name, void** opt_ptr) {
for (auto iter : options) {
const auto opt_info =
OptionTypeInfo::Find(short_name, *(iter.type_map), opt_name);
if (opt_info != nullptr) {
*opt_ptr = iter.opt_ptr;
return opt_info;
if (iter.type_map != nullptr) {
const auto opt_info =
OptionTypeInfo::Find(short_name, *(iter.type_map), opt_name);
if (opt_info != nullptr) {
*opt_ptr = iter.opt_ptr;
return opt_info;
}
}
}
return nullptr;
@ -280,12 +286,14 @@ Status ConfigurableHelper::ConfigureOptions(
if (!opts_map.empty()) {
#ifndef ROCKSDB_LITE
for (const auto& iter : configurable.options_) {
s = ConfigureSomeOptions(config_options, configurable, *(iter.type_map),
&remaining, iter.opt_ptr);
if (remaining.empty()) { // Are there more options left?
break;
} else if (!s.ok()) {
break;
if (iter.type_map != nullptr) {
s = ConfigureSomeOptions(config_options, configurable, *(iter.type_map),
&remaining, iter.opt_ptr);
if (remaining.empty()) { // Are there more options left?
break;
} else if (!s.ok()) {
break;
}
}
}
#else
@ -573,36 +581,38 @@ Status ConfigurableHelper::SerializeOptions(const ConfigOptions& config_options,
std::string* result) {
assert(result);
for (auto const& opt_iter : configurable.options_) {
for (const auto& map_iter : *(opt_iter.type_map)) {
const auto& opt_name = map_iter.first;
const auto& opt_info = map_iter.second;
if (opt_info.ShouldSerialize()) {
std::string value;
Status s;
if (!config_options.mutable_options_only) {
s = opt_info.Serialize(config_options, prefix + opt_name,
opt_iter.opt_ptr, &value);
} else if (opt_info.IsMutable()) {
ConfigOptions copy = config_options;
copy.mutable_options_only = false;
s = opt_info.Serialize(copy, prefix + opt_name, opt_iter.opt_ptr,
&value);
} else if (opt_info.IsConfigurable()) {
// If it is a Configurable and we are either printing all of the
// details or not printing only the name, this option should be
// included in the list
if (config_options.IsDetailed() ||
!opt_info.IsEnabled(OptionTypeFlags::kStringNameOnly)) {
if (opt_iter.type_map != nullptr) {
for (const auto& map_iter : *(opt_iter.type_map)) {
const auto& opt_name = map_iter.first;
const auto& opt_info = map_iter.second;
if (opt_info.ShouldSerialize()) {
std::string value;
Status s;
if (!config_options.mutable_options_only) {
s = opt_info.Serialize(config_options, prefix + opt_name,
opt_iter.opt_ptr, &value);
} else if (opt_info.IsMutable()) {
ConfigOptions copy = config_options;
copy.mutable_options_only = false;
s = opt_info.Serialize(copy, prefix + opt_name, opt_iter.opt_ptr,
&value);
} else if (opt_info.IsConfigurable()) {
// If it is a Configurable and we are either printing all of the
// details or not printing only the name, this option should be
// included in the list
if (config_options.IsDetailed() ||
!opt_info.IsEnabled(OptionTypeFlags::kStringNameOnly)) {
s = opt_info.Serialize(config_options, prefix + opt_name,
opt_iter.opt_ptr, &value);
}
}
if (!s.ok()) {
return s;
} else if (!value.empty()) {
// <prefix><opt_name>=<value><delimiter>
result->append(prefix + opt_name + "=" + value +
config_options.delimiter);
}
}
if (!s.ok()) {
return s;
} else if (!value.empty()) {
// <prefix><opt_name>=<value><delimiter>
result->append(prefix + opt_name + "=" + value +
config_options.delimiter);
}
}
}
@ -629,16 +639,18 @@ Status ConfigurableHelper::ListOptions(
const std::string& prefix, std::unordered_set<std::string>* result) {
Status status;
for (auto const& opt_iter : configurable.options_) {
for (const auto& map_iter : *(opt_iter.type_map)) {
const auto& opt_name = map_iter.first;
const auto& opt_info = map_iter.second;
// If the option is no longer used in rocksdb and marked as deprecated,
// we skip it in the serialization.
if (!opt_info.IsDeprecated() && !opt_info.IsAlias()) {
if (!config_options.mutable_options_only) {
result->emplace(prefix + opt_name);
} else if (opt_info.IsMutable()) {
result->emplace(prefix + opt_name);
if (opt_iter.type_map != nullptr) {
for (const auto& map_iter : *(opt_iter.type_map)) {
const auto& opt_name = map_iter.first;
const auto& opt_info = map_iter.second;
// If the option is no longer used in rocksdb and marked as deprecated,
// we skip it in the serialization.
if (!opt_info.IsDeprecated() && !opt_info.IsAlias()) {
if (!config_options.mutable_options_only) {
result->emplace(prefix + opt_name);
} else if (opt_info.IsMutable()) {
result->emplace(prefix + opt_name);
}
}
}
}
@ -702,7 +714,7 @@ bool ConfigurableHelper::AreEquivalent(const ConfigOptions& config_options,
if (this_offset != that_offset) {
if (this_offset == nullptr || that_offset == nullptr) {
return false;
} else {
} else if (o.type_map != nullptr) {
for (const auto& map_iter : *(o.type_map)) {
const auto& opt_info = map_iter.second;
if (config_options.IsCheckEnabled(opt_info.GetSanityLevel())) {

24
options/configurable_test.cc
View File

@ -656,6 +656,30 @@ TEST_F(ConfigurableTest, TestNoCompare) {
ASSERT_TRUE(base->AreEquivalent(config_options_, copy.get(), &mismatch));
ASSERT_FALSE(copy->AreEquivalent(config_options_, base.get(), &mismatch));
}
TEST_F(ConfigurableTest, NullOptionMapTest) {
std::unique_ptr<Configurable> base;
std::unordered_set<std::string> names;
std::string str;
base.reset(
SimpleConfigurable::Create("c", TestConfigMode::kDefaultMode, nullptr));
ASSERT_NOK(base->ConfigureFromString(config_options_, "int=10"));
ASSERT_NOK(base->ConfigureFromString(config_options_, "int=20"));
ASSERT_NOK(base->ConfigureOption(config_options_, "int", "20"));
ASSERT_NOK(base->GetOption(config_options_, "int", &str));
ASSERT_NE(base->GetOptions<TestOptions>("c"), nullptr);
ASSERT_OK(base->GetOptionNames(config_options_, &names));
ASSERT_EQ(names.size(), 0UL);
ASSERT_OK(base->PrepareOptions(config_options_));
ASSERT_OK(base->ValidateOptions(DBOptions(), ColumnFamilyOptions()));
std::unique_ptr<Configurable> copy;
copy.reset(
SimpleConfigurable::Create("c", TestConfigMode::kDefaultMode, nullptr));
ASSERT_OK(base->GetOptionString(config_options_, &str));
ASSERT_OK(copy->ConfigureFromString(config_options_, str));
ASSERT_TRUE(base->AreEquivalent(config_options_, copy.get(), &str));
}
#endif
static std::unordered_map<std::string, ConfigTestFactoryFunc> TestFactories = {

82
options/customizable_test.cc
View File

@ -612,11 +612,20 @@ static std::unordered_map<std::string, OptionTypeInfo> inner_option_info = {
#endif // ROCKSDB_LITE
};
struct InnerOptions {
static const char* kName() { return "InnerOptions"; }
std::shared_ptr<Customizable> inner;
};
class InnerCustomizable : public Customizable {
public:
explicit InnerCustomizable(const std::shared_ptr<Customizable>& w)
: inner_(w) {}
explicit InnerCustomizable(const std::shared_ptr<Customizable>& w) {
iopts_.inner = w;
RegisterOptions(&iopts_, &inner_option_info);
}
static const char* kClassName() { return "Inner"; }
const char* Name() const override { return kClassName(); }
bool IsInstanceOf(const std::string& name) const override {
if (name == kClassName()) {
return true;
@ -626,26 +635,51 @@ class InnerCustomizable : public Customizable {
}
protected:
const Customizable* Inner() const override { return inner_.get(); }
const Customizable* Inner() const override { return iopts_.inner.get(); }
private:
std::shared_ptr<Customizable> inner_;
InnerOptions iopts_;
};
struct WrappedOptions1 {
static const char* kName() { return "WrappedOptions1"; }
int i = 42;
};
class WrappedCustomizable1 : public InnerCustomizable {
public:
explicit WrappedCustomizable1(const std::shared_ptr<Customizable>& w)
: InnerCustomizable(w) {}
: InnerCustomizable(w) {
RegisterOptions(&wopts_, nullptr);
}
const char* Name() const override { return kClassName(); }
static const char* kClassName() { return "Wrapped1"; }
private:
WrappedOptions1 wopts_;
};
struct WrappedOptions2 {
static const char* kName() { return "WrappedOptions2"; }
std::string s = "42";
};
class WrappedCustomizable2 : public InnerCustomizable {
public:
explicit WrappedCustomizable2(const std::shared_ptr<Customizable>& w)
: InnerCustomizable(w) {}
const void* GetOptionsPtr(const std::string& name) const override {
if (name == WrappedOptions2::kName()) {
return &wopts_;
} else {
return InnerCustomizable::GetOptionsPtr(name);
}
}
const char* Name() const override { return kClassName(); }
static const char* kClassName() { return "Wrapped2"; }
private:
WrappedOptions2 wopts_;
};
} // namespace
@ -677,6 +711,29 @@ TEST_F(CustomizableTest, WrappedInnerTest) {
ASSERT_EQ(wc2->CheckedCast<TestCustomizable>(), ac.get());
}
TEST_F(CustomizableTest, CustomizableInnerTest) {
std::shared_ptr<Customizable> c =
std::make_shared<InnerCustomizable>(std::make_shared<ACustomizable>("a"));
std::shared_ptr<Customizable> wc1 = std::make_shared<WrappedCustomizable1>(c);
std::shared_ptr<Customizable> wc2 = std::make_shared<WrappedCustomizable2>(c);
auto inner = c->GetOptions<InnerOptions>();
ASSERT_NE(inner, nullptr);
auto aopts = c->GetOptions<AOptions>();
ASSERT_NE(aopts, nullptr);
ASSERT_EQ(aopts, wc1->GetOptions<AOptions>());
ASSERT_EQ(aopts, wc2->GetOptions<AOptions>());
auto w1opts = wc1->GetOptions<WrappedOptions1>();
ASSERT_NE(w1opts, nullptr);
ASSERT_EQ(c->GetOptions<WrappedOptions1>(), nullptr);
ASSERT_EQ(wc2->GetOptions<WrappedOptions1>(), nullptr);
auto w2opts = wc2->GetOptions<WrappedOptions2>();
ASSERT_NE(w2opts, nullptr);
ASSERT_EQ(c->GetOptions<WrappedOptions2>(), nullptr);
ASSERT_EQ(wc1->GetOptions<WrappedOptions2>(), nullptr);
}
TEST_F(CustomizableTest, CopyObjectTest) {
class CopyCustomizable : public Customizable {
public:
@ -714,20 +771,9 @@ TEST_F(CustomizableTest, CopyObjectTest) {
}
TEST_F(CustomizableTest, TestStringDepth) {
class ShallowCustomizable : public Customizable {
public:
ShallowCustomizable() {
inner_ = std::make_shared<ACustomizable>("a");
RegisterOptions("inner", &inner_, &inner_option_info);
}
static const char* kClassName() { return "shallow"; }
const char* Name() const override { return kClassName(); }
private:
std::shared_ptr<TestCustomizable> inner_;
};
ConfigOptions shallow = config_options_;
std::unique_ptr<Configurable> c(new ShallowCustomizable());
std::unique_ptr<Configurable> c(
new InnerCustomizable(std::make_shared<ACustomizable>("a")));
std::string opt_str;
shallow.depth = ConfigOptions::Depth::kDepthShallow;
ASSERT_OK(c->GetOptionString(shallow, &opt_str));