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

@@ -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) {

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;

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_add_ = false;
-  is_column_family_drop_ = 0;
+  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;
+  uint64_t file_size = 0;
-  SequenceNumber smallest_seqno;
+  SequenceNumber smallest_seqno = 0;
-  SequenceNumber largest_seqno;
+  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 number = 0;
-        uint64_t file_size;
+        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 number = 0;
-        uint64_t file_size;
+        uint64_t file_size = 0;
-        SequenceNumber smallest_seqno;
+        SequenceNumber smallest_seqno = 0;
-        SequenceNumber largest_seqno;
+        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;
+        uint64_t number = 0;
-        uint32_t path_id;
+        uint32_t path_id = 0;
-        uint64_t file_size;
+        uint64_t file_size = 0;
-        SequenceNumber smallest_seqno;
+        SequenceNumber smallest_seqno = 0;
-        SequenceNumber largest_seqno;
+        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();
+  for (const auto& deleted_file : deleted_files_) {
-       iter != deleted_files_.end();
-       ++iter) {
     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();
+    for (const auto& deleted_file : deleted_files_) {
-         iter != deleted_files_.end();
-         ++iter) {
       jw.StartArrayedObject();
-      jw << "Level" << iter->first;
+      jw << "Level" << deleted_file.first;
-      jw << "FileNumber" << iter->second;
+      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_;
   }

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) {
+  bool HasNextFile() const { return has_next_file_number_; }
-    has_last_sequence_ = true;
+  uint64_t GetNextFile() const { return next_file_number_; }
-    last_sequence_ = seq;
+
-  }
   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_; }
+  void SetLastSequence(SequenceNumber seq) {
-
+    has_last_sequence_ = true;
-  bool has_log_number() { return has_log_number_; }
+    last_sequence_ = seq;
-
+  }
-  uint64_t log_number() { return log_number_; }
+  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<std::pair<int, uint64_t>>;
+  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".
+  // Retrieve the files added as well as their associated levels.
-  void DeleteFile(int level, uint64_t file) {
+  using NewFiles = std::vector<std::pair<int, FileMetaData>>;
-    deleted_files_.insert({level, file});
+  const NewFiles& GetNewFiles() const { return new_files_; }
-  }
 
   // Number of edits
-  size_t NumEntries() { return new_files_.size() + deleted_files_.size(); }
+  size_t NumEntries() const { return new_files_.size() + deleted_files_.size(); }
-
-  bool IsColumnFamilyManipulation() {
-    return is_column_family_add_ || is_column_family_drop_;
-  }
 
   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 IsColumnFamilyManipulation() const {
-  bool EncodeTo(std::string* dst) const;
+    return is_column_family_add_ || is_column_family_drop_;
-  Status DecodeFrom(const Slice& src);
-
-  const char* DecodeNewFile4From(Slice* input);
-
-  typedef std::set<std::pair<int, uint64_t>> DeletedFileSet;
-
-  const DeletedFileSet& GetDeletedFiles() { return deleted_files_; }
-  const std::vector<std::pair<int, FileMetaData>>& GetNewFiles() {
-    return new_files_;
   }
 
   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 log_number_ = 0;
-  uint64_t prev_log_number_;
+  uint64_t prev_log_number_ = 0;
-  uint64_t next_file_number_;
+  uint64_t next_file_number_ = 0;
-  uint32_t max_column_family_;
+  uint32_t max_column_family_ = 0;
   // The most recent WAL log number that is deleted
-  uint64_t min_log_number_to_keep_;
+  uint64_t min_log_number_to_keep_ = 0;
-  SequenceNumber last_sequence_;
+  SequenceNumber last_sequence_ = 0;
-  bool has_db_id_;
+  bool has_db_id_ = false;
-  bool has_comparator_;
+  bool has_comparator_ = false;
-  bool has_log_number_;
+  bool has_log_number_ = false;
-  bool has_prev_log_number_;
+  bool has_prev_log_number_ = false;
-  bool has_next_file_number_;
+  bool has_next_file_number_ = false;
-  bool has_last_sequence_;
+  bool has_max_column_family_ = false;
-  bool has_max_column_family_;
+  bool has_min_log_number_to_keep_ = false;
-  bool has_min_log_number_to_keep_;
+  bool has_last_sequence_ = false;
-
+
-  DeletedFileSet deleted_files_;
+  DeletedFiles deleted_files_;
-  std::vector<std::pair<int, FileMetaData>> new_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_drop_ = false;
-  bool is_column_family_add_;
+  bool is_column_family_add_ = false;
   std::string column_family_name_;
 
-  bool is_in_atomic_group_;
+  bool is_in_atomic_group_ = false;
-  uint32_t remaining_entries_;
+  uint32_t remaining_entries_ = 0;
 };
 
 }  // namespace rocksdb

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.HasLogNumber());
-  ASSERT_TRUE(ve.has_next_file_number());
+  ASSERT_TRUE(ve.HasNextFile());
-  ASSERT_EQ(66, ve.log_number());
+  ASSERT_EQ(66, ve.GetLogNumber());
-  ASSERT_EQ(88, ve.next_file_number());
+  ASSERT_EQ(88, ve.GetNextFile());
 }
 
 TEST_F(VersionEditTest, DbId) {

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_) {