From 45e9e4f0bb443ea01d8c67bbee280b612cf671c4 Mon Sep 17 00:00:00 2001 From: Islam AbdelRahman Date: Fri, 11 Sep 2015 11:36:33 -0700 Subject: [PATCH] Refactor NewTableReader to accept TableReaderOptions Summary: Refactoring NewTableReader to accept TableReaderOptions This will make it easier to add new options in the future, for example in this diff https://reviews.facebook.net/D46071 Test Plan: run existing tests Reviewers: igor, yhchiang, anthony, rven, sdong Reviewed By: sdong Subscribers: dhruba Differential Revision: https://reviews.facebook.net/D46179 --- db/plain_table_db_test.cc | 27 +++++++++++++++------------ db/table_cache.cc | 5 +++-- include/rocksdb/table.h | 17 ++++++++--------- table/adaptive_table_factory.cc | 10 +++++----- table/adaptive_table_factory.h | 4 +--- table/block_based_table_factory.cc | 19 ++++++++++++++----- table/block_based_table_factory.h | 14 +++----------- table/cuckoo_table_factory.cc | 10 +++++----- table/cuckoo_table_factory.h | 4 +--- table/mock_table.cc | 3 +-- table/mock_table.h | 4 +--- table/plain_table_factory.cc | 12 ++++++------ table/plain_table_factory.h | 4 +--- table/table_builder.h | 14 ++++++++++++++ table/table_reader_bench.cc | 6 +++--- table/table_test.cc | 8 ++++---- util/sst_dump_tool.cc | 8 ++++---- 17 files changed, 89 insertions(+), 80 deletions(-) diff --git a/db/plain_table_db_test.cc b/db/plain_table_db_test.cc index 83ae4acf1c..633850cadb 100644 --- a/db/plain_table_db_test.cc +++ b/db/plain_table_db_test.cc @@ -27,6 +27,7 @@ #include "rocksdb/table.h" #include "table/meta_blocks.h" #include "table/bloom_block.h" +#include "table/table_builder.h" #include "table/plain_table_factory.h" #include "table/plain_table_reader.h" #include "util/hash.h" @@ -256,28 +257,29 @@ class TestPlainTableFactory : public PlainTableFactory { store_index_in_file_(options.store_index_in_file), expect_bloom_not_match_(expect_bloom_not_match) {} - Status NewTableReader(const ImmutableCFOptions& ioptions, - const EnvOptions& env_options, - const InternalKeyComparator& internal_comparator, + Status NewTableReader(const TableReaderOptions& table_reader_options, unique_ptr&& file, uint64_t file_size, unique_ptr* table) const override { TableProperties* props = nullptr; - auto s = ReadTableProperties(file.get(), file_size, kPlainTableMagicNumber, - ioptions.env, ioptions.info_log, &props); + auto s = + ReadTableProperties(file.get(), file_size, kPlainTableMagicNumber, + table_reader_options.ioptions.env, + table_reader_options.ioptions.info_log, &props); EXPECT_TRUE(s.ok()); if (store_index_in_file_) { BlockHandle bloom_block_handle; s = FindMetaBlock(file.get(), file_size, kPlainTableMagicNumber, - ioptions.env, BloomBlockBuilder::kBloomBlock, - &bloom_block_handle); + table_reader_options.ioptions.env, + BloomBlockBuilder::kBloomBlock, &bloom_block_handle); EXPECT_TRUE(s.ok()); BlockHandle index_block_handle; - s = FindMetaBlock( - file.get(), file_size, kPlainTableMagicNumber, ioptions.env, - PlainTableIndexBuilder::kPlainTableIndexBlock, &index_block_handle); + s = FindMetaBlock(file.get(), file_size, kPlainTableMagicNumber, + table_reader_options.ioptions.env, + PlainTableIndexBuilder::kPlainTableIndexBlock, + &index_block_handle); EXPECT_TRUE(s.ok()); } @@ -289,9 +291,10 @@ class TestPlainTableFactory : public PlainTableFactory { DecodeFixed32(encoding_type_prop->second.c_str())); std::unique_ptr new_reader(new TestPlainTableReader( - env_options, internal_comparator, encoding_type, file_size, + table_reader_options.env_options, + table_reader_options.internal_comparator, encoding_type, file_size, bloom_bits_per_key_, hash_table_ratio_, index_sparseness_, props, - std::move(file), ioptions, expect_bloom_not_match_, + std::move(file), table_reader_options.ioptions, expect_bloom_not_match_, store_index_in_file_)); *table = std::move(new_reader); diff --git a/db/table_cache.cc b/db/table_cache.cc index 8f932302af..b240fc7d0f 100644 --- a/db/table_cache.cc +++ b/db/table_cache.cc @@ -15,6 +15,7 @@ #include "rocksdb/statistics.h" #include "table/iterator_wrapper.h" +#include "table/table_builder.h" #include "table/table_reader.h" #include "table/get_context.h" #include "util/coding.h" @@ -106,8 +107,8 @@ Status TableCache::GetTableReader( ioptions_.statistics, record_read_stats, file_read_hist)); s = ioptions_.table_factory->NewTableReader( - ioptions_, env_options, internal_comparator, std::move(file_reader), - fd.GetFileSize(), table_reader); + TableReaderOptions(ioptions_, env_options, internal_comparator), + std::move(file_reader), fd.GetFileSize(), table_reader); TEST_SYNC_POINT("TableCache::GetTableReader:0"); } return s; diff --git a/include/rocksdb/table.h b/include/rocksdb/table.h index f3a22dc318..d642319f16 100644 --- a/include/rocksdb/table.h +++ b/include/rocksdb/table.h @@ -31,6 +31,7 @@ namespace rocksdb { // -- Block-based Table class FlushBlockPolicyFactory; class RandomAccessFile; +struct TableReaderOptions; struct TableBuilderOptions; class TableBuilder; class TableReader; @@ -340,16 +341,14 @@ class TableFactory { // and cache the table object returned. // (1) SstFileReader (for SST Dump) opens the table and dump the table // contents using the interator of the table. - // ImmutableCFOptions is a subset of Options that can not be altered. - // EnvOptions is a subset of Options that will be used by Env. - // Multiple configured can be accessed from there, including and not - // limited to block cache and key comparators. - // file is a file handler to handle the file for the table - // file_size is the physical file size of the file - // table_reader is the output table reader + // + // table_reader_options is a TableReaderOptions which contain all the + // needed parameters and configuration to open the table. + // file is a file handler to handle the file for the table. + // file_size is the physical file size of the file. + // table_reader is the output table reader. virtual Status NewTableReader( - const ImmutableCFOptions& ioptions, const EnvOptions& env_options, - const InternalKeyComparator& internal_comparator, + const TableReaderOptions& table_reader_options, unique_ptr&& file, uint64_t file_size, unique_ptr* table_reader) const = 0; diff --git a/table/adaptive_table_factory.cc b/table/adaptive_table_factory.cc index f642fa7bd1..c589c07a97 100644 --- a/table/adaptive_table_factory.cc +++ b/table/adaptive_table_factory.cc @@ -5,6 +5,7 @@ #ifndef ROCKSDB_LITE #include "table/adaptive_table_factory.h" +#include "table/table_builder.h" #include "table/format.h" #include "port/port.h" @@ -40,8 +41,7 @@ extern const uint64_t kLegacyBlockBasedTableMagicNumber; extern const uint64_t kCuckooTableMagicNumber; Status AdaptiveTableFactory::NewTableReader( - const ImmutableCFOptions& ioptions, const EnvOptions& env_options, - const InternalKeyComparator& icomp, + const TableReaderOptions& table_reader_options, unique_ptr&& file, uint64_t file_size, unique_ptr* table) const { Footer footer; @@ -52,14 +52,14 @@ Status AdaptiveTableFactory::NewTableReader( if (footer.table_magic_number() == kPlainTableMagicNumber || footer.table_magic_number() == kLegacyPlainTableMagicNumber) { return plain_table_factory_->NewTableReader( - ioptions, env_options, icomp, std::move(file), file_size, table); + table_reader_options, std::move(file), file_size, table); } else if (footer.table_magic_number() == kBlockBasedTableMagicNumber || footer.table_magic_number() == kLegacyBlockBasedTableMagicNumber) { return block_based_table_factory_->NewTableReader( - ioptions, env_options, icomp, std::move(file), file_size, table); + table_reader_options, std::move(file), file_size, table); } else if (footer.table_magic_number() == kCuckooTableMagicNumber) { return cuckoo_table_factory_->NewTableReader( - ioptions, env_options, icomp, std::move(file), file_size, table); + table_reader_options, std::move(file), file_size, table); } else { return Status::NotSupported("Unidentified table format"); } diff --git a/table/adaptive_table_factory.h b/table/adaptive_table_factory.h index 47a17de2ca..1685c15631 100644 --- a/table/adaptive_table_factory.h +++ b/table/adaptive_table_factory.h @@ -33,9 +33,7 @@ class AdaptiveTableFactory : public TableFactory { const char* Name() const override { return "AdaptiveTableFactory"; } - Status NewTableReader(const ImmutableCFOptions& ioptions, - const EnvOptions& env_options, - const InternalKeyComparator& internal_comparator, + Status NewTableReader(const TableReaderOptions& table_reader_options, unique_ptr&& file, uint64_t file_size, unique_ptr* table) const override; diff --git a/table/block_based_table_factory.cc b/table/block_based_table_factory.cc index 7f56d72d91..ea910c6b2e 100644 --- a/table/block_based_table_factory.cc +++ b/table/block_based_table_factory.cc @@ -42,13 +42,22 @@ BlockBasedTableFactory::BlockBasedTableFactory( } Status BlockBasedTableFactory::NewTableReader( - const ImmutableCFOptions& ioptions, const EnvOptions& soptions, - const InternalKeyComparator& internal_comparator, + const TableReaderOptions& table_reader_options, + unique_ptr&& file, uint64_t file_size, + unique_ptr* table_reader) const { + return NewTableReader(table_reader_options, std::move(file), file_size, + table_reader, + /*prefetch_index_and_filter=*/true); +} + +Status BlockBasedTableFactory::NewTableReader( + const TableReaderOptions& table_reader_options, unique_ptr&& file, uint64_t file_size, unique_ptr* table_reader, const bool prefetch_enabled) const { - return BlockBasedTable::Open(ioptions, soptions, table_options_, - internal_comparator, std::move(file), file_size, - table_reader, prefetch_enabled); + return BlockBasedTable::Open( + table_reader_options.ioptions, table_reader_options.env_options, + table_options_, table_reader_options.internal_comparator, std::move(file), + file_size, table_reader, prefetch_enabled); } TableBuilder* BlockBasedTableFactory::NewTableBuilder( diff --git a/table/block_based_table_factory.h b/table/block_based_table_factory.h index fc6cdc55ae..8bdd4cd742 100644 --- a/table/block_based_table_factory.h +++ b/table/block_based_table_factory.h @@ -33,22 +33,14 @@ class BlockBasedTableFactory : public TableFactory { const char* Name() const override { return "BlockBasedTable"; } - Status NewTableReader(const ImmutableCFOptions& ioptions, - const EnvOptions& soptions, - const InternalKeyComparator& internal_comparator, + Status NewTableReader(const TableReaderOptions& table_reader_options, unique_ptr&& file, uint64_t file_size, - unique_ptr* table_reader) const override { - return NewTableReader(ioptions, soptions, internal_comparator, - std::move(file), file_size, table_reader, - /*prefetch_index_and_filter=*/true); - } + unique_ptr* table_reader) const override; // This is a variant of virtual member function NewTableReader function with // added capability to disable pre-fetching of blocks on BlockBasedTable::Open - Status NewTableReader(const ImmutableCFOptions& ioptions, - const EnvOptions& soptions, - const InternalKeyComparator& internal_comparator, + Status NewTableReader(const TableReaderOptions& table_reader_options, unique_ptr&& file, uint64_t file_size, unique_ptr* table_reader, diff --git a/table/cuckoo_table_factory.cc b/table/cuckoo_table_factory.cc index 1899f6c28d..16bf3fbe50 100644 --- a/table/cuckoo_table_factory.cc +++ b/table/cuckoo_table_factory.cc @@ -13,12 +13,12 @@ namespace rocksdb { Status CuckooTableFactory::NewTableReader( - const ImmutableCFOptions& ioptions, const EnvOptions& env_options, - const InternalKeyComparator& icomp, - std::unique_ptr&& file, uint64_t file_size, + const TableReaderOptions& table_reader_options, + unique_ptr&& file, uint64_t file_size, std::unique_ptr* table) const { - std::unique_ptr new_reader(new CuckooTableReader(ioptions, - std::move(file), file_size, icomp.user_comparator(), nullptr)); + std::unique_ptr new_reader(new CuckooTableReader( + table_reader_options.ioptions, std::move(file), file_size, + table_reader_options.internal_comparator.user_comparator(), nullptr)); Status s = new_reader->status(); if (s.ok()) { *table = std::move(new_reader); diff --git a/table/cuckoo_table_factory.h b/table/cuckoo_table_factory.h index 9f2a677650..394e834fa3 100644 --- a/table/cuckoo_table_factory.h +++ b/table/cuckoo_table_factory.h @@ -55,9 +55,7 @@ class CuckooTableFactory : public TableFactory { const char* Name() const override { return "CuckooTable"; } - Status NewTableReader(const ImmutableCFOptions& ioptions, - const EnvOptions& env_options, - const InternalKeyComparator& internal_comparator, + Status NewTableReader(const TableReaderOptions& table_reader_options, unique_ptr&& file, uint64_t file_size, unique_ptr* table) const override; diff --git a/table/mock_table.cc b/table/mock_table.cc index 00bedec59c..ff56d6311c 100644 --- a/table/mock_table.cc +++ b/table/mock_table.cc @@ -56,8 +56,7 @@ std::shared_ptr MockTableReader::GetTableProperties() MockTableFactory::MockTableFactory() : next_id_(1) {} Status MockTableFactory::NewTableReader( - const ImmutableCFOptions& ioptions, const EnvOptions& env_options, - const InternalKeyComparator& internal_key, + const TableReaderOptions& table_reader_options, unique_ptr&& file, uint64_t file_size, unique_ptr* table_reader) const { uint32_t id = GetIDFromFile(file.get()); diff --git a/table/mock_table.h b/table/mock_table.h index d57e26e8ba..322a51d1e3 100644 --- a/table/mock_table.h +++ b/table/mock_table.h @@ -145,9 +145,7 @@ class MockTableFactory : public TableFactory { public: MockTableFactory(); const char* Name() const override { return "MockTable"; } - Status NewTableReader(const ImmutableCFOptions& ioptions, - const EnvOptions& env_options, - const InternalKeyComparator& internal_key, + Status NewTableReader(const TableReaderOptions& table_reader_options, unique_ptr&& file, uint64_t file_size, unique_ptr* table_reader) const override; diff --git a/table/plain_table_factory.cc b/table/plain_table_factory.cc index b7ce3b7528..6e86ff54fc 100644 --- a/table/plain_table_factory.cc +++ b/table/plain_table_factory.cc @@ -15,14 +15,14 @@ namespace rocksdb { Status PlainTableFactory::NewTableReader( - const ImmutableCFOptions& ioptions, const EnvOptions& env_options, - const InternalKeyComparator& icomp, + const TableReaderOptions& table_reader_options, unique_ptr&& file, uint64_t file_size, unique_ptr* table) const { - return PlainTableReader::Open(ioptions, env_options, icomp, std::move(file), - file_size, table, bloom_bits_per_key_, - hash_table_ratio_, index_sparseness_, - huge_page_tlb_size_, full_scan_mode_); + return PlainTableReader::Open( + table_reader_options.ioptions, table_reader_options.env_options, + table_reader_options.internal_comparator, std::move(file), file_size, + table, bloom_bits_per_key_, hash_table_ratio_, index_sparseness_, + huge_page_tlb_size_, full_scan_mode_); } TableBuilder* PlainTableFactory::NewTableBuilder( diff --git a/table/plain_table_factory.h b/table/plain_table_factory.h index e7e72bea44..a7d48beefe 100644 --- a/table/plain_table_factory.h +++ b/table/plain_table_factory.h @@ -153,9 +153,7 @@ class PlainTableFactory : public TableFactory { full_scan_mode_(options.full_scan_mode), store_index_in_file_(options.store_index_in_file) {} const char* Name() const override { return "PlainTable"; } - Status NewTableReader(const ImmutableCFOptions& options, - const EnvOptions& soptions, - const InternalKeyComparator& internal_comparator, + Status NewTableReader(const TableReaderOptions& table_reader_options, unique_ptr&& file, uint64_t file_size, unique_ptr* table) const override; diff --git a/table/table_builder.h b/table/table_builder.h index e6a27740a5..55a1077fa7 100644 --- a/table/table_builder.h +++ b/table/table_builder.h @@ -16,6 +16,7 @@ #include "db/table_properties_collector.h" #include "rocksdb/options.h" #include "rocksdb/table_properties.h" +#include "util/file_reader_writer.h" #include "util/mutable_cf_options.h" namespace rocksdb { @@ -23,6 +24,19 @@ namespace rocksdb { class Slice; class Status; +struct TableReaderOptions { + TableReaderOptions(const ImmutableCFOptions& _ioptions, + const EnvOptions& _env_options, + const InternalKeyComparator& _internal_comparator) + : ioptions(_ioptions), + env_options(_env_options), + internal_comparator(_internal_comparator) {} + + const ImmutableCFOptions& ioptions; + const EnvOptions& env_options; + const InternalKeyComparator& internal_comparator; +}; + struct TableBuilderOptions { TableBuilderOptions( const ImmutableCFOptions& _ioptions, diff --git a/table/table_reader_bench.cc b/table/table_reader_bench.cc index e8636f57b4..7142018085 100644 --- a/table/table_reader_bench.cc +++ b/table/table_reader_bench.cc @@ -130,9 +130,9 @@ void TableReaderBenchmark(Options& opts, EnvOptions& env_options, env->GetFileSize(file_name, &file_size); unique_ptr file_reader( new RandomAccessFileReader(std::move(raf))); - s = opts.table_factory->NewTableReader(ioptions, env_options, ikc, - std::move(file_reader), file_size, - &table_reader); + s = opts.table_factory->NewTableReader( + TableReaderOptions(ioptions, env_options, ikc), std::move(file_reader), + file_size, &table_reader); } Random rnd(301); diff --git a/table/table_test.cc b/table/table_test.cc index 4e6d1fa57b..09a1a539b2 100644 --- a/table/table_test.cc +++ b/table/table_test.cc @@ -295,8 +295,8 @@ class TableConstructor: public Constructor { file_reader_.reset(test::GetRandomAccessFileReader(new test::StringSource( GetSink()->contents(), uniq_id_, ioptions.allow_mmap_reads))); return ioptions.table_factory->NewTableReader( - ioptions, soptions, internal_comparator, std::move(file_reader_), - GetSink()->contents().size(), &table_reader_); + TableReaderOptions(ioptions, soptions, internal_comparator), + std::move(file_reader_), GetSink()->contents().size(), &table_reader_); } virtual Iterator* NewIterator() const override { @@ -317,8 +317,8 @@ class TableConstructor: public Constructor { file_reader_.reset(test::GetRandomAccessFileReader(new test::StringSource( GetSink()->contents(), uniq_id_, ioptions.allow_mmap_reads))); return ioptions.table_factory->NewTableReader( - ioptions, soptions, *last_internal_key_, std::move(file_reader_), - GetSink()->contents().size(), &table_reader_); + TableReaderOptions(ioptions, soptions, *last_internal_key_), + std::move(file_reader_), GetSink()->contents().size(), &table_reader_); } virtual TableReader* GetTableReader() { diff --git a/util/sst_dump_tool.cc b/util/sst_dump_tool.cc index bd074ea231..540ca6f407 100644 --- a/util/sst_dump_tool.cc +++ b/util/sst_dump_tool.cc @@ -91,16 +91,16 @@ Status SstFileReader::NewTableReader( if (block_table_factory) { return block_table_factory->NewTableReader( - ioptions_, soptions_, internal_comparator_, std::move(file_), file_size, - &table_reader_, /*enable_prefetch=*/false); + TableReaderOptions(ioptions_, soptions_, internal_comparator_), + std::move(file_), file_size, &table_reader_, /*enable_prefetch=*/false); } assert(!block_table_factory); // For all other factory implementation return options_.table_factory->NewTableReader( - ioptions_, soptions_, internal_comparator_, std::move(file_), file_size, - &table_reader_); + TableReaderOptions(ioptions_, soptions_, internal_comparator_), + std::move(file_), file_size, &table_reader_); } Status SstFileReader::DumpTable(const std::string& out_filename) {