From 783fcffd74a4e4c5f5398bb2ea463a6def6e747c Mon Sep 17 00:00:00 2001 From: Chris Mumford Date: Wed, 27 Mar 2019 12:52:11 -0700 Subject: [PATCH] Fixed style of snapshot compaction bug fix. Simple formatting and variable name fixes to a prior fix for snapshot compactions to conform to the Google C++ style guide. --- db/version_set.cc | 76 ++++++++++--------- db/version_set_test.cc | 162 +++++++++++++++++++++------------------- issues/issue320_test.cc | 58 +++++++------- 3 files changed, 151 insertions(+), 145 deletions(-) diff --git a/db/version_set.cc b/db/version_set.cc index 7891dfca73..47a5d01dd8 100644 --- a/db/version_set.cc +++ b/db/version_set.cc @@ -4,8 +4,8 @@ #include "db/version_set.h" -#include #include +#include #include "db/filename.h" #include "db/log_reader.h" #include "db/log_writer.h" @@ -1347,38 +1347,44 @@ Compaction* VersionSet::PickCompaction() { return c; } -// find the largest key in a vector of files. returns true if files it not empty -bool FindLargestKey(const InternalKeyComparator & icmp, const std::vector & files, InternalKey *largestKey) { +// find the largest key in a vector of files. returns true if files it not +// empty. +bool FindLargestKey(const InternalKeyComparator& icmp, + const std::vector& files, + InternalKey* largest_key) { if (files.empty()) { return false; } - *largestKey = files[0]->largest; + *largest_key = files[0]->largest; for (size_t i = 1; i < files.size(); ++i) { FileMetaData* f = files[i]; - if (icmp.Compare(f->largest, *largestKey) > 0) { - *largestKey = f->largest; + if (icmp.Compare(f->largest, *largest_key) > 0) { + *largest_key = f->largest; } } return true; } -// find minimum file b2=(l2, u2) in level file for which l2 > u1 and user_key(l2) = user_key(u1) -FileMetaData* FindSmallestBoundaryFile(const InternalKeyComparator & icmp, - const std::vector & levelFiles, - const InternalKey & largestKey) { +// find minimum file b2=(l2, u2) in level file for which l2 > u1 and +// user_key(l2) = user_key(u1) +FileMetaData* FindSmallestBoundaryFile( + const InternalKeyComparator& icmp, + const std::vector& level_files, + const InternalKey& largest_key) { const Comparator* user_cmp = icmp.user_comparator(); - FileMetaData* smallestBoundaryFile = NULL; - for (size_t i = 0; i < levelFiles.size(); ++i) { - FileMetaData* f = levelFiles[i]; - if (icmp.Compare(f->smallest, largestKey) > 0 && - user_cmp->Compare(f->smallest.user_key(), largestKey.user_key()) == 0) { - if (smallestBoundaryFile == NULL || - icmp.Compare(f->smallest, smallestBoundaryFile->smallest) < 0) { - smallestBoundaryFile = f; + FileMetaData* smallest_boundary_file = NULL; + for (size_t i = 0; i < level_files.size(); ++i) { + FileMetaData* f = level_files[i]; + if (icmp.Compare(f->smallest, largest_key) > 0 && + user_cmp->Compare(f->smallest.user_key(), largest_key.user_key()) == + 0) { + if (smallest_boundary_file == NULL || + icmp.Compare(f->smallest, smallest_boundary_file->smallest) < 0) { + smallest_boundary_file = f; } } } - return smallestBoundaryFile; + return smallest_boundary_file; } // If there are two blocks, b1=(l1, u1) and b2=(l2, u2) and @@ -1388,35 +1394,35 @@ FileMetaData* FindSmallestBoundaryFile(const InternalKeyComparator & icmp, // i rather than from b1 because it searches level by level // for records matching the supplied user key. // -// This function extracts the largest file b1 from compactionFiles -// and then searches for a b2 in levelFiles for which user_key(u1) = +// This function extracts the largest file b1 from compaction_files +// and then searches for a b2 in level_files for which user_key(u1) = // user_key(l2). If it finds such a file b2 (known as a boundary file) -// it adds it to compactionFiles and then searches again using this +// it adds it to compaction_files and then searches again using this // new upper bound. // // parameters: -// in levelFiles: list of files to search for boundary files -// in/out compactionFiles: list of files to extend by adding boundary files +// in level_files: list of files to search for boundary files. +// in/out compaction_files: list of files to extend by adding boundary files. void AddBoundaryInputs(const InternalKeyComparator& icmp, - const std::vector& levelFiles, - std::vector* compactionFiles) { - InternalKey largestKey; + const std::vector& level_files, + std::vector* compaction_files) { + InternalKey largest_key; - // find largestKey in compactionFiles, quick return if compactionFiles is + // find largest_key in compaction_files, quick return if compaction_files is // empty - if (!FindLargestKey(icmp, *compactionFiles, &largestKey)) { + if (!FindLargestKey(icmp, *compaction_files, &largest_key)) { return; } bool continueSearching = true; while (continueSearching) { - FileMetaData* smallestBoundaryFile = - FindSmallestBoundaryFile(icmp, levelFiles, largestKey); + FileMetaData* smallest_boundary_file = + FindSmallestBoundaryFile(icmp, level_files, largest_key); - // if a boundary file was found advance largestKey, otherwise we're done - if (smallestBoundaryFile != NULL) { - compactionFiles->push_back(smallestBoundaryFile); - largestKey = smallestBoundaryFile->largest; + // if a boundary file was found advance largest_key, otherwise we're done. + if (smallest_boundary_file != NULL) { + compaction_files->push_back(smallest_boundary_file); + largest_key = smallest_boundary_file->largest; } else { continueSearching = false; } diff --git a/db/version_set_test.cc b/db/version_set_test.cc index 090f115ed8..4b923fc91b 100644 --- a/db/version_set_test.cc +++ b/db/version_set_test.cc @@ -172,153 +172,161 @@ TEST(FindFileTest, OverlappingFiles) { ASSERT_TRUE(Overlaps("600", "700")); } -void AddBoundaryInputs(const InternalKeyComparator &icmp, - const std::vector &levelFiles, - std::vector *compactionFiles); +void AddBoundaryInputs(const InternalKeyComparator& icmp, + const std::vector& level_files, + std::vector* compaction_files); class AddBoundaryInputsTest { public: - std::vector levelFiles_; - std::vector compactionFiles_; - std::vector allFiles_; + std::vector level_files_; + std::vector compaction_files_; + std::vector all_files_; InternalKeyComparator icmp_; AddBoundaryInputsTest() : icmp_(BytewiseComparator()){}; ~AddBoundaryInputsTest() { - for (size_t i = 0; i < allFiles_.size(); ++i) { - delete allFiles_[i]; + for (size_t i = 0; i < all_files_.size(); ++i) { + delete all_files_[i]; } - allFiles_.clear(); + all_files_.clear(); }; - FileMetaData *CreateFileMetaData(uint64_t number, InternalKey smallest, + FileMetaData* CreateFileMetaData(uint64_t number, InternalKey smallest, InternalKey largest) { - FileMetaData *f = new FileMetaData(); + FileMetaData* f = new FileMetaData(); f->number = number; f->smallest = smallest; f->largest = largest; - allFiles_.push_back(f); + all_files_.push_back(f); return f; } }; TEST(AddBoundaryInputsTest, TestEmptyFileSets) { - AddBoundaryInputs(icmp_, levelFiles_, &compactionFiles_); - ASSERT_TRUE(compactionFiles_.empty()); - ASSERT_TRUE(levelFiles_.empty()); + AddBoundaryInputs(icmp_, level_files_, &compaction_files_); + ASSERT_TRUE(compaction_files_.empty()); + ASSERT_TRUE(level_files_.empty()); } TEST(AddBoundaryInputsTest, TestEmptyLevelFiles) { - FileMetaData *f1 = + FileMetaData* f1 = CreateFileMetaData(1, InternalKey("100", 2, kTypeValue), InternalKey(InternalKey("100", 1, kTypeValue))); - compactionFiles_.push_back(f1); + compaction_files_.push_back(f1); - AddBoundaryInputs(icmp_, levelFiles_, &compactionFiles_); - ASSERT_EQ(1, compactionFiles_.size()); - ASSERT_EQ(f1, compactionFiles_[0]); - ASSERT_TRUE(levelFiles_.empty()); + AddBoundaryInputs(icmp_, level_files_, &compaction_files_); + ASSERT_EQ(1, compaction_files_.size()); + ASSERT_EQ(f1, compaction_files_[0]); + ASSERT_TRUE(level_files_.empty()); } TEST(AddBoundaryInputsTest, TestEmptyCompactionFiles) { - FileMetaData *f1 = + FileMetaData* f1 = CreateFileMetaData(1, InternalKey("100", 2, kTypeValue), InternalKey(InternalKey("100", 1, kTypeValue))); - levelFiles_.push_back(f1); + level_files_.push_back(f1); - AddBoundaryInputs(icmp_, levelFiles_, &compactionFiles_); - ASSERT_TRUE(compactionFiles_.empty()); - ASSERT_EQ(1, levelFiles_.size()); - ASSERT_EQ(f1, levelFiles_[0]); + AddBoundaryInputs(icmp_, level_files_, &compaction_files_); + ASSERT_TRUE(compaction_files_.empty()); + ASSERT_EQ(1, level_files_.size()); + ASSERT_EQ(f1, level_files_[0]); } TEST(AddBoundaryInputsTest, TestNoBoundaryFiles) { - FileMetaData *f1 = + FileMetaData* f1 = CreateFileMetaData(1, InternalKey("100", 2, kTypeValue), InternalKey(InternalKey("100", 1, kTypeValue))); - FileMetaData *f2 = + FileMetaData* f2 = CreateFileMetaData(1, InternalKey("200", 2, kTypeValue), InternalKey(InternalKey("200", 1, kTypeValue))); - FileMetaData *f3 = + FileMetaData* f3 = CreateFileMetaData(1, InternalKey("300", 2, kTypeValue), InternalKey(InternalKey("300", 1, kTypeValue))); - levelFiles_.push_back(f3); - levelFiles_.push_back(f2); - levelFiles_.push_back(f1); - compactionFiles_.push_back(f2); - compactionFiles_.push_back(f3); + level_files_.push_back(f3); + level_files_.push_back(f2); + level_files_.push_back(f1); + compaction_files_.push_back(f2); + compaction_files_.push_back(f3); - AddBoundaryInputs(icmp_, levelFiles_, &compactionFiles_); - ASSERT_EQ(2, compactionFiles_.size()); + AddBoundaryInputs(icmp_, level_files_, &compaction_files_); + ASSERT_EQ(2, compaction_files_.size()); } TEST(AddBoundaryInputsTest, TestOneBoundaryFiles) { - FileMetaData *f1 = + FileMetaData* f1 = CreateFileMetaData(1, InternalKey("100", 3, kTypeValue), InternalKey(InternalKey("100", 2, kTypeValue))); - FileMetaData *f2 = + FileMetaData* f2 = CreateFileMetaData(1, InternalKey("100", 1, kTypeValue), InternalKey(InternalKey("200", 3, kTypeValue))); - FileMetaData *f3 = + FileMetaData* f3 = CreateFileMetaData(1, InternalKey("300", 2, kTypeValue), InternalKey(InternalKey("300", 1, kTypeValue))); - levelFiles_.push_back(f3); - levelFiles_.push_back(f2); - levelFiles_.push_back(f1); - compactionFiles_.push_back(f1); + level_files_.push_back(f3); + level_files_.push_back(f2); + level_files_.push_back(f1); + compaction_files_.push_back(f1); - AddBoundaryInputs(icmp_, levelFiles_, &compactionFiles_); - ASSERT_EQ(2, compactionFiles_.size()); - ASSERT_EQ(f1, compactionFiles_[0]); - ASSERT_EQ(f2, compactionFiles_[1]); + AddBoundaryInputs(icmp_, level_files_, &compaction_files_); + ASSERT_EQ(2, compaction_files_.size()); + ASSERT_EQ(f1, compaction_files_[0]); + ASSERT_EQ(f2, compaction_files_[1]); } TEST(AddBoundaryInputsTest, TestTwoBoundaryFiles) { - FileMetaData *f1 = + FileMetaData* f1 = CreateFileMetaData(1, InternalKey("100", 6, kTypeValue), InternalKey(InternalKey("100", 5, kTypeValue))); - FileMetaData *f2 = + FileMetaData* f2 = CreateFileMetaData(1, InternalKey("100", 2, kTypeValue), InternalKey(InternalKey("300", 1, kTypeValue))); - FileMetaData *f3 = + FileMetaData* f3 = CreateFileMetaData(1, InternalKey("100", 4, kTypeValue), InternalKey(InternalKey("100", 3, kTypeValue))); - levelFiles_.push_back(f2); - levelFiles_.push_back(f3); - levelFiles_.push_back(f1); - compactionFiles_.push_back(f1); + level_files_.push_back(f2); + level_files_.push_back(f3); + level_files_.push_back(f1); + compaction_files_.push_back(f1); - AddBoundaryInputs(icmp_, levelFiles_, &compactionFiles_); - ASSERT_EQ(3, compactionFiles_.size()); - ASSERT_EQ(f1, compactionFiles_[0]); - ASSERT_EQ(f3, compactionFiles_[1]); - ASSERT_EQ(f2, compactionFiles_[2]); + AddBoundaryInputs(icmp_, level_files_, &compaction_files_); + ASSERT_EQ(3, compaction_files_.size()); + ASSERT_EQ(f1, compaction_files_[0]); + ASSERT_EQ(f3, compaction_files_[1]); + ASSERT_EQ(f2, compaction_files_[2]); } TEST(AddBoundaryInputsTest, TestDisjoinFilePointers) { - FileMetaData *f1 = CreateFileMetaData(1, InternalKey("100", 6, kTypeValue), InternalKey(InternalKey("100", 5, kTypeValue))); - FileMetaData *f2 = CreateFileMetaData(1, InternalKey("100", 6, kTypeValue), InternalKey(InternalKey("100", 5, kTypeValue))); - FileMetaData *f3 = CreateFileMetaData(1, InternalKey("100", 2, kTypeValue), InternalKey(InternalKey("300", 1, kTypeValue))); - FileMetaData *f4 = CreateFileMetaData(1, InternalKey("100", 4, kTypeValue), InternalKey(InternalKey("100", 3, kTypeValue))); - - levelFiles_.push_back(f2); - levelFiles_.push_back(f3); - levelFiles_.push_back(f4); - - compactionFiles_.push_back(f1); - - AddBoundaryInputs(icmp_, levelFiles_, &compactionFiles_); - ASSERT_EQ(3, compactionFiles_.size()); - ASSERT_EQ(f1, compactionFiles_[0]); - ASSERT_EQ(f4, compactionFiles_[1]); - ASSERT_EQ(f3, compactionFiles_[2]); + FileMetaData* f1 = + CreateFileMetaData(1, InternalKey("100", 6, kTypeValue), + InternalKey(InternalKey("100", 5, kTypeValue))); + FileMetaData* f2 = + CreateFileMetaData(1, InternalKey("100", 6, kTypeValue), + InternalKey(InternalKey("100", 5, kTypeValue))); + FileMetaData* f3 = + CreateFileMetaData(1, InternalKey("100", 2, kTypeValue), + InternalKey(InternalKey("300", 1, kTypeValue))); + FileMetaData* f4 = + CreateFileMetaData(1, InternalKey("100", 4, kTypeValue), + InternalKey(InternalKey("100", 3, kTypeValue))); + + level_files_.push_back(f2); + level_files_.push_back(f3); + level_files_.push_back(f4); + + compaction_files_.push_back(f1); + + AddBoundaryInputs(icmp_, level_files_, &compaction_files_); + ASSERT_EQ(3, compaction_files_.size()); + ASSERT_EQ(f1, compaction_files_[0]); + ASSERT_EQ(f4, compaction_files_[1]); + ASSERT_EQ(f3, compaction_files_[2]); } -} // namespace leveldb +} // namespace leveldb int main(int argc, char** argv) { return leveldb::test::RunAllTests(); diff --git a/issues/issue320_test.cc b/issues/issue320_test.cc index 002a2faebd..06ed53d41d 100644 --- a/issues/issue320_test.cc +++ b/issues/issue320_test.cc @@ -2,29 +2,21 @@ // Use of this source code is governed by a BSD-style license that can be // found in the LICENSE file. See the AUTHORS file for names of contributors. #include -#include #include -#include +#include #include -#include - -#include #include #include #include -using namespace std; - namespace leveldb { namespace { -unsigned int random(unsigned int max) { - return rand() % max; -} +unsigned int random(unsigned int max) { return std::rand() % max; } -string newString(int32_t index) { +std::string newString(int32_t index) { const unsigned int len = 1024; char bytes[len]; unsigned int i = 0; @@ -36,12 +28,12 @@ string newString(int32_t index) { bytes[i] = 'a' + random(26); ++i; } - return string(bytes, sizeof(bytes)); + return std::string(bytes, sizeof(bytes)); } } // namespace -class Issue320 { }; +class Issue320 {}; TEST(Issue320, Test) { srandom(0); @@ -49,8 +41,8 @@ TEST(Issue320, Test) { bool delete_before_put = false; bool keep_snapshots = true; - vector*> test_map(10000, nullptr); - vector snapshots(100, nullptr); + std::vector*> test_map(10000, nullptr); + std::vector snapshots(100, nullptr); DB* db; Options options; @@ -62,14 +54,14 @@ TEST(Issue320, Test) { unsigned int target_size = 10000; unsigned int num_items = 0; unsigned long count = 0; - string key; - string value, old_value; + std::string key; + std::string value, old_value; - WriteOptions writeOptions; - ReadOptions readOptions; + WriteOptions write_options; + ReadOptions read_options; while (count < 200000) { if ((++count % 1000) == 0) { - cout << "count: " << count << endl; + std::cout << "count: " << count << std::endl; } unsigned int index = random(test_map.size()); @@ -77,18 +69,20 @@ TEST(Issue320, Test) { if (test_map[index] == nullptr) { num_items++; - test_map[index] = - new pair(newString(index), newString(index)); + test_map[index] = new std::pair( + newString(index), newString(index)); batch.Put(test_map[index]->first, test_map[index]->second); } else { - ASSERT_OK(db->Get(readOptions, test_map[index]->first, &old_value)); + ASSERT_OK(db->Get(read_options, test_map[index]->first, &old_value)); if (old_value != test_map[index]->second) { - cout << "ERROR incorrect value returned by Get" << endl; - cout << " count=" << count << endl; - cout << " old value=" << old_value << endl; - cout << " test_map[index]->second=" << test_map[index]->second << endl; - cout << " test_map[index]->first=" << test_map[index]->first << endl; - cout << " index=" << index << endl; + std::cout << "ERROR incorrect value returned by Get" << std::endl; + std::cout << " count=" << count << std::endl; + std::cout << " old value=" << old_value << std::endl; + std::cout << " test_map[index]->second=" << test_map[index]->second + << std::endl; + std::cout << " test_map[index]->first=" << test_map[index]->first + << std::endl; + std::cout << " index=" << index << std::endl; ASSERT_NE(old_value, test_map[index]->second); } @@ -104,7 +98,7 @@ TEST(Issue320, Test) { } } - ASSERT_OK(db->Write(writeOptions, &batch)); + ASSERT_OK(db->Write(write_options, &batch)); if (keep_snapshots && random(10) == 0) { unsigned int i = random(snapshots.size()); @@ -134,6 +128,4 @@ TEST(Issue320, Test) { } // namespace leveldb -int main(int argc, char** argv) { - return leveldb::test::RunAllTests(); -} +int main(int argc, char** argv) { return leveldb::test::RunAllTests(); }