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

Make Configurable/Customizable options copyable (#8704)

Browse Source 
Summary:
The atomic variable "is_prepared_" was keeping Configurable objects from being copy-constructed.  Removed the atomic to allow copies.

Since the variable is only changed from false to true (and never back), there is no reason it had to be atomic.

Added tests that simple Configurable and Customizable objects can be put on the stack and copied.

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

Reviewed By: anand1976

Differential Revision: D30530526

Pulled By: ltamasi

fbshipit-source-id: 4dd4439b3e5ad7fa396573d0b25d9fb709160576
This commit is contained in:
mrambacher 2021-08-25 17:46:31 -07:00 committed by Levi Tamasi
parent 3a3f4801d0
commit 3efc6fd641
5 changed files with 70 additions and 35 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

9
include/rocksdb/configurable.h
View File

@ -56,7 +56,6 @@ class Configurable {
}; };
public: public:
Configurable();
virtual ~Configurable() {} virtual ~Configurable() {}
// Returns the raw pointer of the named options that is used by this // Returns the raw pointer of the named options that is used by this
@ -262,10 +261,6 @@ class Configurable {
virtual Status ValidateOptions(const DBOptions& db_opts, virtual Status ValidateOptions(const DBOptions& db_opts,
const ColumnFamilyOptions& cf_opts) const; const ColumnFamilyOptions& cf_opts) const;
// Returns true if this object has been initialized via PrepareOptions, false
// otherwise. Once an object has been prepared, only mutable options may be
// changed.
virtual bool IsPrepared() const { return is_prepared_; }
// Splits the input opt_value into the ID field and the remaining options. // 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 // The input opt_value can be in the form of "name" or "name=value
@ -286,10 +281,6 @@ class Configurable {
std::string* id, std::unordered_map<std::string, std::string>* options); std::string* id, std::unordered_map<std::string, std::string>* options);
protected: protected:
// True once the object is prepared. Once the object is prepared, only
// mutable options can be configured.
std::atomic<bool> is_prepared_;
// Returns the raw pointer for the associated named option. // Returns the raw pointer for the associated named option.
// The name is typically the name of an option registered via the // The name is typically the name of an option registered via the
// Classes may override this method to provide further specialization (such as // Classes may override this method to provide further specialization (such as

5
options/configurable.cc
View File

