From bf86af5174cb703b2671c55b2f153037742bddbe Mon Sep 17 00:00:00 2001 From: kailiu Date: Fri, 28 Feb 2014 16:39:27 -0800 Subject: [PATCH] Remove the terrible hack in for flush_block_policy_factory Summary: Previous code is too convoluted and I must be drunk for letting such code to be written without a second thought. Thanks to the discussion with @sdong, I added the `Options` when generating the flusher, thus avoiding the tricks. Just FYI: I resisted to add Options in flush_block_policy.h since I wanted to avoid cyclic dependencies: FlushBlockPolicy dpends on Options and Options also depends FlushBlockPolicy... While I appreciate my effort to prevent it, the old design turns out creating more troubles than it tried to avoid. Test Plan: ran ./table_test Reviewers: sdong Reviewed By: sdong CC: sdong, leveldb Differential Revision: https://reviews.facebook.net/D16503 --- db/version_set.cc | 3 +-- include/rocksdb/flush_block_policy.h | 14 ++++------- table/block_based_table_builder.cc | 4 ++-- table/block_based_table_factory.cc | 35 +++++++++------------------- table/block_based_table_factory.h | 3 +-- table/flush_block_policy.cc | 8 +++---- table/table_test.cc | 3 +-- 7 files changed, 24 insertions(+), 46 deletions(-) diff --git a/db/version_set.cc b/db/version_set.cc index b4c122970..18162040b 100644 --- a/db/version_set.cc +++ b/db/version_set.cc @@ -1553,8 +1553,7 @@ Status VersionSet::LogAndApply(VersionEdit* edit, port::Mutex* mu, // only one thread can be here at the same time if (!new_manifest_filename.empty()) { unique_ptr descriptor_file; - s = env_->NewWritableFile(new_manifest_filename, - &descriptor_file, + s = env_->NewWritableFile(new_manifest_filename, &descriptor_file, storage_options_.AdaptForLogWrite()); if (s.ok()) { descriptor_log_.reset(new log::Writer(std::move(descriptor_file))); diff --git a/include/rocksdb/flush_block_policy.h b/include/rocksdb/flush_block_policy.h index 1740d879c..8340ad616 100644 --- a/include/rocksdb/flush_block_policy.h +++ b/include/rocksdb/flush_block_policy.h @@ -11,6 +11,7 @@ namespace rocksdb { class Slice; class BlockBuilder; +struct Options; // FlushBlockPolicy provides a configurable way to determine when to flush a // block in the block based tables, @@ -36,29 +37,22 @@ class FlushBlockPolicyFactory { // Callers must delete the result after any database that is using the // result has been closed. virtual FlushBlockPolicy* NewFlushBlockPolicy( - const BlockBuilder& data_block_builder) const = 0; + const Options& options, const BlockBuilder& data_block_builder) const = 0; virtual ~FlushBlockPolicyFactory() { } }; class FlushBlockBySizePolicyFactory : public FlushBlockPolicyFactory { public: - FlushBlockBySizePolicyFactory(const uint64_t block_size, - const uint64_t block_size_deviation) : - block_size_(block_size), - block_size_deviation_(block_size_deviation) { - } + FlushBlockBySizePolicyFactory() {} virtual const char* Name() const override { return "FlushBlockBySizePolicyFactory"; } virtual FlushBlockPolicy* NewFlushBlockPolicy( + const Options& options, const BlockBuilder& data_block_builder) const override; - - private: - const uint64_t block_size_; - const uint64_t block_size_deviation_; }; } // rocksdb diff --git a/table/block_based_table_builder.cc b/table/block_based_table_builder.cc index 75f204670..9890ef33b 100644 --- a/table/block_based_table_builder.cc +++ b/table/block_based_table_builder.cc @@ -87,8 +87,8 @@ struct BlockBasedTableBuilder::Rep { filter_block(opt.filter_policy == nullptr ? nullptr : new FilterBlockBuilder(opt, &internal_comparator)), - flush_block_policy( - flush_block_policy_factory->NewFlushBlockPolicy(data_block)) {} + flush_block_policy(flush_block_policy_factory->NewFlushBlockPolicy( + options, data_block)) {} }; BlockBasedTableBuilder::BlockBasedTableBuilder( diff --git a/table/block_based_table_factory.cc b/table/block_based_table_factory.cc index 6a4a64462..6e06c4dbb 100644 --- a/table/block_based_table_factory.cc +++ b/table/block_based_table_factory.cc @@ -18,6 +18,15 @@ namespace rocksdb { +BlockBasedTableFactory::BlockBasedTableFactory( + const BlockBasedTableOptions& table_options) + : table_options_(table_options) { + if (table_options_.flush_block_policy_factory == nullptr) { + table_options_.flush_block_policy_factory.reset( + new FlushBlockBySizePolicyFactory()); + } +} + Status BlockBasedTableFactory::NewTableReader( const Options& options, const EnvOptions& soptions, const InternalKeyComparator& internal_comparator, @@ -31,35 +40,13 @@ Status BlockBasedTableFactory::NewTableReader( TableBuilder* BlockBasedTableFactory::NewTableBuilder( const Options& options, const InternalKeyComparator& internal_comparator, WritableFile* file, CompressionType compression_type) const { - auto flush_block_policy_factory = - table_options_.flush_block_policy_factory.get(); - - // if flush block policy factory is not set, we'll create the default one - // from the options. - // - // NOTE: we cannot pre-cache the "default block policy factory" because - // `FlushBlockBySizePolicyFactory` takes `options.block_size` and - // `options.block_size_deviation` as parameters, which may be different - // every time. - if (flush_block_policy_factory == nullptr) { - flush_block_policy_factory = - new FlushBlockBySizePolicyFactory(options.block_size, - options.block_size_deviation); - } + auto flush_block_policy_factory = + table_options_.flush_block_policy_factory.get(); auto table_builder = new BlockBasedTableBuilder(options, internal_comparator, file, flush_block_policy_factory, compression_type); - // Delete flush_block_policy_factory only when it's just created from the - // options. - // We can safely delete flush_block_policy_factory since it will only be used - // during the construction of `BlockBasedTableBuilder`. - if (flush_block_policy_factory != - table_options_.flush_block_policy_factory.get()) { - delete flush_block_policy_factory; - } - return table_builder; } diff --git a/table/block_based_table_factory.h b/table/block_based_table_factory.h index 556997065..492349c55 100644 --- a/table/block_based_table_factory.h +++ b/table/block_based_table_factory.h @@ -26,8 +26,7 @@ class BlockBasedTableBuilder; class BlockBasedTableFactory : public TableFactory { public: explicit BlockBasedTableFactory( - const BlockBasedTableOptions& table_options = BlockBasedTableOptions()) - : table_options_(table_options) {} + const BlockBasedTableOptions& table_options = BlockBasedTableOptions()); ~BlockBasedTableFactory() {} diff --git a/table/flush_block_policy.cc b/table/flush_block_policy.cc index a953a78a7..4e2235205 100644 --- a/table/flush_block_policy.cc +++ b/table/flush_block_policy.cc @@ -3,6 +3,7 @@ // LICENSE file in the root directory of this source tree. An additional grant // of patent rights can be found in the PATENTS file in the same directory. +#include "rocksdb/options.h" #include "rocksdb/flush_block_policy.h" #include "rocksdb/slice.h" #include "table/block_builder.h" @@ -61,10 +62,9 @@ class FlushBlockBySizePolicy : public FlushBlockPolicy { }; FlushBlockPolicy* FlushBlockBySizePolicyFactory::NewFlushBlockPolicy( - const BlockBuilder& data_block_builder) const { - return new FlushBlockBySizePolicy(block_size_, - block_size_deviation_, - data_block_builder); + const Options& options, const BlockBuilder& data_block_builder) const { + return new FlushBlockBySizePolicy( + options.block_size, options.block_size_deviation, data_block_builder); } } // namespace rocksdb diff --git a/table/table_test.cc b/table/table_test.cc index f2a95966a..b9b87e525 100644 --- a/table/table_test.cc +++ b/table/table_test.cc @@ -689,8 +689,7 @@ class Harness { switch (args.type) { case BLOCK_BASED_TABLE_TEST: table_options.flush_block_policy_factory.reset( - new FlushBlockBySizePolicyFactory(options_.block_size, - options_.block_size_deviation)); + new FlushBlockBySizePolicyFactory()); options_.table_factory.reset(new BlockBasedTableFactory(table_options)); constructor_ = new TableConstructor(options_.comparator); break;