From fb01755aa4921ec58571e87ab4de16ad610544ba Mon Sep 17 00:00:00 2001 From: Igor Canadi Date: Wed, 22 Jan 2014 10:55:16 -0800 Subject: [PATCH] Unfriending classes Summary: In this diff I made some effort to reduce usage of friending. To do that, I had to expose Compaction::inputs_ through a method inputs(). Not sure if this is a good idea, there is a trade-off. I think it's less confusing than having lots of friends. I also thought about other friendship relationships, but they are too much tangled at this point. Once you friend two classes, it's very hard to unfriend them :) Test Plan: make check Reviewers: haobo, kailiu, sdong, dhruba Reviewed By: kailiu CC: leveldb Differential Revision: https://reviews.facebook.net/D15267 --- db/compaction.cc | 6 +++--- db/compaction.h | 9 ++++++--- db/version_set.cc | 16 +++++++--------- 3 files changed, 16 insertions(+), 15 deletions(-) diff --git a/db/compaction.cc b/db/compaction.cc index 703e7aeae..536b7e233 100644 --- a/db/compaction.cc +++ b/db/compaction.cc @@ -26,7 +26,7 @@ Compaction::Compaction(Version* input_version, int level, int out_level, : level_(level), out_level_(out_level), max_output_file_size_(target_file_size), - maxGrandParentOverlapBytes_(max_grandparent_overlap_bytes), + max_grandparent_overlap_bytes_(max_grandparent_overlap_bytes), input_version_(input_version), number_levels_(input_version_->NumberLevels()), seek_compaction_(seek_compaction), @@ -64,7 +64,7 @@ bool Compaction::IsTrivialMove() const { return (level_ != out_level_ && num_input_files(0) == 1 && num_input_files(1) == 0 && - TotalFileSize(grandparents_) <= maxGrandParentOverlapBytes_); + TotalFileSize(grandparents_) <= max_grandparent_overlap_bytes_); } void Compaction::AddInputDeletions(VersionEdit* edit) { @@ -117,7 +117,7 @@ bool Compaction::ShouldStopBefore(const Slice& internal_key) { } seen_key_ = true; - if (overlapped_bytes_ > maxGrandParentOverlapBytes_) { + if (overlapped_bytes_ > max_grandparent_overlap_bytes_) { // Too much overlap for current output; start new output overlapped_bytes_ = 0; return true; diff --git a/db/compaction.h b/db/compaction.h index 5e696a053..efd6ef71f 100644 --- a/db/compaction.h +++ b/db/compaction.h @@ -33,9 +33,14 @@ class Compaction { // "which" must be either 0 or 1 int num_input_files(int which) const { return inputs_[which].size(); } + // Returns input version of the compaction + Version* input_version() const { return input_version_; } + // Return the ith input file at "level()+which" ("which" must be 0 or 1). FileMetaData* input(int which, int i) const { return inputs_[which][i]; } + std::vector* inputs(int which) { return &inputs_[which]; } + // Maximum size of files to build during this compaction. uint64_t MaxOutputFileSize() const { return max_output_file_size_; } @@ -74,8 +79,6 @@ class Compaction { bool IsFullCompaction() { return is_full_compaction_; } private: - friend class Version; - friend class VersionSet; friend class CompactionPicker; friend class UniversalCompactionPicker; friend class LevelCompactionPicker; @@ -87,7 +90,7 @@ class Compaction { int level_; int out_level_; // levels to which output files are stored uint64_t max_output_file_size_; - uint64_t maxGrandParentOverlapBytes_; + uint64_t max_grandparent_overlap_bytes_; Version* input_version_; VersionEdit* edit_; int number_levels_; diff --git a/db/version_set.cc b/db/version_set.cc index ebd2805bc..a08feb875 100644 --- a/db/version_set.cc +++ b/db/version_set.cc @@ -1994,23 +1994,21 @@ Iterator* VersionSet::MakeInputIterator(Compaction* c) { // Level-0 files have to be merged together. For other levels, // we will make a concatenating iterator per level. // TODO(opt): use concatenating iterator for level-0 if there is no overlap - const int space = (c->level() == 0 ? c->inputs_[0].size() + 1 : 2); + const int space = (c->level() == 0 ? c->inputs(0)->size() + 1 : 2); Iterator** list = new Iterator*[space]; int num = 0; for (int which = 0; which < 2; which++) { - if (!c->inputs_[which].empty()) { + if (!c->inputs(which)->empty()) { if (c->level() + which == 0) { - const std::vector& files = c->inputs_[which]; - for (size_t i = 0; i < files.size(); i++) { + for (const auto& file : *c->inputs(which)) { list[num++] = table_cache_->NewIterator( - options, storage_options_compactions_, - files[i]->number, files[i]->file_size, nullptr, - true /* for compaction */); + options, storage_options_compactions_, file->number, + file->file_size, nullptr, true /* for compaction */); } } else { // Create concatenating iterator for the files from this level list[num++] = NewTwoLevelIterator( - new Version::LevelFileNumIterator(icmp_, &c->inputs_[which]), + new Version::LevelFileNumIterator(icmp_, c->inputs(which)), &GetFileIterator, table_cache_, options, storage_options_, true /* for compaction */); } @@ -2034,7 +2032,7 @@ uint64_t VersionSet::MaxFileSizeForLevel(int level) { // in the current version bool VersionSet::VerifyCompactionFileConsistency(Compaction* c) { #ifndef NDEBUG - if (c->input_version_ != current_) { + if (c->input_version() != current_) { Log(options_->info_log, "VerifyCompactionFileConsistency version mismatch"); }