From 5edfe3a3d89e29515decb3bea9195ae880462762 Mon Sep 17 00:00:00 2001 From: Akanksha Mahajan Date: Wed, 1 Jul 2020 14:56:48 -0700 Subject: [PATCH] Update Flush policy in PartitionedIndexBuilder on switching from user-key to internal-key mode (#7022) Summary: When format_version is high enough to support user-key and there are index entries for same user key that spans multiple data blocks then it changes from user-key mode to internal-key mode. But the flush policy is not reset to point to Block Builder of internal-keys. After this switch, no entries are added to user key index partition result, thus it never triggers flushing the block. Fix: After adding the entry in sub_builder_index_, if there is a switch from user-key to internal-key, then flush policy is updated to point to Block Builder of internal-keys index partition. Pull Request resolved: https://github.com/facebook/rocksdb/pull/7022 Test Plan: 1. make check -j64 2. Added one unit test case Reviewed By: ajkr Differential Revision: D22197734 Pulled By: akankshamahajan15 fbshipit-source-id: d87e9e46bccab8e896ee6979d6b79c51f73d479e --- HISTORY.md | 1 + db/db_test2.cc | 32 ++++++++++++++++++++++++++++++ table/block_based/index_builder.cc | 21 ++++++++++++++++---- 3 files changed, 50 insertions(+), 4 deletions(-) diff --git a/HISTORY.md b/HISTORY.md index 556b257828..ed7561af2c 100644 --- a/HISTORY.md +++ b/HISTORY.md @@ -22,6 +22,7 @@ ### Bug Fixes * Fail recovery and report once hitting a physical log record checksum mismatch, while reading MANIFEST. RocksDB should not continue processing the MANIFEST any further. +* Fix a bug when index_type == kTwoLevelIndexSearch in PartitionedIndexBuilder to update FlushPolicy to point to internal key partitioner when it changes from user-key mode to internal-key mode in index partition. ## 6.11 (6/12/2020) ### Bug Fixes diff --git a/db/db_test2.cc b/db/db_test2.cc index a0fc938b04..fd2933b8c3 100644 --- a/db/db_test2.cc +++ b/db/db_test2.cc @@ -144,6 +144,38 @@ INSTANTIATE_TEST_CASE_P(TestReadOnlyWithCompressedCache, TestReadOnlyWithCompressedCache, ::testing::Combine(::testing::Values(-1, 100), ::testing::Bool())); + +class PartitionedIndexTestListener : public EventListener { + public: + void OnFlushCompleted(DB* /*db*/, const FlushJobInfo& info) override { + ASSERT_GT(info.table_properties.index_partitions, 1); + ASSERT_EQ(info.table_properties.index_key_is_user_key, 0); + } +}; + +TEST_F(DBTest2, PartitionedIndexUserToInternalKey) { + BlockBasedTableOptions table_options; + Options options = CurrentOptions(); + table_options.index_type = BlockBasedTableOptions::kTwoLevelIndexSearch; + PartitionedIndexTestListener* listener = new PartitionedIndexTestListener(); + options.table_factory.reset(NewBlockBasedTableFactory(table_options)); + options.listeners.emplace_back(listener); + std::vector snapshots; + Reopen(options); + Random rnd(301); + + for (int i = 0; i < 3000; i++) { + int j = i % 30; + std::string value = RandomString(&rnd, 10500); + ASSERT_OK(Put("keykey_" + std::to_string(j), value)); + snapshots.push_back(db_->GetSnapshot()); + } + Flush(); + for (auto s : snapshots) { + db_->ReleaseSnapshot(s); + } +} + #endif // ROCKSDB_LITE class PrefixFullBloomWithReverseComparator diff --git a/table/block_based/index_builder.cc b/table/block_based/index_builder.cc index 277bec61dd..a052c65d69 100644 --- a/table/block_based/index_builder.cc +++ b/table/block_based/index_builder.cc @@ -112,6 +112,7 @@ void PartitionedIndexBuilder::MakeNewSubIndexBuilder() { ? sub_index_builder_->index_block_builder_ : sub_index_builder_->index_block_builder_without_seq_)); partition_cut_requested_ = false; + seperator_is_key_plus_seq_ = false; } void PartitionedIndexBuilder::RequestPartitionCut() { @@ -129,9 +130,15 @@ void PartitionedIndexBuilder::AddIndexEntry( } sub_index_builder_->AddIndexEntry(last_key_in_current_block, first_key_in_next_block, block_handle); - if (sub_index_builder_->seperator_is_key_plus_seq_) { - // then we need to apply it to all sub-index builders + if (!seperator_is_key_plus_seq_ && + sub_index_builder_->seperator_is_key_plus_seq_) { + // then we need to apply it to all sub-index builders and reset + // flush_policy to point to Block Builder of sub_index_builder_ that store + // internal keys. seperator_is_key_plus_seq_ = true; + flush_policy_.reset(FlushBlockBySizePolicyFactory::NewFlushBlockPolicy( + table_opt_.metadata_block_size, table_opt_.block_size_deviation, + sub_index_builder_->index_block_builder_)); } sub_index_last_key_ = std::string(*last_key_in_current_block); entries_.push_back( @@ -161,8 +168,14 @@ void PartitionedIndexBuilder::AddIndexEntry( sub_index_builder_->AddIndexEntry(last_key_in_current_block, first_key_in_next_block, block_handle); sub_index_last_key_ = std::string(*last_key_in_current_block); - if (sub_index_builder_->seperator_is_key_plus_seq_) { - // then we need to apply it to all sub-index builders + if (!seperator_is_key_plus_seq_ && + sub_index_builder_->seperator_is_key_plus_seq_) { + // then we need to apply it to all sub-index builders and reset + // flush_policy to point to Block Builder of sub_index_builder_ that store + // internal keys. + flush_policy_.reset(FlushBlockBySizePolicyFactory::NewFlushBlockPolicy( + table_opt_.metadata_block_size, table_opt_.block_size_deviation, + sub_index_builder_->index_block_builder_)); seperator_is_key_plus_seq_ = true; } }