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
wpc · Sep 11, 2015 · 45e9e4f · 45e9e4f
1 parent ddb950f
commit 45e9e4f
Show file tree

Hide file tree

Showing 17 changed files with 89 additions and 80 deletions.
diff --git 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<RandomAccessFileReader>&& file,
                         uint64_t file_size,
                         unique_ptr<TableReader>* 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<PlainTableReader> 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
@@ -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
@@ -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<RandomAccessFileReader>&& file, uint64_t file_size,
       unique_ptr<TableReader>* table_reader) const = 0;
 

diff --git 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<RandomAccessFileReader>&& file, uint64_t file_size,
     unique_ptr<TableReader>* 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
@@ -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<RandomAccessFileReader>&& file,
                         uint64_t file_size,
                         unique_ptr<TableReader>* table) const override;

diff --git 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<RandomAccessFileReader>&& file, uint64_t file_size,
+    unique_ptr<TableReader>* 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<RandomAccessFileReader>&& file, uint64_t file_size,
     unique_ptr<TableReader>* 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
@@ -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<RandomAccessFileReader>&& file,
                         uint64_t file_size,
-                        unique_ptr<TableReader>* table_reader) const override {
-    return NewTableReader(ioptions, soptions, internal_comparator,
-                          std::move(file), file_size, table_reader,
-                          /*prefetch_index_and_filter=*/true);
-  }
+                        unique_ptr<TableReader>* 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<RandomAccessFileReader>&& file,
                         uint64_t file_size,
                         unique_ptr<TableReader>* table_reader,

diff --git 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<RandomAccessFileReader>&& file, uint64_t file_size,
+    const TableReaderOptions& table_reader_options,
+    unique_ptr<RandomAccessFileReader>&& file, uint64_t file_size,
     std::unique_ptr<TableReader>* table) const {
-  std::unique_ptr<CuckooTableReader> new_reader(new CuckooTableReader(ioptions,
-      std::move(file), file_size, icomp.user_comparator(), nullptr));
+  std::unique_ptr<CuckooTableReader> 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
@@ -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<RandomAccessFileReader>&& file,
                         uint64_t file_size,
                         unique_ptr<TableReader>* table) const override;

diff --git a/table/mock_table.cc b/table/mock_table.cc
@@ -56,8 +56,7 @@ std::shared_ptr<const TableProperties> 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<RandomAccessFileReader>&& file, uint64_t file_size,
     unique_ptr<TableReader>* table_reader) const {
   uint32_t id = GetIDFromFile(file.get());

diff --git 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<RandomAccessFileReader>&& file,
                         uint64_t file_size,
                         unique_ptr<TableReader>* table_reader) const override;

diff --git 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<RandomAccessFileReader>&& file, uint64_t file_size,
     unique_ptr<TableReader>* 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
@@ -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<RandomAccessFileReader>&& file,
                         uint64_t file_size,
                         unique_ptr<TableReader>* table) const override;

diff --git a/table/table_builder.h b/table/table_builder.h
@@ -16,13 +16,27 @@
 #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 {
 
 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
@@ -130,9 +130,9 @@ void TableReaderBenchmark(Options& opts, EnvOptions& env_options,
     env->GetFileSize(file_name, &file_size);
     unique_ptr<RandomAccessFileReader> 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
@@ -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
@@ -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) {