From 752c87af78381b775c93f3d878f2d1f6b4098e14 Mon Sep 17 00:00:00 2001 From: Levi Tamasi Date: Fri, 7 Feb 2020 13:25:07 -0800 Subject: [PATCH] Clean up VersionEdit a bit (#6383) Summary: This is a bunch of small improvements to `VersionEdit`. Namely, the patch * Makes the names and order of variables, methods, and code chunks related to the various information elements more consistent, and adds missing getters for the sake of completeness. * Initializes previously uninitialized stack variables. * Marks all getters const to improve const correctness. * Adds in-class initializers and removes the default ctor that would create an object with uninitialized built-in fields and call `Clear` afterwards. * Adds a new type alias for new files and changes the existing `typedef` for deleted files into a type alias as well. * Makes the helper method `DecodeNewFile4From` private. * Switches from long-winded iterator syntax to range based loops in a couple of places. * Fixes a couple of assignments where an integer 0 was assigned to boolean members. * Fixes a getter which used to return a `const std::string` instead of the intended `const std::string&`. Pull Request resolved: https://github.com/facebook/rocksdb/pull/6383 Test Plan: make check Differential Revision: D19780537 Pulled By: ltamasi fbshipit-source-id: b0b4f09fee0ec0e7c7b7a6d76bfe5346e91824d0 --- db/db_impl/db_impl_files.cc | 4 +- db/version_builder.cc | 2 +- db/version_edit.cc | 110 +++++++++++++++--------------- db/version_edit.h | 131 +++++++++++++++++++----------------- db/version_edit_test.cc | 8 +-- db/version_set.cc | 2 +- 6 files changed, 132 insertions(+), 125 deletions(-) diff --git a/db/db_impl/db_impl_files.cc b/db/db_impl/db_impl_files.cc index d6053e8be..b52500fc6 100644 --- a/db/db_impl/db_impl_files.cc +++ b/db/db_impl/db_impl_files.cc @@ -618,9 +618,9 @@ uint64_t PrecomputeMinLogNumberToKeep( // family being flushed (`cfd_to_flush`). uint64_t cf_min_log_number_to_keep = 0; for (auto& e : edit_list) { - if (e->has_log_number()) { + if (e->HasLogNumber()) { cf_min_log_number_to_keep = - std::max(cf_min_log_number_to_keep, e->log_number()); + std::max(cf_min_log_number_to_keep, e->GetLogNumber()); } } if (cf_min_log_number_to_keep == 0) { diff --git a/db/version_builder.cc b/db/version_builder.cc index 6cd3de186..f2d3acb50 100644 --- a/db/version_builder.cc +++ b/db/version_builder.cc @@ -290,7 +290,7 @@ class VersionBuilder::Rep { } // Delete files - const VersionEdit::DeletedFileSet& del = edit->GetDeletedFiles(); + const auto& del = edit->GetDeletedFiles(); for (const auto& del_file : del) { const auto level = del_file.first; const auto number = del_file.second; diff --git a/db/version_edit.cc b/db/version_edit.cc index dc1d821d9..ff2741f8f 100644 --- a/db/version_edit.cc +++ b/db/version_edit.cc @@ -118,28 +118,28 @@ void FileMetaData::UpdateBoundaries(const Slice& key, const Slice& value, } void VersionEdit::Clear() { + max_level_ = 0; db_id_.clear(); comparator_.clear(); - max_level_ = 0; log_number_ = 0; prev_log_number_ = 0; - last_sequence_ = 0; next_file_number_ = 0; max_column_family_ = 0; min_log_number_to_keep_ = 0; + last_sequence_ = 0; has_db_id_ = false; has_comparator_ = false; has_log_number_ = false; has_prev_log_number_ = false; has_next_file_number_ = false; - has_last_sequence_ = false; has_max_column_family_ = false; has_min_log_number_to_keep_ = false; + has_last_sequence_ = false; deleted_files_.clear(); new_files_.clear(); column_family_ = 0; - is_column_family_add_ = 0; - is_column_family_drop_ = 0; + is_column_family_add_ = false; + is_column_family_drop_ = false; column_family_name_.clear(); is_in_atomic_group_ = false; remaining_entries_ = 0; @@ -163,12 +163,12 @@ bool VersionEdit::EncodeTo(std::string* dst) const { if (has_next_file_number_) { PutVarint32Varint64(dst, kNextFileNumber, next_file_number_); } - if (has_last_sequence_) { - PutVarint32Varint64(dst, kLastSequence, last_sequence_); - } if (has_max_column_family_) { PutVarint32Varint32(dst, kMaxColumnFamily, max_column_family_); } + if (has_last_sequence_) { + PutVarint32Varint64(dst, kLastSequence, last_sequence_); + } for (const auto& deleted : deleted_files_) { PutVarint32Varint32Varint64(dst, kDeletedFile, deleted.first /* level */, deleted.second /* file number */); @@ -287,7 +287,7 @@ static bool GetInternalKey(Slice* input, InternalKey* dst) { } bool VersionEdit::GetLevel(Slice* input, int* level, const char** /*msg*/) { - uint32_t v; + uint32_t v = 0; if (GetVarint32(input, &v)) { *level = v; if (max_level_ < *level) { @@ -301,13 +301,13 @@ bool VersionEdit::GetLevel(Slice* input, int* level, const char** /*msg*/) { const char* VersionEdit::DecodeNewFile4From(Slice* input) { const char* msg = nullptr; - int level; + int level = 0; FileMetaData f; - uint64_t number; + uint64_t number = 0; uint32_t path_id = 0; - uint64_t file_size; - SequenceNumber smallest_seqno; - SequenceNumber largest_seqno; + uint64_t file_size = 0; + SequenceNumber smallest_seqno = 0; + SequenceNumber largest_seqno = kMaxSequenceNumber; // Since this is the only forward-compatible part of the code, we hack new // extension into this record. When we do, we set this boolean to distinguish // the record from the normal NewFile records. @@ -318,7 +318,7 @@ const char* VersionEdit::DecodeNewFile4From(Slice* input) { GetVarint64(input, &largest_seqno)) { // See comments in VersionEdit::EncodeTo() for format of customized fields while (true) { - uint32_t custom_tag; + uint32_t custom_tag = 0; Slice field; if (!GetVarint32(input, &custom_tag)) { return "new-file4 custom field"; @@ -389,10 +389,10 @@ Status VersionEdit::DecodeFrom(const Slice& src) { Clear(); Slice input = src; const char* msg = nullptr; - uint32_t tag; + uint32_t tag = 0; // Temporary storage for parsing - int level; + int level = 0; FileMetaData f; Slice str; InternalKey key; @@ -439,14 +439,6 @@ Status VersionEdit::DecodeFrom(const Slice& src) { } break; - case kLastSequence: - if (GetVarint64(&input, &last_sequence_)) { - has_last_sequence_ = true; - } else { - msg = "last sequence number"; - } - break; - case kMaxColumnFamily: if (GetVarint32(&input, &max_column_family_)) { has_max_column_family_ = true; @@ -463,6 +455,14 @@ Status VersionEdit::DecodeFrom(const Slice& src) { } break; + case kLastSequence: + if (GetVarint64(&input, &last_sequence_)) { + has_last_sequence_ = true; + } else { + msg = "last sequence number"; + } + break; + case kCompactPointer: if (GetLevel(&input, &level, &msg) && GetInternalKey(&input, &key)) { @@ -477,7 +477,7 @@ Status VersionEdit::DecodeFrom(const Slice& src) { break; case kDeletedFile: { - uint64_t number; + uint64_t number = 0; if (GetLevel(&input, &level, &msg) && GetVarint64(&input, &number)) { deleted_files_.insert(std::make_pair(level, number)); } else { @@ -489,8 +489,8 @@ Status VersionEdit::DecodeFrom(const Slice& src) { } case kNewFile: { - uint64_t number; - uint64_t file_size; + uint64_t number = 0; + uint64_t file_size = 0; if (GetLevel(&input, &level, &msg) && GetVarint64(&input, &number) && GetVarint64(&input, &file_size) && GetInternalKey(&input, &f.smallest) && @@ -505,10 +505,10 @@ Status VersionEdit::DecodeFrom(const Slice& src) { break; } case kNewFile2: { - uint64_t number; - uint64_t file_size; - SequenceNumber smallest_seqno; - SequenceNumber largest_seqno; + uint64_t number = 0; + uint64_t file_size = 0; + SequenceNumber smallest_seqno = 0; + SequenceNumber largest_seqno = kMaxSequenceNumber; if (GetLevel(&input, &level, &msg) && GetVarint64(&input, &number) && GetVarint64(&input, &file_size) && GetInternalKey(&input, &f.smallest) && @@ -527,11 +527,11 @@ Status VersionEdit::DecodeFrom(const Slice& src) { } case kNewFile3: { - uint64_t number; - uint32_t path_id; - uint64_t file_size; - SequenceNumber smallest_seqno; - SequenceNumber largest_seqno; + uint64_t number = 0; + uint32_t path_id = 0; + uint64_t file_size = 0; + SequenceNumber smallest_seqno = 0; + SequenceNumber largest_seqno = kMaxSequenceNumber; if (GetLevel(&input, &level, &msg) && GetVarint64(&input, &number) && GetVarint32(&input, &path_id) && GetVarint64(&input, &file_size) && GetInternalKey(&input, &f.smallest) && @@ -640,6 +640,10 @@ std::string VersionEdit::DebugString(bool hex_key) const { r.append("\n NextFileNumber: "); AppendNumberTo(&r, next_file_number_); } + if (has_max_column_family_) { + r.append("\n MaxColumnFamily: "); + AppendNumberTo(&r, max_column_family_); + } if (has_min_log_number_to_keep_) { r.append("\n MinLogNumberToKeep: "); AppendNumberTo(&r, min_log_number_to_keep_); @@ -648,13 +652,11 @@ std::string VersionEdit::DebugString(bool hex_key) const { r.append("\n LastSeq: "); AppendNumberTo(&r, last_sequence_); } - for (DeletedFileSet::const_iterator iter = deleted_files_.begin(); - iter != deleted_files_.end(); - ++iter) { + for (const auto& deleted_file : deleted_files_) { r.append("\n DeleteFile: "); - AppendNumberTo(&r, iter->first); + AppendNumberTo(&r, deleted_file.first); r.append(" "); - AppendNumberTo(&r, iter->second); + AppendNumberTo(&r, deleted_file.second); } for (size_t i = 0; i < new_files_.size(); i++) { const FileMetaData& f = new_files_[i].second; @@ -686,10 +688,6 @@ std::string VersionEdit::DebugString(bool hex_key) const { if (is_column_family_drop_) { r.append("\n ColumnFamilyDrop"); } - if (has_max_column_family_) { - r.append("\n MaxColumnFamily: "); - AppendNumberTo(&r, max_column_family_); - } if (is_in_atomic_group_) { r.append("\n AtomicGroup: "); AppendNumberTo(&r, remaining_entries_); @@ -718,6 +716,12 @@ std::string VersionEdit::DebugJSON(int edit_num, bool hex_key) const { if (has_next_file_number_) { jw << "NextFileNumber" << next_file_number_; } + if (has_max_column_family_) { + jw << "MaxColumnFamily" << max_column_family_; + } + if (has_min_log_number_to_keep_) { + jw << "MinLogNumberToKeep" << min_log_number_to_keep_; + } if (has_last_sequence_) { jw << "LastSeq" << last_sequence_; } @@ -726,12 +730,10 @@ std::string VersionEdit::DebugJSON(int edit_num, bool hex_key) const { jw << "DeletedFiles"; jw.StartArray(); - for (DeletedFileSet::const_iterator iter = deleted_files_.begin(); - iter != deleted_files_.end(); - ++iter) { + for (const auto& deleted_file : deleted_files_) { jw.StartArrayedObject(); - jw << "Level" << iter->first; - jw << "FileNumber" << iter->second; + jw << "Level" << deleted_file.first; + jw << "FileNumber" << deleted_file.second; jw.EndArrayedObject(); } @@ -767,12 +769,6 @@ std::string VersionEdit::DebugJSON(int edit_num, bool hex_key) const { if (is_column_family_drop_) { jw << "ColumnFamilyDrop" << column_family_name_; } - if (has_max_column_family_) { - jw << "MaxColumnFamily" << max_column_family_; - } - if (has_min_log_number_to_keep_) { - jw << "MinLogNumberToKeep" << min_log_number_to_keep_; - } if (is_in_atomic_group_) { jw << "AtomicGroup" << remaining_entries_; } diff --git a/db/version_edit.h b/db/version_edit.h index 5815d18dc..b95ba2a28 100644 --- a/db/version_edit.h +++ b/db/version_edit.h @@ -237,56 +237,74 @@ struct LevelFilesBrief { // to the MANIFEST file. class VersionEdit { public: - VersionEdit() { Clear(); } - ~VersionEdit() { } - void Clear(); void SetDBId(const std::string& db_id) { has_db_id_ = true; db_id_ = db_id; } + bool HasDbId() const { return has_db_id_; } + const std::string& GetDbId() const { return db_id_; } void SetComparatorName(const Slice& name) { has_comparator_ = true; comparator_ = name.ToString(); } + bool HasComparatorName() const { return has_comparator_; } + const std::string& GetComparatorName() const { return comparator_; } + void SetLogNumber(uint64_t num) { has_log_number_ = true; log_number_ = num; } + bool HasLogNumber() const { return has_log_number_; } + uint64_t GetLogNumber() const { return log_number_; } + void SetPrevLogNumber(uint64_t num) { has_prev_log_number_ = true; prev_log_number_ = num; } + bool HasPrevLogNumber() const { return has_prev_log_number_; } + uint64_t GetPrevLogNumber() const { return prev_log_number_; } + void SetNextFile(uint64_t num) { has_next_file_number_ = true; next_file_number_ = num; } - void SetLastSequence(SequenceNumber seq) { - has_last_sequence_ = true; - last_sequence_ = seq; - } + bool HasNextFile() const { return has_next_file_number_; } + uint64_t GetNextFile() const { return next_file_number_; } + void SetMaxColumnFamily(uint32_t max_column_family) { has_max_column_family_ = true; max_column_family_ = max_column_family; } + bool HasMaxColumnFamily() const { return has_max_column_family_; } + uint32_t GetMaxColumnFamily() const { return max_column_family_; } + void SetMinLogNumberToKeep(uint64_t num) { has_min_log_number_to_keep_ = true; min_log_number_to_keep_ = num; } + bool HasMinLogNumberToKeep() const { return has_min_log_number_to_keep_; } + uint64_t GetMinLogNumberToKeep() const { return min_log_number_to_keep_; } - bool has_db_id() { return has_db_id_; } - - bool has_log_number() { return has_log_number_; } - - uint64_t log_number() { return log_number_; } + void SetLastSequence(SequenceNumber seq) { + has_last_sequence_ = true; + last_sequence_ = seq; + } + bool HasLastSequence() const { return has_last_sequence_; } + SequenceNumber GetLastSequence() const { return last_sequence_; } - bool has_next_file_number() const { return has_next_file_number_; } + // Delete the specified "file" from the specified "level". + void DeleteFile(int level, uint64_t file) { + deleted_files_.emplace(level, file); + } - uint64_t next_file_number() const { return next_file_number_; } + // Retrieve the files deleted as well as their associated levels. + using DeletedFiles = std::set>; + const DeletedFiles& GetDeletedFiles() const { return deleted_files_; } - // Add the specified file at the specified number. + // Add the specified file at the specified level. // REQUIRES: This version has not been saved (see VersionSet::SaveTo) // REQUIRES: "smallest" and "largest" are smallest and largest keys in file // REQUIRES: "oldest_blob_file_number" is the number of the oldest blob file @@ -310,21 +328,17 @@ class VersionEdit { new_files_.emplace_back(level, f); } - // Delete the specified "file" from the specified "level". - void DeleteFile(int level, uint64_t file) { - deleted_files_.insert({level, file}); - } + // Retrieve the files added as well as their associated levels. + using NewFiles = std::vector>; + const NewFiles& GetNewFiles() const { return new_files_; } // Number of edits - size_t NumEntries() { return new_files_.size() + deleted_files_.size(); } - - bool IsColumnFamilyManipulation() { - return is_column_family_add_ || is_column_family_drop_; - } + size_t NumEntries() const { return new_files_.size() + deleted_files_.size(); } void SetColumnFamily(uint32_t column_family_id) { column_family_ = column_family_id; } + uint32_t GetColumnFamily() const { return column_family_; } // set column family ID by calling SetColumnFamily() void AddColumnFamily(const std::string& name) { @@ -343,29 +357,24 @@ class VersionEdit { is_column_family_drop_ = true; } - // return true on success. - bool EncodeTo(std::string* dst) const; - Status DecodeFrom(const Slice& src); - - const char* DecodeNewFile4From(Slice* input); - - typedef std::set> DeletedFileSet; - - const DeletedFileSet& GetDeletedFiles() { return deleted_files_; } - const std::vector>& GetNewFiles() { - return new_files_; + bool IsColumnFamilyManipulation() const { + return is_column_family_add_ || is_column_family_drop_; } void MarkAtomicGroup(uint32_t remaining_entries) { is_in_atomic_group_ = true; remaining_entries_ = remaining_entries; } + bool IsInAtomicGroup() const { return is_in_atomic_group_; } + uint32_t GetRemainingEntries() const { return remaining_entries_; } + + // return true on success. + bool EncodeTo(std::string* dst) const; + Status DecodeFrom(const Slice& src); std::string DebugString(bool hex_key = false) const; std::string DebugJSON(int edit_num, bool hex_key = false) const; - const std::string GetDbId() { return db_id_; } - private: friend class ReactiveVersionSet; friend class VersionSet; @@ -374,40 +383,42 @@ class VersionEdit { bool GetLevel(Slice* input, int* level, const char** msg); - int max_level_; + const char* DecodeNewFile4From(Slice* input); + + int max_level_ = 0; std::string db_id_; std::string comparator_; - uint64_t log_number_; - uint64_t prev_log_number_; - uint64_t next_file_number_; - uint32_t max_column_family_; + uint64_t log_number_ = 0; + uint64_t prev_log_number_ = 0; + uint64_t next_file_number_ = 0; + uint32_t max_column_family_ = 0; // The most recent WAL log number that is deleted - uint64_t min_log_number_to_keep_; - SequenceNumber last_sequence_; - bool has_db_id_; - bool has_comparator_; - bool has_log_number_; - bool has_prev_log_number_; - bool has_next_file_number_; - bool has_last_sequence_; - bool has_max_column_family_; - bool has_min_log_number_to_keep_; - - DeletedFileSet deleted_files_; - std::vector> new_files_; + uint64_t min_log_number_to_keep_ = 0; + SequenceNumber last_sequence_ = 0; + bool has_db_id_ = false; + bool has_comparator_ = false; + bool has_log_number_ = false; + bool has_prev_log_number_ = false; + bool has_next_file_number_ = false; + bool has_max_column_family_ = false; + bool has_min_log_number_to_keep_ = false; + bool has_last_sequence_ = false; + + DeletedFiles deleted_files_; + NewFiles new_files_; // Each version edit record should have column_family_ set // If it's not set, it is default (0) - uint32_t column_family_; + uint32_t column_family_ = 0; // a version edit can be either column_family add or // column_family drop. If it's column family add, // it also includes column family name. - bool is_column_family_drop_; - bool is_column_family_add_; + bool is_column_family_drop_ = false; + bool is_column_family_add_ = false; std::string column_family_name_; - bool is_in_atomic_group_; - uint32_t remaining_entries_; + bool is_in_atomic_group_ = false; + uint32_t remaining_entries_ = 0; }; } // namespace rocksdb diff --git a/db/version_edit_test.cc b/db/version_edit_test.cc index 8a4c1380c..cf88a182d 100644 --- a/db/version_edit_test.cc +++ b/db/version_edit_test.cc @@ -254,10 +254,10 @@ TEST_F(VersionEditTest, IgnorableField) { ASSERT_OK(ve.DecodeFrom(encoded)); - ASSERT_TRUE(ve.has_log_number()); - ASSERT_TRUE(ve.has_next_file_number()); - ASSERT_EQ(66, ve.log_number()); - ASSERT_EQ(88, ve.next_file_number()); + ASSERT_TRUE(ve.HasLogNumber()); + ASSERT_TRUE(ve.HasNextFile()); + ASSERT_EQ(66, ve.GetLogNumber()); + ASSERT_EQ(88, ve.GetNextFile()); } TEST_F(VersionEditTest, DbId) { diff --git a/db/version_set.cc b/db/version_set.cc index 5ccdbe06d..6511a4d8d 100644 --- a/db/version_set.cc +++ b/db/version_set.cc @@ -5875,7 +5875,7 @@ Status ReactiveVersionSet::ApplyOneVersionEditToBuilder( // Some other error has occurred during LoadTableHandlers. } - if (version_edit->has_next_file_number()) { + if (version_edit->HasNextFile()) { next_file_number_.store(version_edit->next_file_number_ + 1); } if (version_edit->has_last_sequence_) {