@ -17,8 +17,6 @@
namespace ROCKSDB_NAMESPACE { namespace ROCKSDB_NAMESPACE {
Configurable::Configurable() : is_prepared_(false) {}
void Configurable::RegisterOptions( void Configurable::RegisterOptions(
const std::string& name, void* opt_ptr, const std::string& name, void* opt_ptr,
const std::unordered_map<std::string, OptionTypeInfo>* type_map) { const std::unordered_map<std::string, OptionTypeInfo>* type_map) {
@ -68,9 +66,6 @@ Status Configurable::PrepareOptions(const ConfigOptions& opts) {
#else #else
(void)opts; (void)opts;
#endif // ROCKSDB_LITE #endif // ROCKSDB_LITE
if (status.ok()) {
is_prepared_ = true;
}
return status; return status;
} }

34
options/configurable_test.cc
View File

@ -331,6 +331,40 @@ TEST_F(ConfigurableTest, PrepareOptionsTest) {
ASSERT_EQ(*up, 0); ASSERT_EQ(*up, 0);
} }
TEST_F(ConfigurableTest, CopyObjectTest) {
class CopyConfigurable : public Configurable {
public:
CopyConfigurable() : prepared_(0), validated_(0) {}
Status PrepareOptions(const ConfigOptions& options) override {
prepared_++;
return Configurable::PrepareOptions(options);
}
Status ValidateOptions(const DBOptions& db_opts,
const ColumnFamilyOptions& cf_opts) const override {
validated_++;
return Configurable::ValidateOptions(db_opts, cf_opts);
}
int prepared_;
mutable int validated_;
};
CopyConfigurable c1;
ConfigOptions config_options;
Options options;
ASSERT_OK(c1.PrepareOptions(config_options));
ASSERT_OK(c1.ValidateOptions(options, options));
ASSERT_EQ(c1.prepared_, 1);
ASSERT_EQ(c1.validated_, 1);
CopyConfigurable c2 = c1;
ASSERT_OK(c1.PrepareOptions(config_options));
ASSERT_OK(c1.ValidateOptions(options, options));
ASSERT_EQ(c2.prepared_, 1);
ASSERT_EQ(c2.validated_, 1);
ASSERT_EQ(c1.prepared_, 2);
ASSERT_EQ(c1.validated_, 2);
}
TEST_F(ConfigurableTest, MutableOptionsTest) { TEST_F(ConfigurableTest, MutableOptionsTest) {
static std::unordered_map<std::string, OptionTypeInfo> imm_option_info = { static std::unordered_map<std::string, OptionTypeInfo> imm_option_info = {
#ifndef ROCKSDB_LITE #ifndef ROCKSDB_LITE

55
options/customizable_test.cc
View File

@ -553,7 +553,6 @@ TEST_F(CustomizableTest, PrepareOptionsTest) {
ConfigOptions prepared(config_options_); ConfigOptions prepared(config_options_);
prepared.invoke_prepare_options = true; prepared.invoke_prepare_options = true;
ASSERT_FALSE(base->IsPrepared());
ASSERT_OK(base->ConfigureFromString( ASSERT_OK(base->ConfigureFromString(
prepared, "unique=A_1; shared={id=B;string=s}; pointer.id=S")); prepared, "unique=A_1; shared={id=B;string=s}; pointer.id=S"));
SimpleOptions* simple = base->GetOptions<SimpleOptions>(); SimpleOptions* simple = base->GetOptions<SimpleOptions>();
@ -561,10 +560,6 @@ TEST_F(CustomizableTest, PrepareOptionsTest) {
ASSERT_NE(simple->cu, nullptr); ASSERT_NE(simple->cu, nullptr);
ASSERT_NE(simple->cs, nullptr); ASSERT_NE(simple->cs, nullptr);
ASSERT_NE(simple->cp, nullptr); ASSERT_NE(simple->cp, nullptr);
ASSERT_TRUE(base->IsPrepared());
ASSERT_TRUE(simple->cu->IsPrepared());
ASSERT_TRUE(simple->cs->IsPrepared());
ASSERT_TRUE(simple->cp->IsPrepared());
delete simple->cp; delete simple->cp;
base.reset(new SimpleConfigurable()); base.reset(new SimpleConfigurable());
ASSERT_OK(base->ConfigureFromString( ASSERT_OK(base->ConfigureFromString(
@ -575,16 +570,8 @@ TEST_F(CustomizableTest, PrepareOptionsTest) {
ASSERT_NE(simple->cu, nullptr); ASSERT_NE(simple->cu, nullptr);
ASSERT_NE(simple->cs, nullptr); ASSERT_NE(simple->cs, nullptr);
ASSERT_NE(simple->cp, nullptr); ASSERT_NE(simple->cp, nullptr);
ASSERT_FALSE(base->IsPrepared());
ASSERT_FALSE(simple->cu->IsPrepared());
ASSERT_FALSE(simple->cs->IsPrepared());
ASSERT_FALSE(simple->cp->IsPrepared());
ASSERT_OK(base->PrepareOptions(config_options_)); ASSERT_OK(base->PrepareOptions(config_options_));
ASSERT_TRUE(base->IsPrepared());
ASSERT_TRUE(simple->cu->IsPrepared());
ASSERT_TRUE(simple->cs->IsPrepared());
ASSERT_TRUE(simple->cp->IsPrepared());
delete simple->cp; delete simple->cp;
base.reset(new SimpleConfigurable()); base.reset(new SimpleConfigurable());
simple = base->GetOptions<SimpleOptions>(); simple = base->GetOptions<SimpleOptions>();
@ -597,21 +584,16 @@ TEST_F(CustomizableTest, PrepareOptionsTest) {
ASSERT_OK( ASSERT_OK(
base->ConfigureFromString(prepared, "unique={id=P; can_prepare=true}")); base->ConfigureFromString(prepared, "unique={id=P; can_prepare=true}"));
ASSERT_NE(simple->cu, nullptr); ASSERT_NE(simple->cu, nullptr);
ASSERT_TRUE(simple->cu->IsPrepared());
ASSERT_OK(base->ConfigureFromString(config_options_, ASSERT_OK(base->ConfigureFromString(config_options_,
"unique={id=P; can_prepare=true}")); "unique={id=P; can_prepare=true}"));
ASSERT_NE(simple->cu, nullptr); ASSERT_NE(simple->cu, nullptr);
ASSERT_FALSE(simple->cu->IsPrepared());
ASSERT_OK(simple->cu->PrepareOptions(prepared)); ASSERT_OK(simple->cu->PrepareOptions(prepared));
ASSERT_TRUE(simple->cu->IsPrepared());
ASSERT_OK(base->ConfigureFromString(config_options_, ASSERT_OK(base->ConfigureFromString(config_options_,
"unique={id=P; can_prepare=false}")); "unique={id=P; can_prepare=false}"));
ASSERT_NE(simple->cu, nullptr); ASSERT_NE(simple->cu, nullptr);
ASSERT_FALSE(simple->cu->IsPrepared());
ASSERT_NOK(simple->cu->PrepareOptions(prepared)); ASSERT_NOK(simple->cu->PrepareOptions(prepared));
ASSERT_FALSE(simple->cu->IsPrepared());
} }
namespace { namespace {
@ -688,6 +670,42 @@ TEST_F(CustomizableTest, WrappedInnerTest) {
ASSERT_EQ(wc2->CheckedCast<TestCustomizable>(), ac.get()); ASSERT_EQ(wc2->CheckedCast<TestCustomizable>(), ac.get());
} }
TEST_F(CustomizableTest, CopyObjectTest) {
class CopyCustomizable : public Customizable {
public:
CopyCustomizable() : prepared_(0), validated_(0) {}
const char* Name() const override { return "CopyCustomizable"; }
Status PrepareOptions(const ConfigOptions& options) override {
prepared_++;
return Customizable::PrepareOptions(options);
}
Status ValidateOptions(const DBOptions& db_opts,
const ColumnFamilyOptions& cf_opts) const override {
validated_++;
return Customizable::ValidateOptions(db_opts, cf_opts);
}
int prepared_;
mutable int validated_;
};
CopyCustomizable c1;
ConfigOptions config_options;
Options options;
ASSERT_OK(c1.PrepareOptions(config_options));
ASSERT_OK(c1.ValidateOptions(options, options));
ASSERT_EQ(c1.prepared_, 1);
ASSERT_EQ(c1.validated_, 1);
CopyCustomizable c2 = c1;
ASSERT_OK(c1.PrepareOptions(config_options));
ASSERT_OK(c1.ValidateOptions(options, options));
ASSERT_EQ(c2.prepared_, 1);
ASSERT_EQ(c2.validated_, 1);
ASSERT_EQ(c1.prepared_, 2);
ASSERT_EQ(c1.validated_, 2);
}
TEST_F(CustomizableTest, TestStringDepth) { TEST_F(CustomizableTest, TestStringDepth) {
class ShallowCustomizable : public Customizable { class ShallowCustomizable : public Customizable {
public: public:
@ -983,7 +1001,6 @@ TEST_F(CustomizableTest, MutableOptionsTest) {
std::string opt_str; std::string opt_str;
ConfigOptions options = config_options_; ConfigOptions options = config_options_;
ASSERT_FALSE(mc.IsPrepared());
ASSERT_OK(mc.ConfigureOption(options, "mutable", "{id=B;}")); ASSERT_OK(mc.ConfigureOption(options, "mutable", "{id=B;}"));
options.mutable_options_only = true; options.mutable_options_only = true;
ASSERT_OK(mc.GetOptionString(options, &opt_str)); ASSERT_OK(mc.GetOptionString(options, &opt_str));

2
options/options_test.cc
View File

@ -1482,13 +1482,11 @@ TEST_F(OptionsTest, MutableTableOptions) {
bbtf.reset(NewBlockBasedTableFactory()); bbtf.reset(NewBlockBasedTableFactory());
auto bbto = bbtf->GetOptions<BlockBasedTableOptions>(); auto bbto = bbtf->GetOptions<BlockBasedTableOptions>();
ASSERT_NE(bbto, nullptr); ASSERT_NE(bbto, nullptr);
ASSERT_FALSE(bbtf->IsPrepared());
ASSERT_OK(bbtf->ConfigureOption(config_options, "block_align", "true")); ASSERT_OK(bbtf->ConfigureOption(config_options, "block_align", "true"));
ASSERT_OK(bbtf->ConfigureOption(config_options, "block_size", "1024")); ASSERT_OK(bbtf->ConfigureOption(config_options, "block_size", "1024"));
ASSERT_EQ(bbto->block_align, true); ASSERT_EQ(bbto->block_align, true);
ASSERT_EQ(bbto->block_size, 1024); ASSERT_EQ(bbto->block_size, 1024);
ASSERT_OK(bbtf->PrepareOptions(config_options)); ASSERT_OK(bbtf->PrepareOptions(config_options));
ASSERT_TRUE(bbtf->IsPrepared());
config_options.mutable_options_only = true; config_options.mutable_options_only = true;
ASSERT_OK(bbtf->ConfigureOption(config_options, "block_size", "1024")); ASSERT_OK(bbtf->ConfigureOption(config_options, "block_size", "1024"));
ASSERT_EQ(bbto->block_align, true); ASSERT_EQ(bbto->block_align, true);