From 63a2215c636e53bac16e439eb6ecc5355a024c69 Mon Sep 17 00:00:00 2001 From: Yueh-Hsuan Chiang Date: Wed, 20 Aug 2014 15:53:39 -0700 Subject: [PATCH] Improve Options sanitization and add MmapReadRequired() to TableFactory Summary: Currently, PlainTable must use mmap_reads. When PlainTable is used but allow_mmap_reads is not set, rocksdb will fail in flush. This diff improve Options sanitization and add MmapReadRequired() to TableFactory. Test Plan: export ROCKSDB_TESTS=PlainTableOptionsSanitizeTest make db_test -j32 ./db_test Reviewers: sdong, ljin Reviewed By: ljin Subscribers: you, leveldb Differential Revision: https://reviews.facebook.net/D21939 --- db/db_impl.cc | 19 ++++++++++++++++++- db/db_test.cc | 12 ++++++++++++ include/rocksdb/table.h | 6 ++++++ table/adaptive_table_factory.h | 10 ++++++++++ table/block_based_table_factory.h | 5 +++++ table/cuckoo_table_factory.h | 5 +++++ table/plain_table_factory.h | 9 +++++++++ 7 files changed, 65 insertions(+), 1 deletion(-) diff --git a/db/db_impl.cc b/db/db_impl.cc index 5ee92759d..4cef9381c 100644 --- a/db/db_impl.cc +++ b/db/db_impl.cc @@ -291,6 +291,19 @@ DBOptions SanitizeOptions(const std::string& dbname, const DBOptions& src) { return result; } +Status SanitizeDBOptionsByCFOptions( + DBOptions* db_opts, + const std::vector& column_families) { + Status s; + for (auto cf : column_families) { + s = cf.options.table_factory->SanitizeDBOptions(db_opts); + if (!s.ok()) { + return s; + } + } + return Status::OK(); +} + namespace { CompressionType GetCompressionFlush(const Options& options) { // Compressing memtable flushes might not help unless the sequential load @@ -4743,7 +4756,11 @@ Status DB::Open(const Options& options, const std::string& dbname, DB** dbptr) { column_families.push_back( ColumnFamilyDescriptor(kDefaultColumnFamilyName, cf_options)); std::vector handles; - Status s = DB::Open(db_options, dbname, column_families, &handles, dbptr); + Status s = SanitizeDBOptionsByCFOptions(&db_options, column_families); + if (!s.ok()) { + return s; + } + s = DB::Open(db_options, dbname, column_families, &handles, dbptr); if (s.ok()) { assert(handles.size() == 1); // i can delete the handle since DBImpl is always holding a reference to diff --git a/db/db_test.cc b/db/db_test.cc index 5702f7798..37a4c6c9e 100644 --- a/db/db_test.cc +++ b/db/db_test.cc @@ -7679,6 +7679,18 @@ TEST(DBTest, RateLimitingTest) { ASSERT_TRUE(ratio < 0.6); } +TEST(DBTest, TableOptionsSanitizeTest) { + Options options = CurrentOptions(); + options.create_if_missing = true; + DestroyAndReopen(&options); + ASSERT_EQ(db_->GetOptions().allow_mmap_reads, false); + + options.table_factory.reset(new PlainTableFactory()); + options.prefix_extractor.reset(NewNoopTransform()); + Destroy(&options); + ASSERT_TRUE(TryReopen(&options).IsNotSupported()); +} + } // namespace rocksdb int main(int argc, char** argv) { diff --git a/include/rocksdb/table.h b/include/rocksdb/table.h index 6a73239e8..9fd0e5591 100644 --- a/include/rocksdb/table.h +++ b/include/rocksdb/table.h @@ -253,6 +253,12 @@ class TableFactory { virtual TableBuilder* NewTableBuilder( const Options& options, const InternalKeyComparator& internal_comparator, WritableFile* file, CompressionType compression_type) const = 0; + + // Sanitizes the specified DB Options. + // + // If the function cannot find a way to sanitize the input DB Options, + // a non-ok Status will be returned. + virtual Status SanitizeDBOptions(DBOptions* db_opts) const = 0; }; #ifndef ROCKSDB_LITE diff --git a/table/adaptive_table_factory.h b/table/adaptive_table_factory.h index d89826640..fc6b02653 100644 --- a/table/adaptive_table_factory.h +++ b/table/adaptive_table_factory.h @@ -41,6 +41,16 @@ class AdaptiveTableFactory : public TableFactory { CompressionType compression_type) const override; + // Sanitizes the specified DB Options. + Status SanitizeDBOptions(DBOptions* db_opts) const override { + if (db_opts->allow_mmap_reads == false) { + return Status::NotSupported( + "AdaptiveTable with allow_mmap_reads == false is not supported."); + } + return Status::OK(); + } + + private: std::shared_ptr table_factory_to_write_; std::shared_ptr block_based_table_factory_; diff --git a/table/block_based_table_factory.h b/table/block_based_table_factory.h index 656b531ae..99d306c3d 100644 --- a/table/block_based_table_factory.h +++ b/table/block_based_table_factory.h @@ -43,6 +43,11 @@ class BlockBasedTableFactory : public TableFactory { const Options& options, const InternalKeyComparator& internal_comparator, WritableFile* file, CompressionType compression_type) const override; + // Sanitizes the specified DB Options. + Status SanitizeDBOptions(DBOptions* db_opts) const override { + return Status::OK(); + } + private: BlockBasedTableOptions table_options_; }; diff --git a/table/cuckoo_table_factory.h b/table/cuckoo_table_factory.h index a4b670a1f..7402e4821 100644 --- a/table/cuckoo_table_factory.h +++ b/table/cuckoo_table_factory.h @@ -40,6 +40,11 @@ class CuckooTableFactory : public TableFactory { const InternalKeyComparator& icomparator, WritableFile* file, CompressionType compression_type) const override; + // Sanitizes the specified DB Options. + Status SanitizeDBOptions(DBOptions* db_opts) const override { + return Status::OK(); + } + private: const double hash_table_ratio_; const uint32_t max_search_depth_; diff --git a/table/plain_table_factory.h b/table/plain_table_factory.h index ed54c4d10..ba3f9f813 100644 --- a/table/plain_table_factory.h +++ b/table/plain_table_factory.h @@ -166,6 +166,15 @@ class PlainTableFactory : public TableFactory { static const char kValueTypeSeqId0 = 0xFF; + // Sanitizes the specified DB Options. + Status SanitizeDBOptions(DBOptions* db_opts) const override { + if (db_opts->allow_mmap_reads == false) { + return Status::NotSupported( + "PlainTable with allow_mmap_reads == false is not supported."); + } + return Status::OK(); + } + private: uint32_t user_key_len_; int bloom_bits_per_key_;