From d83542ca830e9f4f313ddb29ec3bbec742d7f001 Mon Sep 17 00:00:00 2001 From: Levi Tamasi Date: Mon, 17 May 2021 18:27:42 -0700 Subject: [PATCH] Make it possible to apply only a subrange of table property collectors (#8298) Summary: This patch does two things: 1) Introduces some aliases in order to eliminate/prevent long-winded type names w/r/t the internal table property collectors (see e.g. `std::vector>`). 2) Makes it possible to apply only a subrange of table property collectors during table building by turning `TableBuilderOptions::int_tbl_prop_collector_factories` from a pointer to a `vector` into a range (i.e. a pair of iterators). Rationale: I plan to introduce a BlobDB related table property collector, which should only be applied during table creation if blob storage is enabled at the moment (which can be changed dynamically). This change will make it possible to include/ exclude the BlobDB related collector as needed without having to introduce a second `vector` of collectors in `ColumnFamilyData` with pretty much the same contents. Pull Request resolved: https://github.com/facebook/rocksdb/pull/8298 Test Plan: `make check` Reviewed By: jay-zhuang Differential Revision: D28430910 Pulled By: ltamasi fbshipit-source-id: a81d28f2c59495865300f43deb2257d2e6977c8e --- db/column_family.cc | 5 ++-- db/column_family.h | 9 ++---- db/table_properties_collector.h | 7 +++++ db/table_properties_collector_test.cc | 20 ++++++------- db/version_set_test.cc | 3 +- .../block_based/block_based_table_builder.cc | 7 +++-- .../block_based_table_reader_test.cc | 2 +- .../block_based/data_block_hash_index_test.cc | 3 +- table/block_fetcher_test.cc | 2 +- table/plain/plain_table_builder.cc | 10 ++++--- table/plain/plain_table_builder.h | 3 +- table/sst_file_dumper.cc | 3 +- table/sst_file_writer.cc | 3 +- table/table_builder.h | 29 ++++++++++++++++--- table/table_reader_bench.cc | 3 +- table/table_test.cc | 27 ++++++----------- tools/sst_dump_test.cc | 4 +-- 17 files changed, 76 insertions(+), 64 deletions(-) diff --git a/db/column_family.cc b/db/column_family.cc index d8f483908..5c60cd254 100644 --- a/db/column_family.cc +++ b/db/column_family.cc @@ -109,8 +109,9 @@ const Comparator* ColumnFamilyHandleImpl::GetComparator() const { void GetIntTblPropCollectorFactory( const ImmutableCFOptions& ioptions, - std::vector>* - int_tbl_prop_collector_factories) { + IntTblPropCollectorFactories* int_tbl_prop_collector_factories) { + assert(int_tbl_prop_collector_factories); + auto& collector_factories = ioptions.table_properties_collector_factories; for (size_t i = 0; i < ioptions.table_properties_collector_factories.size(); ++i) { diff --git a/db/column_family.h b/db/column_family.h index 256270fe0..6b6ac6c7c 100644 --- a/db/column_family.h +++ b/db/column_family.h @@ -258,8 +258,7 @@ extern ColumnFamilyOptions SanitizeOptions(const ImmutableDBOptions& db_options, // one too. extern void GetIntTblPropCollectorFactory( const ImmutableCFOptions& ioptions, - std::vector>* - int_tbl_prop_collector_factories); + IntTblPropCollectorFactories* int_tbl_prop_collector_factories); class ColumnFamilySet; @@ -428,8 +427,7 @@ class ColumnFamilyData { return internal_comparator_; } - const std::vector>* - int_tbl_prop_collector_factories() const { + const IntTblPropCollectorFactories* int_tbl_prop_collector_factories() const { return &int_tbl_prop_collector_factories_; } @@ -548,8 +546,7 @@ class ColumnFamilyData { std::atomic dropped_; // true if client dropped it const InternalKeyComparator internal_comparator_; - std::vector> - int_tbl_prop_collector_factories_; + IntTblPropCollectorFactories int_tbl_prop_collector_factories_; const ColumnFamilyOptions initial_cf_options_; const ImmutableOptions ioptions_; diff --git a/db/table_properties_collector.h b/db/table_properties_collector.h index 23a2853ac..befb43652 100644 --- a/db/table_properties_collector.h +++ b/db/table_properties_collector.h @@ -48,6 +48,13 @@ class IntTblPropCollectorFactory { virtual const char* Name() const = 0; }; +using IntTblPropCollectorFactories = + std::vector>; +using IntTblPropCollectorFactoryIter = + IntTblPropCollectorFactories::const_iterator; +using IntTblPropCollectorFactoryRange = + std::pair; + // When rocksdb creates a new table, it will encode all "user keys" into // "internal keys", which contains meta information of a given entry. // diff --git a/db/table_properties_collector_test.cc b/db/table_properties_collector_test.cc index 0c67c8901..301302bae 100644 --- a/db/table_properties_collector_test.cc +++ b/db/table_properties_collector_test.cc @@ -41,13 +41,13 @@ namespace { static const uint32_t kTestColumnFamilyId = 66; static const std::string kTestColumnFamilyName = "test_column_fam"; -void MakeBuilder(const Options& options, const ImmutableOptions& ioptions, - const MutableCFOptions& moptions, - const InternalKeyComparator& internal_comparator, - const std::vector>* - int_tbl_prop_collector_factories, - std::unique_ptr* writable, - std::unique_ptr* builder) { +void MakeBuilder( + const Options& options, const ImmutableOptions& ioptions, + const MutableCFOptions& moptions, + const InternalKeyComparator& internal_comparator, + const IntTblPropCollectorFactories* int_tbl_prop_collector_factories, + std::unique_ptr* writable, + std::unique_ptr* builder) { std::unique_ptr wf(new test::StringSink); writable->reset( new WritableFileWriter(std::move(wf), "" /* don't care */, EnvOptions())); @@ -264,8 +264,7 @@ void TestCustomizedTablePropertiesCollector( std::unique_ptr writer; const ImmutableOptions ioptions(options); const MutableCFOptions moptions(options); - std::vector> - int_tbl_prop_collector_factories; + IntTblPropCollectorFactories int_tbl_prop_collector_factories; if (test_int_tbl_prop_collector) { int_tbl_prop_collector_factories.emplace_back( new RegularKeysStartWithAFactory(backward_mode)); @@ -395,8 +394,7 @@ void TestInternalKeyPropertiesCollector( Options options; test::PlainInternalKeyComparator pikc(options.comparator); - std::vector> - int_tbl_prop_collector_factories; + IntTblPropCollectorFactories int_tbl_prop_collector_factories; options.table_factory = table_factory; if (sanitized) { options.table_properties_collector_factories.emplace_back( diff --git a/db/version_set_test.cc b/db/version_set_test.cc index 74502668d..65fea27f8 100644 --- a/db/version_set_test.cc +++ b/db/version_set_test.cc @@ -2775,8 +2775,7 @@ class VersionSetTestMissingFiles : public VersionSetTestBase, ASSERT_OK(s); std::unique_ptr fwriter(new WritableFileWriter( std::move(file), fname, FileOptions(), env_->GetSystemClock().get())); - std::vector> - int_tbl_prop_collector_factories; + IntTblPropCollectorFactories int_tbl_prop_collector_factories; std::unique_ptr builder(table_factory_->NewTableBuilder( TableBuilderOptions( diff --git a/table/block_based/block_based_table_builder.cc b/table/block_based/block_based_table_builder.cc index 314b5393e..43663639f 100644 --- a/table/block_based/block_based_table_builder.cc +++ b/table/block_based/block_based_table_builder.cc @@ -505,9 +505,12 @@ struct BlockBasedTableBuilder::Rep { use_delta_encoding_for_index_values, p_index_builder_)); } - for (auto& collector_factories : *tbo.int_tbl_prop_collector_factories) { + const auto& factory_range = tbo.int_tbl_prop_collector_factories; + for (auto it = factory_range.first; it != factory_range.second; ++it) { + assert(*it); + table_properties_collectors.emplace_back( - collector_factories->CreateIntTblPropCollector(column_family_id)); + (*it)->CreateIntTblPropCollector(column_family_id)); } table_properties_collectors.emplace_back( new BlockBasedTablePropertiesCollector( diff --git a/table/block_based/block_based_table_reader_test.cc b/table/block_based/block_based_table_reader_test.cc index f6ea36ab1..07136dbf8 100644 --- a/table/block_based/block_based_table_reader_test.cc +++ b/table/block_based/block_based_table_reader_test.cc @@ -63,7 +63,7 @@ class BlockBasedTableReaderTest InternalKeyComparator comparator(options.comparator); ColumnFamilyOptions cf_options; MutableCFOptions moptions(cf_options); - std::vector> factories; + IntTblPropCollectorFactories factories; std::unique_ptr table_builder(table_factory_->NewTableBuilder( TableBuilderOptions(ioptions, moptions, comparator, &factories, compression_type, CompressionOptions(), diff --git a/table/block_based/data_block_hash_index_test.cc b/table/block_based/data_block_hash_index_test.cc index def3d4aa7..121f78cef 100644 --- a/table/block_based/data_block_hash_index_test.cc +++ b/table/block_based/data_block_hash_index_test.cc @@ -551,8 +551,7 @@ void TestBoundary(InternalKey& ik1, std::string& v1, InternalKey& ik2, file_writer.reset( new WritableFileWriter(std::move(f), "" /* don't care */, FileOptions())); std::unique_ptr builder; - std::vector> - int_tbl_prop_collector_factories; + IntTblPropCollectorFactories int_tbl_prop_collector_factories; std::string column_family_name; builder.reset(ioptions.table_factory->NewTableBuilder( TableBuilderOptions( diff --git a/table/block_fetcher_test.cc b/table/block_fetcher_test.cc index fad877eac..4499272b4 100644 --- a/table/block_fetcher_test.cc +++ b/table/block_fetcher_test.cc @@ -97,7 +97,7 @@ class BlockFetcherTest : public testing::Test { InternalKeyComparator comparator(options_.comparator); ColumnFamilyOptions cf_options(options_); MutableCFOptions moptions(cf_options); - std::vector> factories; + IntTblPropCollectorFactories factories; std::unique_ptr table_builder(table_factory_.NewTableBuilder( TableBuilderOptions(ioptions, moptions, comparator, &factories, compression_type, CompressionOptions(), diff --git a/table/plain/plain_table_builder.cc b/table/plain/plain_table_builder.cc index 6e1e121a8..3a1f0a41b 100644 --- a/table/plain/plain_table_builder.cc +++ b/table/plain/plain_table_builder.cc @@ -58,8 +58,7 @@ extern const uint64_t kLegacyPlainTableMagicNumber = 0x4f3418eb7a8f13b8ull; PlainTableBuilder::PlainTableBuilder( const ImmutableOptions& ioptions, const MutableCFOptions& moptions, - const std::vector>* - int_tbl_prop_collector_factories, + const IntTblPropCollectorFactoryRange& int_tbl_prop_collector_factories, uint32_t column_family_id, WritableFileWriter* file, uint32_t user_key_len, EncodingType encoding_type, size_t index_sparseness, uint32_t bloom_bits_per_key, const std::string& column_family_name, @@ -113,9 +112,12 @@ PlainTableBuilder::PlainTableBuilder( properties_.user_collected_properties [PlainTablePropertyNames::kEncodingType] = val; - for (auto& collector_factories : *int_tbl_prop_collector_factories) { + for (auto it = int_tbl_prop_collector_factories.first; + it != int_tbl_prop_collector_factories.second; ++it) { + assert(*it); + table_properties_collectors_.emplace_back( - collector_factories->CreateIntTblPropCollector(column_family_id)); + (*it)->CreateIntTblPropCollector(column_family_id)); } } diff --git a/table/plain/plain_table_builder.h b/table/plain/plain_table_builder.h index 88c3a6177..7305cb153 100644 --- a/table/plain/plain_table_builder.h +++ b/table/plain/plain_table_builder.h @@ -38,8 +38,7 @@ class PlainTableBuilder: public TableBuilder { // that the caller does not know which level the output file will reside. PlainTableBuilder( const ImmutableOptions& ioptions, const MutableCFOptions& moptions, - const std::vector>* - int_tbl_prop_collector_factories, + const IntTblPropCollectorFactoryRange& int_tbl_prop_collector_factories, uint32_t column_family_id, WritableFileWriter* file, uint32_t user_key_size, EncodingType encoding_type, size_t index_sparseness, uint32_t bloom_bits_per_key, diff --git a/table/sst_file_dumper.cc b/table/sst_file_dumper.cc index 8586934df..d2ade5bfe 100644 --- a/table/sst_file_dumper.cc +++ b/table/sst_file_dumper.cc @@ -268,8 +268,7 @@ Status SstFileDumper::ShowCompressionSize( const ColumnFamilyOptions cfo(opts); const MutableCFOptions moptions(cfo); ROCKSDB_NAMESPACE::InternalKeyComparator ikc(opts.comparator); - std::vector> - block_based_table_factories; + IntTblPropCollectorFactories block_based_table_factories; std::string column_family_name; int unknown_level = -1; diff --git a/table/sst_file_writer.cc b/table/sst_file_writer.cc index e8c0f06a1..dc05357e7 100644 --- a/table/sst_file_writer.cc +++ b/table/sst_file_writer.cc @@ -213,8 +213,7 @@ Status SstFileWriter::Open(const std::string& file_path) { compression_opts = r->mutable_cf_options.compression_opts; } - std::vector> - int_tbl_prop_collector_factories; + IntTblPropCollectorFactories int_tbl_prop_collector_factories; // SstFileWriter properties collector to add SstFileWriter version. int_tbl_prop_collector_factories.emplace_back( diff --git a/table/table_builder.h b/table/table_builder.h index 8735124b7..c5f22a226 100644 --- a/table/table_builder.h +++ b/table/table_builder.h @@ -93,8 +93,7 @@ struct TableBuilderOptions { TableBuilderOptions( const ImmutableOptions& _ioptions, const MutableCFOptions& _moptions, const InternalKeyComparator& _internal_comparator, - const std::vector>* - _int_tbl_prop_collector_factories, + const IntTblPropCollectorFactoryRange& _int_tbl_prop_collector_factories, CompressionType _compression_type, const CompressionOptions& _compression_opts, uint32_t _column_family_id, const std::string& _column_family_name, int _level, @@ -122,11 +121,33 @@ struct TableBuilderOptions { is_bottommost(_is_bottommost), reason(_reason) {} + TableBuilderOptions( + const ImmutableOptions& _ioptions, const MutableCFOptions& _moptions, + const InternalKeyComparator& _internal_comparator, + const IntTblPropCollectorFactories* _int_tbl_prop_collector_factories, + CompressionType _compression_type, + const CompressionOptions& _compression_opts, uint32_t _column_family_id, + const std::string& _column_family_name, int _level, + bool _is_bottommost = false, + TableFileCreationReason _reason = TableFileCreationReason::kMisc, + const uint64_t _creation_time = 0, const int64_t _oldest_key_time = 0, + const uint64_t _file_creation_time = 0, const std::string& _db_id = "", + const std::string& _db_session_id = "", + const uint64_t _target_file_size = 0) + : TableBuilderOptions(_ioptions, _moptions, _internal_comparator, + IntTblPropCollectorFactoryRange( + _int_tbl_prop_collector_factories->begin(), + _int_tbl_prop_collector_factories->end()), + _compression_type, _compression_opts, + _column_family_id, _column_family_name, _level, + _is_bottommost, _reason, _creation_time, + _oldest_key_time, _file_creation_time, _db_id, + _db_session_id, _target_file_size) {} + const ImmutableOptions& ioptions; const MutableCFOptions& moptions; const InternalKeyComparator& internal_comparator; - const std::vector>* - int_tbl_prop_collector_factories; + const IntTblPropCollectorFactoryRange int_tbl_prop_collector_factories; const CompressionType compression_type; const CompressionOptions& compression_opts; const uint32_t column_family_id; diff --git a/table/table_reader_bench.cc b/table/table_reader_bench.cc index 1d5969148..df4a750d7 100644 --- a/table/table_reader_bench.cc +++ b/table/table_reader_bench.cc @@ -95,8 +95,7 @@ void TableReaderBenchmark(Options& opts, EnvOptions& env_options, FileOptions(env_options), &file_writer, nullptr)); - std::vector > - int_tbl_prop_collector_factories; + IntTblPropCollectorFactories int_tbl_prop_collector_factories; int unknown_level = -1; tb = opts.table_factory->NewTableBuilder( diff --git a/table/table_test.cc b/table/table_test.cc index 1849e2e53..9374fbd9e 100644 --- a/table/table_test.cc +++ b/table/table_test.cc @@ -352,8 +352,7 @@ class TableConstructor : public Constructor { file_writer_.reset(new WritableFileWriter( std::move(sink), "" /* don't care */, FileOptions())); std::unique_ptr builder; - std::vector> - int_tbl_prop_collector_factories; + IntTblPropCollectorFactories int_tbl_prop_collector_factories; if (largest_seqno_ != 0) { // Pretend that it's an external file written by SstFileWriter. @@ -3314,8 +3313,7 @@ TEST_P(BlockBasedTableTest, NoFileChecksum) { std::unique_ptr comparator( new InternalKeyComparator(BytewiseComparator())); int level = 0; - std::vector> - int_tbl_prop_collector_factories; + IntTblPropCollectorFactories int_tbl_prop_collector_factories; std::string column_family_name; FileChecksumTestHelper f(true); @@ -3345,8 +3343,7 @@ TEST_P(BlockBasedTableTest, Crc32cFileChecksum) { std::unique_ptr comparator( new InternalKeyComparator(BytewiseComparator())); int level = 0; - std::vector> - int_tbl_prop_collector_factories; + IntTblPropCollectorFactories int_tbl_prop_collector_factories; std::string column_family_name; FileChecksumGenContext gen_context; @@ -3403,8 +3400,7 @@ TEST_F(PlainTableTest, BasicPlainTableProperties) { const ImmutableOptions ioptions(options); const MutableCFOptions moptions(options); InternalKeyComparator ikc(options.comparator); - std::vector> - int_tbl_prop_collector_factories; + IntTblPropCollectorFactories int_tbl_prop_collector_factories; std::string column_family_name; int unknown_level = -1; std::unique_ptr builder(factory.NewTableBuilder( @@ -3456,8 +3452,7 @@ TEST_F(PlainTableTest, NoFileChecksum) { const ImmutableOptions ioptions(options); const MutableCFOptions moptions(options); InternalKeyComparator ikc(options.comparator); - std::vector> - int_tbl_prop_collector_factories; + IntTblPropCollectorFactories int_tbl_prop_collector_factories; std::string column_family_name; int unknown_level = -1; FileChecksumTestHelper f(true); @@ -3490,8 +3485,7 @@ TEST_F(PlainTableTest, Crc32cFileChecksum) { const ImmutableOptions ioptions(options); const MutableCFOptions moptions(options); InternalKeyComparator ikc(options.comparator); - std::vector> - int_tbl_prop_collector_factories; + IntTblPropCollectorFactories int_tbl_prop_collector_factories; std::string column_family_name; int unknown_level = -1; @@ -4063,8 +4057,7 @@ TEST_P(BlockBasedTableTest, DISABLED_TableWithGlobalSeqno) { const ImmutableOptions ioptions(options); const MutableCFOptions moptions(options); InternalKeyComparator ikc(options.comparator); - std::vector> - int_tbl_prop_collector_factories; + IntTblPropCollectorFactories int_tbl_prop_collector_factories; int_tbl_prop_collector_factories.emplace_back( new SstFileWriterPropertiesCollectorFactory(2 /* version */, 0 /* global_seqno*/)); @@ -4251,8 +4244,7 @@ TEST_P(BlockBasedTableTest, BlockAlignTest) { const ImmutableOptions ioptions(options); const MutableCFOptions moptions(options); InternalKeyComparator ikc(options.comparator); - std::vector> - int_tbl_prop_collector_factories; + IntTblPropCollectorFactories int_tbl_prop_collector_factories; std::string column_family_name; std::unique_ptr builder(options.table_factory->NewTableBuilder( TableBuilderOptions(ioptions, moptions, ikc, @@ -4344,8 +4336,7 @@ TEST_P(BlockBasedTableTest, PropertiesBlockRestartPointTest) { const ImmutableOptions ioptions(options); const MutableCFOptions moptions(options); InternalKeyComparator ikc(options.comparator); - std::vector> - int_tbl_prop_collector_factories; + IntTblPropCollectorFactories int_tbl_prop_collector_factories; std::string column_family_name; std::unique_ptr builder(options.table_factory->NewTableBuilder( diff --git a/tools/sst_dump_test.cc b/tools/sst_dump_test.cc index 25f26c450..5cc215362 100644 --- a/tools/sst_dump_test.cc +++ b/tools/sst_dump_test.cc @@ -101,9 +101,7 @@ class SSTDumpToolTest : public testing::Test { ROCKSDB_NAMESPACE::InternalKeyComparator ikc(opts.comparator); std::unique_ptr tb; - - std::vector > - int_tbl_prop_collector_factories; + IntTblPropCollectorFactories int_tbl_prop_collector_factories; std::unique_ptr file_writer; ASSERT_OK(WritableFileWriter::Create(test_env->GetFileSystem(), file_name, file_options, &file_writer, nullptr));