diff --git a/include/rocksdb/comparator.h b/include/rocksdb/comparator.h index fe96eb0c7..37c2925bc 100644 --- a/include/rocksdb/comparator.h +++ b/include/rocksdb/comparator.h @@ -10,7 +10,6 @@ #include -#include "rocksdb/customizable.h" #include "rocksdb/rocksdb_namespace.h" namespace ROCKSDB_NAMESPACE { @@ -21,7 +20,7 @@ class Slice; // used as keys in an sstable or a database. A Comparator implementation // must be thread-safe since rocksdb may invoke its methods concurrently // from multiple threads. -class Comparator : public Customizable { +class Comparator { public: Comparator() : timestamp_size_(0) {} @@ -38,11 +37,7 @@ class Comparator : public Customizable { virtual ~Comparator() {} - static Status CreateFromString(const ConfigOptions& opts, - const std::string& id, - const Comparator** comp); static const char* Type() { return "Comparator"; } - // Three-way comparison. Returns value: // < 0 iff "a" < "b", // == 0 iff "a" == "b", diff --git a/include/rocksdb/configurable.h b/include/rocksdb/configurable.h index 78ca771ba..b56072dbe 100644 --- a/include/rocksdb/configurable.h +++ b/include/rocksdb/configurable.h @@ -8,7 +8,6 @@ #pragma once -#include #include #include #include @@ -270,7 +269,7 @@ class Configurable { protected: // True once the object is prepared. Once the object is prepared, only // mutable options can be configured. - std::atomic prepared_; + bool prepared_; // Returns the raw pointer for the associated named option. // The name is typically the name of an option registered via the diff --git a/include/rocksdb/utilities/options_type.h b/include/rocksdb/utilities/options_type.h index d7d388985..7057c78ac 100644 --- a/include/rocksdb/utilities/options_type.h +++ b/include/rocksdb/utilities/options_type.h @@ -35,6 +35,7 @@ enum class OptionType { kCompactionPri, kSliceTransform, kCompressionType, + kComparator, kCompactionFilter, kCompactionFilterFactory, kCompactionStopStyle, diff --git a/options/cf_options.cc b/options/cf_options.cc index 3f2b8bb73..005a90c85 100644 --- a/options/cf_options.cc +++ b/options/cf_options.cc @@ -535,30 +535,22 @@ static std::unordered_map OptionVerificationType::kNormal, OptionTypeFlags::kNone, {0, OptionType::kCompressionType})}, {"comparator", - OptionTypeInfo::AsCustomRawPtr( - offset_of(&ImmutableCFOptions::user_comparator), - OptionVerificationType::kByName, OptionTypeFlags::kCompareLoose, - // Serializes a Comparator - [](const ConfigOptions& /*opts*/, const std::string&, - const void* addr, std::string* value) { - // it's a const pointer of const Comparator* - const auto* ptr = static_cast(addr); - - // Since the user-specified comparator will be wrapped by - // InternalKeyComparator, we should persist the user-specified - // one instead of InternalKeyComparator. - if (*ptr == nullptr) { - *value = kNullptrString; - } else { - const Comparator* root_comp = (*ptr)->GetRootComparator(); - if (root_comp == nullptr) { - root_comp = (*ptr); - } - *value = root_comp->Name(); - } - return Status::OK(); - }, - /* Use the default match function*/ nullptr)}, + {offset_of(&ImmutableCFOptions::user_comparator), + OptionType::kComparator, OptionVerificationType::kByName, + OptionTypeFlags::kCompareLoose, + // Parses the string and sets the corresponding comparator + [](const ConfigOptions& opts, const std::string& /*name*/, + const std::string& value, void* addr) { + auto old_comparator = static_cast(addr); + const Comparator* new_comparator = *old_comparator; + Status status = + opts.registry->NewStaticObject(value, &new_comparator); + if (status.ok()) { + *old_comparator = new_comparator; + return status; + } + return Status::OK(); + }}}, {"memtable_insert_with_hint_prefix_extractor", {offset_of( &ImmutableCFOptions::memtable_insert_with_hint_prefix_extractor), diff --git a/options/customizable_test.cc b/options/customizable_test.cc index 4dc648e80..d48ed1040 100644 --- a/options/customizable_test.cc +++ b/options/customizable_test.cc @@ -771,15 +771,6 @@ static int RegisterTestObjects(ObjectLibrary& library, guard->reset(new mock::MockTableFactory()); return guard->get(); }); - library.Register( - test::SimpleSuffixReverseComparator::kClassName(), - [](const std::string& /*uri*/, - std::unique_ptr* /*guard*/, - std::string* /* errmsg */) { - static test::SimpleSuffixReverseComparator ssrc; - return &ssrc; - }); - return static_cast(library.GetFactoryCount(&num_types)); } @@ -789,7 +780,6 @@ static int RegisterLocalObjects(ObjectLibrary& library, // Load any locally defined objects here return static_cast(library.GetFactoryCount(&num_types)); } -#endif // !ROCKSDB_LITE class LoadCustomizableTest : public testing::Test { public: @@ -829,31 +819,7 @@ TEST_F(LoadCustomizableTest, LoadTableFactoryTest) { ASSERT_STREQ(factory->Name(), "MockTable"); } } - -TEST_F(LoadCustomizableTest, LoadComparatorTest) { - const Comparator* bytewise = BytewiseComparator(); - const Comparator* reverse = ReverseBytewiseComparator(); - - const Comparator* result = nullptr; - ASSERT_NOK(Comparator::CreateFromString( - config_options_, test::SimpleSuffixReverseComparator::kClassName(), - &result)); - ASSERT_OK( - Comparator::CreateFromString(config_options_, bytewise->Name(), &result)); - ASSERT_EQ(result, bytewise); - ASSERT_OK( - Comparator::CreateFromString(config_options_, reverse->Name(), &result)); - ASSERT_EQ(result, reverse); - - if (RegisterTests("Test")) { - ASSERT_OK(Comparator::CreateFromString( - config_options_, test::SimpleSuffixReverseComparator::kClassName(), - &result)); - ASSERT_NE(result, nullptr); - ASSERT_STREQ(result->Name(), - test::SimpleSuffixReverseComparator::kClassName()); - } -} +#endif // !ROCKSDB_LITE } // namespace ROCKSDB_NAMESPACE int main(int argc, char** argv) { diff --git a/options/options_helper.cc b/options/options_helper.cc index a554ca358..823ef4a45 100644 --- a/options/options_helper.cc +++ b/options/options_helper.cc @@ -562,6 +562,23 @@ bool SerializeSingleOptionHelper(const void* opt_address, : kNullptrString; break; } + case OptionType::kComparator: { + // it's a const pointer of const Comparator* + const auto* ptr = static_cast(opt_address); + // Since the user-specified comparator will be wrapped by + // InternalKeyComparator, we should persist the user-specified one + // instead of InternalKeyComparator. + if (*ptr == nullptr) { + *value = kNullptrString; + } else { + const Comparator* root_comp = (*ptr)->GetRootComparator(); + if (root_comp == nullptr) { + root_comp = (*ptr); + } + *value = root_comp->Name(); + } + break; + } case OptionType::kCompactionFilter: { // it's a const pointer of const CompactionFilter* const auto* ptr = diff --git a/test_util/testutil.h b/test_util/testutil.h index c1dc09b0c..ae6d1dec4 100644 --- a/test_util/testutil.h +++ b/test_util/testutil.h @@ -98,8 +98,10 @@ class PlainInternalKeyComparator : public InternalKeyComparator { class SimpleSuffixReverseComparator : public Comparator { public: SimpleSuffixReverseComparator() {} - static const char* kClassName() { return "SimpleSuffixReverseComparator"; } - virtual const char* Name() const override { return kClassName(); } + + virtual const char* Name() const override { + return "SimpleSuffixReverseComparator"; + } virtual int Compare(const Slice& a, const Slice& b) const override { Slice prefix_a = Slice(a.data(), 8); diff --git a/util/comparator.cc b/util/comparator.cc index 0ed391f9b..f115a73e9 100644 --- a/util/comparator.cc +++ b/util/comparator.cc @@ -8,17 +8,11 @@ // found in the LICENSE file. See the AUTHORS file for names of contributors. #include "rocksdb/comparator.h" - #include - #include #include -#include - -#include "options/configurable_helper.h" #include "port/port.h" #include "rocksdb/slice.h" -#include "rocksdb/utilities/object_registry.h" namespace ROCKSDB_NAMESPACE { @@ -26,8 +20,8 @@ namespace { class BytewiseComparatorImpl : public Comparator { public: BytewiseComparatorImpl() { } - static const char* kClassName() { return "leveldb.BytewiseComparator"; } - const char* Name() const override { return kClassName(); } + + const char* Name() const override { return "leveldb.BytewiseComparator"; } int Compare(const Slice& a, const Slice& b) const override { return a.compare(b); @@ -145,10 +139,9 @@ class ReverseBytewiseComparatorImpl : public BytewiseComparatorImpl { public: ReverseBytewiseComparatorImpl() { } - static const char* kClassName() { + const char* Name() const override { return "rocksdb.ReverseBytewiseComparator"; } - const char* Name() const override { return kClassName(); } int Compare(const Slice& a, const Slice& b) const override { return -a.compare(b); @@ -227,77 +220,4 @@ const Comparator* ReverseBytewiseComparator() { return &rbytewise; } -#ifndef ROCKSDB_LITE -static int RegisterBuiltinComparators(ObjectLibrary& library, - const std::string& /*arg*/) { - library.Register( - BytewiseComparatorImpl::kClassName(), - [](const std::string& /*uri*/, - std::unique_ptr* /*guard */, - std::string* /* errmsg */) { return BytewiseComparator(); }); - library.Register( - ReverseBytewiseComparatorImpl::kClassName(), - [](const std::string& /*uri*/, - std::unique_ptr* /*guard */, - std::string* /* errmsg */) { return ReverseBytewiseComparator(); }); - return 2; -} -#endif // ROCKSDB_LITE - -Status Comparator::CreateFromString(const ConfigOptions& config_options, - const std::string& value, - const Comparator** result) { -#ifndef ROCKSDB_LITE - static std::once_flag once; - std::call_once(once, [&]() { - RegisterBuiltinComparators(*(ObjectLibrary::Default().get()), ""); - }); -#endif // ROCKSDB_LITE - std::string id; - std::unordered_map opt_map; - Status status = - ConfigurableHelper::GetOptionsMap(value, *result, &id, &opt_map); - if (!status.ok()) { // GetOptionsMap failed - return status; - } - std::string curr_opts; -#ifndef ROCKSDB_LITE - if (*result != nullptr && (*result)->GetId() == id) { - // Try to get the existing options, ignoring any errors - ConfigOptions embedded = config_options; - embedded.delimiter = ";"; - (*result)->GetOptionString(embedded, &curr_opts).PermitUncheckedError(); - } -#endif - if (id == BytewiseComparatorImpl::kClassName()) { - *result = BytewiseComparator(); - } else if (id == ReverseBytewiseComparatorImpl::kClassName()) { - *result = ReverseBytewiseComparator(); - } else if (value.empty()) { - // No Id and no options. Clear the object - *result = nullptr; - return Status::OK(); - } else if (id.empty()) { // We have no Id but have options. Not good - return Status::NotSupported("Cannot reset object ", id); - } else { -#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 (!status.ok()) { - if (config_options.ignore_unsupported_options && - status.IsNotSupported()) { - return Status::OK(); - } else { - return status; - } - } else if (!curr_opts.empty() || !opt_map.empty()) { - Comparator* comparator = const_cast(*result); - status = ConfigurableHelper::ConfigureNewObject( - config_options, comparator, id, curr_opts, opt_map); - } - } - return status; -} } // namespace ROCKSDB_NAMESPACE