From e3f953a863349d76152456d9d55347e633750fba Mon Sep 17 00:00:00 2001 From: Levi Tamasi Date: Thu, 28 May 2020 10:00:19 -0700 Subject: [PATCH] Make it possible to look up files by number in VersionStorageInfo (#6862) Summary: Does what it says on the can: the patch adds a hash map to `VersionStorageInfo` that maps file numbers to file locations, i.e. (level, position in level) pairs. This will enable stricter consistency checks in `VersionBuilder`. The patch also fixes all the unit tests that used duplicate file numbers in a version (which would trigger an assertion with the new code). Pull Request resolved: https://github.com/facebook/rocksdb/pull/6862 Test Plan: `make check` `make whitebox_crash_test` Reviewed By: riversand963 Differential Revision: D21670446 Pulled By: ltamasi fbshipit-source-id: 2eac249945cf33d8fb8597b26bfff5221e1a861a --- db/compaction/compaction_picker_test.cc | 14 ++++---- db/version_set.cc | 20 ++++++++++- db/version_set.h | 46 +++++++++++++++++++++++++ db/version_set_test.cc | 33 +++++++++++++----- 4 files changed, 97 insertions(+), 16 deletions(-) diff --git a/db/compaction/compaction_picker_test.cc b/db/compaction/compaction_picker_test.cc index 695d05e5b..3617be92b 100644 --- a/db/compaction/compaction_picker_test.cc +++ b/db/compaction/compaction_picker_test.cc @@ -414,8 +414,8 @@ TEST_F(CompactionPickerTest, LevelTriggerDynamic4) { mutable_cf_options_.max_bytes_for_level_multiplier = 10; NewVersionStorage(num_levels, kCompactionStyleLevel); Add(0, 1U, "150", "200"); - Add(num_levels - 1, 3U, "200", "250", 300U); - Add(num_levels - 1, 4U, "300", "350", 3000U); + Add(num_levels - 1, 2U, "200", "250", 300U); + Add(num_levels - 1, 3U, "300", "350", 3000U); Add(num_levels - 1, 4U, "400", "450", 3U); Add(num_levels - 2, 5U, "150", "180", 300U); Add(num_levels - 2, 6U, "181", "350", 500U); @@ -782,7 +782,7 @@ TEST_F(CompactionPickerTest, CompactionPriMinOverlapping2) { Add(2, 8U, "201", "300", 60000000U); // Overlaps with file 28, 29, total size 521M - Add(3, 26U, "100", "110", 261000000U); + Add(3, 25U, "100", "110", 261000000U); Add(3, 26U, "150", "170", 261000000U); Add(3, 27U, "171", "179", 260000000U); Add(3, 28U, "191", "220", 260000000U); @@ -1298,7 +1298,7 @@ TEST_F(CompactionPickerTest, EstimateCompactionBytesNeeded1) { // Size ratio L4/L3 is 9.9 // After merge from L3, L4 size is 1000900 Add(4, 11U, "400", "500", 999900); - Add(5, 11U, "400", "500", 8007200); + Add(5, 12U, "400", "500", 8007200); UpdateVersionStorageInfo(); @@ -1611,8 +1611,8 @@ TEST_F(CompactionPickerTest, IsTrivialMoveOn) { Add(3, 5U, "120", "130", 7000U); Add(3, 6U, "170", "180", 7000U); - Add(3, 5U, "220", "230", 7000U); - Add(3, 5U, "270", "280", 7000U); + Add(3, 7U, "220", "230", 7000U); + Add(3, 8U, "270", "280", 7000U); UpdateVersionStorageInfo(); std::unique_ptr compaction(level_compaction_picker.PickCompaction( @@ -1811,7 +1811,7 @@ TEST_F(CompactionPickerTest, UniversalMarkedCompactionFullOverlap) { AddVersionStorage(); // Simulate a flush and mark the file for compaction - Add(0, 1U, "150", "200", kFileSize, 0, 551, 600, 0, true); + Add(0, 7U, "150", "200", kFileSize, 0, 551, 600, 0, true); UpdateVersionStorageInfo(); std::unique_ptr compaction2( diff --git a/db/version_set.cc b/db/version_set.cc index 38d0e9ed0..184a7c652 100644 --- a/db/version_set.cc +++ b/db/version_set.cc @@ -2636,6 +2636,12 @@ void VersionStorageInfo::AddFile(int level, FileMetaData* f, Logger* info_log) { #endif f->refs++; level_files->push_back(f); + + const uint64_t file_number = f->fd.GetNumber(); + + assert(file_locations_.find(file_number) == file_locations_.end()); + file_locations_.emplace(file_number, + FileLocation(level, level_files->size() - 1)); } void VersionStorageInfo::AddBlobFile( @@ -4971,7 +4977,19 @@ Status VersionSet::ReduceNumberOfLevels(const std::string& dbname, } if (first_nonempty_level > 0) { - new_files_list[new_levels - 1] = vstorage->LevelFiles(first_nonempty_level); + auto& new_last_level = new_files_list[new_levels - 1]; + + new_last_level = vstorage->LevelFiles(first_nonempty_level); + + for (size_t i = 0; i < new_last_level.size(); ++i) { + const FileMetaData* const meta = new_last_level[i]; + assert(meta); + + const uint64_t file_number = meta->fd.GetNumber(); + + vstorage->file_locations_[file_number] = + VersionStorageInfo::FileLocation(new_levels - 1, i); + } } delete[] vstorage -> files_; diff --git a/db/version_set.h b/db/version_set.h index ce518fb8a..4a5683dd1 100644 --- a/db/version_set.h +++ b/db/version_set.h @@ -283,6 +283,47 @@ class VersionStorageInfo { return files_[level]; } + class FileLocation { + public: + FileLocation() = default; + FileLocation(int level, size_t position) + : level_(level), position_(position) {} + + int GetLevel() const { return level_; } + size_t GetPosition() const { return position_; } + + bool IsValid() const { return level_ >= 0; } + + bool operator==(const FileLocation& rhs) const { + return level_ == rhs.level_ && position_ == rhs.position_; + } + + bool operator!=(const FileLocation& rhs) const { return !(*this == rhs); } + + static FileLocation Invalid() { return FileLocation(); } + + private: + int level_ = -1; + size_t position_ = 0; + }; + + // REQUIRES: This version has been saved (see VersionSet::SaveTo) + FileLocation GetFileLocation(uint64_t file_number) const { + const auto it = file_locations_.find(file_number); + + if (it == file_locations_.end()) { + return FileLocation::Invalid(); + } + + assert(it->second.GetLevel() < num_levels_); + assert(it->second.GetPosition() < files_[it->second.GetLevel()].size()); + assert(files_[it->second.GetLevel()][it->second.GetPosition()]); + assert(files_[it->second.GetLevel()][it->second.GetPosition()] + ->fd.GetNumber() == file_number); + + return it->second; + } + // REQUIRES: This version has been saved (see VersionSet::SaveTo) using BlobFiles = std::map>; const BlobFiles& GetBlobFiles() const { return blob_files_; } @@ -461,6 +502,11 @@ class VersionStorageInfo { // in increasing order of keys std::vector* files_; + // Map of all table files in version. Maps file number to (level, position on + // level). + using FileLocations = std::unordered_map; + FileLocations file_locations_; + // Map of blob files in version by number. BlobFiles blob_files_; diff --git a/db/version_set_test.cc b/db/version_set_test.cc index 1607ddae1..d3f3361a9 100644 --- a/db/version_set_test.cc +++ b/db/version_set_test.cc @@ -372,19 +372,19 @@ TEST_F(VersionStorageInfoTest, EstimateLiveDataSize) { Add(2, 3U, "6", "8", 1U); // Partial overlap with last level Add(3, 4U, "1", "9", 1U); // Contains range of last level Add(4, 5U, "4", "5", 1U); // Inside range of last level - Add(4, 5U, "6", "7", 1U); // Inside range of last level - Add(5, 6U, "4", "7", 10U); + Add(4, 6U, "6", "7", 1U); // Inside range of last level + Add(5, 7U, "4", "7", 10U); ASSERT_EQ(10U, vstorage_.EstimateLiveDataSize()); } TEST_F(VersionStorageInfoTest, EstimateLiveDataSize2) { Add(0, 1U, "9", "9", 1U); // Level 0 is not ordered - Add(0, 1U, "5", "6", 1U); // Ignored because of [5,6] in l1 - Add(1, 1U, "1", "2", 1U); // Ignored because of [2,3] in l2 - Add(1, 2U, "3", "4", 1U); // Ignored because of [2,3] in l2 - Add(1, 3U, "5", "6", 1U); - Add(2, 4U, "2", "3", 1U); - Add(3, 5U, "7", "8", 1U); + Add(0, 2U, "5", "6", 1U); // Ignored because of [5,6] in l1 + Add(1, 3U, "1", "2", 1U); // Ignored because of [2,3] in l2 + Add(1, 4U, "3", "4", 1U); // Ignored because of [2,3] in l2 + Add(1, 5U, "5", "6", 1U); + Add(2, 6U, "2", "3", 1U); + Add(3, 7U, "7", "8", 1U); ASSERT_EQ(4U, vstorage_.EstimateLiveDataSize()); } @@ -421,6 +421,23 @@ TEST_F(VersionStorageInfoTest, GetOverlappingInputs) { 1, {"i", 0, kTypeValue}, {"j", 0, kTypeValue})); } +TEST_F(VersionStorageInfoTest, FileLocation) { + Add(0, 11U, "1", "2", 5000U); + Add(0, 12U, "1", "2", 5000U); + + Add(2, 7U, "1", "2", 8000U); + + ASSERT_EQ(vstorage_.GetFileLocation(11U), + VersionStorageInfo::FileLocation(0, 0)); + ASSERT_EQ(vstorage_.GetFileLocation(12U), + VersionStorageInfo::FileLocation(0, 1)); + + ASSERT_EQ(vstorage_.GetFileLocation(7U), + VersionStorageInfo::FileLocation(2, 0)); + + ASSERT_FALSE(vstorage_.GetFileLocation(999U).IsValid()); +} + class VersionStorageInfoTimestampTest : public VersionStorageInfoTestBase { public: VersionStorageInfoTimestampTest()