Make Compaction class easier to use

Summary:
The goal of this diff is to make Compaction class easier to use. This should also make new compaction algorithms easier to write (like CompactFiles from @yhchiang and dynamic leveled and multi-leveled universal from @sdong).

Here are couple of things demonstrating that Compaction class is hard to use:
1. we have two constructors of Compaction class
2. there's this thing called grandparents_, but it appears to only be setup for leveled compaction and not compactfiles
3. it's easy to introduce a subtle and dangerous bug like this: D36225
4. SetupBottomMostLevel() is hard to understand and it shouldn't be. See this comment: https://github.com/facebook/rocksdb/blob/afbafeaeaebfd27a0f3e992fee8e0c57d07658fa/db/compaction.cc#L236-L241. It also made it harder for @yhchiang to write CompactFiles, as evidenced by this: https://github.com/facebook/rocksdb/blob/afbafeaeaebfd27a0f3e992fee8e0c57d07658fa/db/compaction_picker.cc#L204-L210

The problem is that we create Compaction object, which holds a lot of state, and then pass it around to some functions. After those functions are done mutating, then we call couple of functions on Compaction object, like SetupBottommostLevel() and MarkFilesBeingCompacted(). It is very hard to see what's happening with all that Compaction's state while it's travelling across different functions. If you're writing a new PickCompaction() function you need to try really hard to understand what are all the functions you need to run on Compaction object and what state you need to setup.

My proposed solution is to make important parts of Compaction immutable after construction. PickCompaction() should calculate compaction inputs and then pass them onto Compaction object once they are finalized. That makes it easy to create a new compaction -- just provide all the parameters to the constructor and you're done. No need to call confusing functions after you created your object.

This diff doesn't fully achieve that goal, but it comes pretty close. Here are some of the changes:
* have one Compaction constructor instead of two.
* inputs_ is constant after construction
* MarkFilesBeingCompacted() is now private to Compaction class and automatically called on construction/destruction.
* SetupBottommostLevel() is gone. Compaction figures it out on its own based on the input.
* CompactionPicker's functions are not passing around Compaction object anymore. They are only passing around the state that they need.

Test Plan:
make check
make asan_check
make valgrind_check

Reviewers: rven, anthony, sdong, yhchiang

Reviewed By: yhchiang

Subscribers: sdong, yhchiang, dhruba, leveldb

Differential Revision: https://reviews.facebook.net/D36687

db/compaction.cc

@@ -36,83 +36,91 @@ void Compaction::SetInputVersion(Version* _input_version) {
 
   cfd_->Ref();
   input_version_->Ref();
-  edit_ = new VersionEdit();
-  edit_->SetColumnFamily(cfd_->GetID());
+  edit_.SetColumnFamily(cfd_->GetID());
 }
 
-Compaction::Compaction(int number_levels, int _start_level, int out_level,
-                       uint64_t target_file_size,
-                       uint64_t max_grandparent_overlap_bytes,
-                       uint32_t output_path_id,
-                       CompressionType output_compression, bool seek_compaction,
-                       bool deletion_compaction)
-    : start_level_(_start_level),
-      output_level_(out_level),
-      max_output_file_size_(target_file_size),
-      max_grandparent_overlap_bytes_(max_grandparent_overlap_bytes),
-      input_version_(nullptr),
-      edit_(nullptr),
-      number_levels_(number_levels),
-      cfd_(nullptr),
-      output_path_id_(output_path_id),
-      output_compression_(output_compression),
-      seek_compaction_(seek_compaction),
-      deletion_compaction_(deletion_compaction),
-      grandparent_index_(0),
-      seen_key_(false),
-      overlapped_bytes_(0),
-      base_index_(-1),
-      parent_index_(-1),
-      score_(0),
-      bottommost_level_(false),
-      is_full_compaction_(false),
-      is_manual_compaction_(false),
-      level_ptrs_(std::vector<size_t>(number_levels_)) {
-  for (int i = 0; i < number_levels_; i++) {
-    level_ptrs_[i] = 0;
+// helper function to determine if compaction is creating files at the
+// bottommost level
+bool Compaction::IsBottommostLevel(
+    int output_level, VersionStorageInfo* vstorage,
+    const std::vector<CompactionInputFiles>& inputs) {
+  if (inputs[0].level == 0 &&
+      inputs[0].files.back() != vstorage->LevelFiles(0).back()) {
+    return false;
+  }
+
+  // checks whether there are files living beyond the output_level.
+  for (int i = output_level + 1; i < vstorage->num_levels(); i++) {
+    if (vstorage->NumLevelFiles(i) > 0) {
+      return false;
+    }
   }
-  int num_levels = output_level_ - start_level_ + 1;
-  input_levels_.resize(num_levels);
-  inputs_.resize(num_levels);
-  for (int i = 0; i < num_levels; ++i) {
-    inputs_[i].level = start_level_ + i;
+  return true;
+}
+
+bool Compaction::IsFullCompaction(
+    VersionStorageInfo* vstorage,
+    const std::vector<CompactionInputFiles>& inputs) {
+  int num_files_in_compaction = 0;
+  int total_num_files = 0;
+  for (int l = 0; l < vstorage->num_levels(); l++) {
+    total_num_files += vstorage->NumLevelFiles(l);
   }
+  for (size_t i = 0; i < inputs.size(); i++) {
+    num_files_in_compaction += inputs[i].size();
+  }
+  return num_files_in_compaction == total_num_files;
 }
 
 Compaction::Compaction(VersionStorageInfo* vstorage,
-    const autovector<CompactionInputFiles>& _inputs,
-    int _start_level, int _output_level,
-    uint64_t _max_grandparent_overlap_bytes,
-    const CompactionOptions& _options,
-    bool _deletion_compaction)
-    : start_level_(_start_level),
+                       const MutableCFOptions& _mutable_cf_options,
+                       std::vector<CompactionInputFiles> _inputs,
+                       int _output_level, uint64_t _target_file_size,
+                       uint64_t _max_grandparent_overlap_bytes,
+                       uint32_t _output_path_id, CompressionType _compression,
+                       std::vector<FileMetaData*> _grandparents,
+                       bool _manual_compaction, double _score,
+                       bool _deletion_compaction)
+    : start_level_(_inputs[0].level),
       output_level_(_output_level),
-      max_output_file_size_(_options.output_file_size_limit),
+      max_output_file_size_(_target_file_size),
       max_grandparent_overlap_bytes_(_max_grandparent_overlap_bytes),
+      mutable_cf_options_(_mutable_cf_options),
       input_version_(nullptr),
       number_levels_(vstorage->num_levels()),
       cfd_(nullptr),
-      output_compression_(_options.compression),
-      seek_compaction_(false),
+      output_path_id_(_output_path_id),
+      output_compression_(_compression),
       deletion_compaction_(_deletion_compaction),
-      inputs_(_inputs),
+      inputs_(std::move(_inputs)),
+      grandparents_(std::move(_grandparents)),
       grandparent_index_(0),
       seen_key_(false),
       overlapped_bytes_(0),
-      base_index_(-1),
-      parent_index_(-1),
-      score_(0),
-      bottommost_level_(false),
-      is_full_compaction_(false),
-      is_manual_compaction_(false),
-      level_ptrs_(std::vector<size_t>(number_levels_)) {
-  for (int i = 0; i < number_levels_; i++) {
-    level_ptrs_[i] = 0;
+      score_(_score),
+      bottommost_level_(IsBottommostLevel(output_level_, vstorage, inputs_)),
+      is_full_compaction_(IsFullCompaction(vstorage, inputs_)),
+      is_manual_compaction_(_manual_compaction),
+      level_ptrs_(std::vector<size_t>(number_levels_, 0)) {
+  MarkFilesBeingCompacted(true);
+
+#ifndef NDEBUG
+  for (size_t i = 1; i < inputs_.size(); ++i) {
+    assert(inputs_[i].level > inputs_[i - 1].level);
+  }
+#endif
+
+  // setup input_levels_
+  {
+    input_levels_.resize(num_input_levels());
+    for (size_t which = 0; which < num_input_levels(); which++) {
+      DoGenerateLevelFilesBrief(&input_levels_[which], inputs_[which].files,
+                                &arena_);
+    }
   }
 }
 
 Compaction::~Compaction() {
-  delete edit_;
   if (input_version_ != nullptr) {
     input_version_->Unref();
   }
@@ -123,14 +131,6 @@ Compaction::~Compaction() {
   }
 }
 
-void Compaction::GenerateFileLevels() {
-  input_levels_.resize(num_input_levels());
-  for (size_t which = 0; which < num_input_levels(); which++) {
-    DoGenerateLevelFilesBrief(&input_levels_[which], inputs_[which].files,
-                              &arena_);
-  }
-}
-
 bool Compaction::InputCompressionMatchesOutput() const {
   int base_level = input_version_->storage_info()->base_level();
   bool matches = (GetCompressionType(*cfd_->ioptions(), start_level_,
@@ -144,25 +144,14 @@ bool Compaction::InputCompressionMatchesOutput() const {
 }
 
 bool Compaction::IsTrivialMove() const {
-  // If start_level_== output_level_, the purpose is to force compaction
-  // filter to be applied to that level, and thus cannot be a trivia move.
-  if (start_level_ == output_level_) {
-    return false;
-  }
-  // If compaction involves more than one file, it is not trivial move.
-  if (num_input_files(0) != 1) {
-    return false;
-  }
-  for (size_t l = 1u; l < num_input_levels(); l++) {
-    if (num_input_files(l) != 0) {
-      return false;
-    }
-  }
-
   // Avoid a move if there is lots of overlapping grandparent data.
   // Otherwise, the move could create a parent file that will require
   // a very expensive merge later on.
-  return (input(0, 0)->fd.GetPathId() == GetOutputPathId() &&
+  // If start_level_== output_level_, the purpose is to force compaction
+  // filter to be applied to that level, and thus cannot be a trivia move.
+  return (start_level_ != output_level_ && num_input_levels() == 1 &&
+          num_input_files(0) == 1 &&
+          input(0, 0)->fd.GetPathId() == GetOutputPathId() &&
           InputCompressionMatchesOutput() &&
           TotalFileSize(grandparents_) <= max_grandparent_overlap_bytes_);
 }
@@ -240,31 +229,6 @@ void Compaction::MarkFilesBeingCompacted(bool mark_as_compacted) {
   }
 }
 
-// Is this compaction producing files at the bottommost level?
-void Compaction::SetupBottomMostLevel(VersionStorageInfo* vstorage,
-                                      bool is_manual, bool level0_only) {
-  if (level0_only) {
-    // If universal compaction style is used and manual
-    // compaction is occuring, then we are guaranteed that
-    // all files will be picked in a single compaction
-    // run. We can safely set bottommost_level_ = true.
-    // If it is not manual compaction, then bottommost_level_
-    // is already set when the Compaction was created.
-    if (is_manual) {
-      bottommost_level_ = true;
-    }
-    return;
-  }
-  bottommost_level_ = true;
-  // checks whether there are files living beyond the output_level.
-  for (int i = output_level_ + 1; i < number_levels_; i++) {
-    if (vstorage->NumLevelFiles(i) > 0) {
-      bottommost_level_ = false;
-      break;
-    }
-  }
-}
-
 // Sample output:
 // If compacting 3 L0 files, 2 L3 files and 1 L4 file, and outputting to L5,
 // print: "3@0 + 2@3 + 1@4 files to L5"
@@ -292,6 +256,7 @@ const char* Compaction::InputLevelSummary(
 }
 
 void Compaction::ReleaseCompactionFiles(Status status) {
+  MarkFilesBeingCompacted(false);
   cfd_->compaction_picker()->ReleaseCompactionFiles(this, status);
 }
 
@@ -323,9 +288,9 @@ int InputSummary(const std::vector<FileMetaData*>& files, char* output,
 void Compaction::Summary(char* output, int len) {
   int write =
       snprintf(output, len, "Base version %" PRIu64
-                            " Base level %d, seek compaction:%d, inputs: [",
+                            " Base level %d, inputs: [",
                input_version_->GetVersionNumber(),
-               start_level_, seek_compaction_);
+               start_level_);
   if (write < 0 || write >= len) {
     return;
   }
@@ -365,15 +330,4 @@ uint64_t Compaction::OutputFilePreallocationSize(
   return preallocation_size * 1.1;
 }
 
-Compaction* Compaction::TEST_NewCompaction(
-    int num_levels, int start_level, int out_level, uint64_t target_file_size,
-    uint64_t max_grandparent_overlap_bytes, uint32_t output_path_id,
-    CompressionType output_compression, bool seek_compaction,
-    bool deletion_compaction) {
-  return new Compaction(num_levels, start_level, out_level, target_file_size,
-                        max_grandparent_overlap_bytes, output_path_id,
-                        output_compression, seek_compaction,
-                        deletion_compaction);
-}
-
 }  // namespace rocksdb

db/compaction.h

@@ -34,11 +34,13 @@ class VersionStorageInfo;
 class Compaction {
  public:
   Compaction(VersionStorageInfo* input_version,
-    const autovector<CompactionInputFiles>& inputs,
-    int start_level, int output_level,
-    uint64_t max_grandparent_overlap_bytes,
-    const CompactionOptions& options,
-    bool deletion_compaction);
+             const MutableCFOptions& mutable_cf_options,
+             std::vector<CompactionInputFiles> inputs, int output_level,
+             uint64_t target_file_size, uint64_t max_grandparent_overlap_bytes,
+             uint32_t output_path_id, CompressionType compression,
+             std::vector<FileMetaData*> grandparents,
+             bool manual_compaction = false, double score = -1,
+             bool deletion_compaction = false);
 
   // No copying allowed
   Compaction(const Compaction&) = delete;
@@ -62,7 +64,7 @@ class Compaction {
 
   // Return the object that holds the edits to the descriptor done
   // by this compaction.
-  VersionEdit* edit() const { return edit_; }
+  VersionEdit* edit() { return &edit_; }
 
   // Returns the number of input files associated to the specified
   // compaction input level.
@@ -113,10 +115,6 @@ class Compaction {
   // Whether need to write output file to second DB path.
   uint32_t GetOutputPathId() const { return output_path_id_; }
 
-  // Generate input_levels_ from inputs_
-  // Should be called when inputs_ is stable
-  void GenerateFileLevels();
-
   // Is this a trivial compaction that can be implemented by just
   // moving a single input file to the next level (no merging or splitting)
   bool IsTrivialMove() const;
@@ -158,8 +156,6 @@ class Compaction {
   // Was this compaction triggered manually by the client?
   bool IsManualCompaction() { return is_manual_compaction_; }
 
-  void SetOutputPathId(uint32_t path_id) { output_path_id_ = path_id; }
-
   // Return the MutableCFOptions that should be used throughout the compaction
   // procedure
   const MutableCFOptions* mutable_cf_options() { return &mutable_cf_options_; }
@@ -171,42 +167,27 @@ class Compaction {
 
   void SetInputVersion(Version* input_version);
 
-  // mark (or clear) all files that are being compacted
-  void MarkFilesBeingCompacted(bool mark_as_compacted);
-
-  // Initialize whether the compaction is producing files at the
-  // bottommost level.
-  //
-  // @see BottomMostLevel()
-  void SetupBottomMostLevel(VersionStorageInfo* vstorage, bool is_manual,
-                            bool level0_only);
-
-  static Compaction* TEST_NewCompaction(
-      int num_levels, int start_level, int out_level, uint64_t target_file_size,
-      uint64_t max_grandparent_overlap_bytes, uint32_t output_path_id,
-      CompressionType output_compression, bool seek_compaction = false,
-      bool deletion_compaction = false);
-
-  CompactionInputFiles* TEST_GetInputFiles(int l) {
-    return &inputs_[l];
-  }
-
   struct InputLevelSummaryBuffer {
     char buffer[128];
   };
 
   const char* InputLevelSummary(InputLevelSummaryBuffer* scratch) const;
 
+  // In case of compaction error, reset the nextIndex that is used
+  // to pick up the next file to be compacted from files_by_size_
+  void ResetNextCompactionIndex();
+
  private:
-  friend class CompactionPicker;
-  friend class UniversalCompactionPicker;
-  friend class FIFOCompactionPicker;
-  friend class LevelCompactionPicker;
+  // mark (or clear) all files that are being compacted
+  void MarkFilesBeingCompacted(bool mark_as_compacted);
 
-  Compaction(int num_levels, int start_level, int out_level,
-             uint64_t target_file_size, uint64_t max_grandparent_overlap_bytes,
-             uint32_t output_path_id, CompressionType output_compression,
-             bool seek_compaction = false, bool deletion_compaction = false);
+  // helper function to determine if compaction with inputs and storage is
+  // bottommost
+  static bool IsBottommostLevel(
+      int output_level, VersionStorageInfo* vstorage,
+      const std::vector<CompactionInputFiles>& inputs);
+  static bool IsFullCompaction(VersionStorageInfo* vstorage,
+                               const std::vector<CompactionInputFiles>& inputs);
 
   const int start_level_;    // the lowest level to be compacted
   const int output_level_;  // levels to which output files are stored
@@ -214,43 +195,38 @@ class Compaction {
   uint64_t max_grandparent_overlap_bytes_;
   MutableCFOptions mutable_cf_options_;
   Version* input_version_;
-  VersionEdit* edit_;
-  int number_levels_;
+  VersionEdit edit_;
+  const int number_levels_;
   ColumnFamilyData* cfd_;
   Arena arena_;          // Arena used to allocate space for file_levels_
 
-  uint32_t output_path_id_;
+  const uint32_t output_path_id_;
   CompressionType output_compression_;
-  bool seek_compaction_;
   // If true, then the comaction can be done by simply deleting input files.
-  bool deletion_compaction_;
+  const bool deletion_compaction_;
 
-  // Compaction input files organized by level.
-  autovector<CompactionInputFiles> inputs_;
+  // Compaction input files organized by level. Constant after construction
+  const std::vector<CompactionInputFiles> inputs_;
 
   // A copy of inputs_, organized more closely in memory
   autovector<LevelFilesBrief, 2> input_levels_;
 
   // State used to check for number of of overlapping grandparent files
   // (grandparent == "output_level_ + 1")
-  // This vector is updated by Version::GetOverlappingInputs().
   std::vector<FileMetaData*> grandparents_;
   size_t grandparent_index_;   // Index in grandparent_starts_
   bool seen_key_;              // Some output key has been seen
   uint64_t overlapped_bytes_;  // Bytes of overlap between current output
                                // and grandparent files
-  int base_index_;    // index of the file in files_[start_level_]
-  int parent_index_;  // index of some file with same range in
-                      // files_[start_level_+1]
-  double score_;      // score that was used to pick this compaction.
+  const double score_;         // score that was used to pick this compaction.
 
   // Is this compaction creating a file in the bottom most level?
-  bool bottommost_level_;
+  const bool bottommost_level_;
   // Does this compaction include all sst files?
-  bool is_full_compaction_;
+  const bool is_full_compaction_;
 
   // Is this compaction requested by the client?
-  bool is_manual_compaction_;
+  const bool is_manual_compaction_;
 
   // "level_ptrs_" holds indices into "input_version_->levels_", where each
   // index remembers which file of an associated level we are currently used
@@ -259,10 +235,6 @@ class Compaction {
   // records indices for all levels beyond "output_level_".
   std::vector<size_t> level_ptrs_;
 
-  // In case of compaction error, reset the nextIndex that is used
-  // to pick up the next file to be compacted from files_by_size_
-  void ResetNextCompactionIndex();
-
   // Does input compression match the output compression?
   bool InputCompressionMatchesOutput() const;
 };

db/compaction_job.cc

@@ -245,7 +245,6 @@ void CompactionJob::Prepare() {
   auto* compaction = compact_->compaction;
 
   // Generate file_levels_ for compaction berfore making Iterator
-  compaction->GenerateFileLevels();
   ColumnFamilyData* cfd = compact_->compaction->column_family_data();
   assert(cfd != nullptr);
   {

db/compaction_job_test.cc

@@ -66,6 +66,9 @@ class CompactionJobTest : public testing::Test {
         auto key = ToString(i * (kKeysPerFile / 2) + k);
         auto value = ToString(i * kKeysPerFile + k);
         InternalKey internal_key(key, ++sequence_number, kTypeValue);
+        // This is how the key will look like once it's written in bottommost
+        // file
+        InternalKey bottommost_internal_key(key, 0, kTypeValue);
         if (k == 0) {
           smallest = internal_key;
           smallest_seqno = sequence_number;
@@ -74,7 +77,7 @@ class CompactionJobTest : public testing::Test {
           largest_seqno = sequence_number;
         }
         std::pair<std::string, std::string> key_value(
-            {internal_key.Encode().ToString(), value});
+            {bottommost_internal_key.Encode().ToString(), value});
         contents.insert(key_value);
         if (i == 1 || k < kKeysPerFile / 2) {
           expected_results.insert(key_value);
@@ -143,15 +146,15 @@ TEST_F(CompactionJobTest, Simple) {
   auto files = cfd->current()->storage_info()->LevelFiles(0);
   ASSERT_EQ(2U, files.size());
 
-  std::unique_ptr<Compaction> compaction(Compaction::TEST_NewCompaction(
-      7, 0, 1, 1024 * 1024, 10, 0, kNoCompression));
+  CompactionInputFiles compaction_input_files;
+  compaction_input_files.level = 0;
+  compaction_input_files.files.push_back(files[0]);
+  compaction_input_files.files.push_back(files[1]);
+  std::unique_ptr<Compaction> compaction(new Compaction(
+      cfd->current()->storage_info(), *cfd->GetLatestMutableCFOptions(),
+      {compaction_input_files}, 1, 1024 * 1024, 10, 0, kNoCompression, {}));
   compaction->SetInputVersion(cfd->current());
 
-  auto compaction_input_files = compaction->TEST_GetInputFiles(0);
-  compaction_input_files->level = 0;
-  compaction_input_files->files.push_back(files[0]);
-  compaction_input_files->files.push_back(files[1]);
-
   SnapshotList snapshots;
   int yield_callback_called = 0;
   std::function<uint64_t()> yield_callback = [&]() {

db/compaction_picker.h

@@ -97,14 +97,14 @@ class CompactionPicker {
   // Takes a list of CompactionInputFiles and returns a Compaction object.
   Compaction* FormCompaction(
       const CompactionOptions& compact_options,
-      const autovector<CompactionInputFiles>& input_files,
-      int output_level, VersionStorageInfo* vstorage,
-      const MutableCFOptions& mutable_cf_options) const;
+      const std::vector<CompactionInputFiles>& input_files, int output_level,
+      VersionStorageInfo* vstorage, const MutableCFOptions& mutable_cf_options,
+      uint32_t output_path_id) const;
 
   // Converts a set of compaction input file numbers into
   // a list of CompactionInputFiles.
   Status GetCompactionInputsFromFileNumbers(
-      autovector<CompactionInputFiles>* input_files,
+      std::vector<CompactionInputFiles>* input_files,
       std::unordered_set<uint64_t>* input_set,
       const VersionStorageInfo* vstorage,
       const CompactionOptions& compact_options) const;
@@ -136,16 +136,25 @@ class CompactionPicker {
   //
   // Will return false if it is impossible to apply this compaction.
   bool ExpandWhileOverlapping(const std::string& cf_name,
-                              VersionStorageInfo* vstorage, Compaction* c);
+                              VersionStorageInfo* vstorage,
+                              CompactionInputFiles* inputs);
 
   // Returns true if any one of the parent files are being compacted
   bool RangeInCompaction(VersionStorageInfo* vstorage,
                          const InternalKey* smallest,
                          const InternalKey* largest, int level, int* index);
 
-  void SetupOtherInputs(const std::string& cf_name,
+  bool SetupOtherInputs(const std::string& cf_name,
                         const MutableCFOptions& mutable_cf_options,
-                        VersionStorageInfo* vstorage, Compaction* c);
+                        VersionStorageInfo* vstorage,
+                        CompactionInputFiles* inputs,
+                        CompactionInputFiles* output_level_inputs,
+                        int* parent_index, int base_index);
+
+  void GetGrandparents(VersionStorageInfo* vstorage,
+                       const CompactionInputFiles& inputs,
+                       const CompactionInputFiles& output_level_inputs,
+                       std::vector<FileMetaData*>* grandparents);
 
   const ImmutableCFOptions& ioptions_;
 
@@ -190,13 +199,14 @@ class LevelCompactionPicker : public CompactionPicker {
                             int level);
 
  private:
-  // For the specfied level, pick a compaction.
-  // Returns nullptr if there is no compaction to be done.
+  // For the specfied level, pick a file that we want to compact.
+  // Returns false if there is no file to compact.
+  // If it returns true, inputs->files.size() will be exactly one.
   // If level is 0 and there is already a compaction on that level, this
-  // function will return nullptr.
-  Compaction* PickCompactionBySize(const MutableCFOptions& mutable_cf_options,
-                                   VersionStorageInfo* vstorage, int level,
-                                   double score);
+  // function will return false.
+  bool PickCompactionBySize(VersionStorageInfo* vstorage, int level,
+                            int output_level, CompactionInputFiles* inputs,
+                            int* parent_index, int* base_index);
 };
 
 #ifndef ROCKSDB_LITE

db/compaction_picker_test.cc

@@ -229,7 +229,7 @@ TEST_F(CompactionPickerTest, Level0TriggerDynamic) {
   ASSERT_EQ(2U, compaction->num_input_files(0));
   ASSERT_EQ(1U, compaction->input(0, 0)->fd.GetNumber());
   ASSERT_EQ(2U, compaction->input(0, 1)->fd.GetNumber());
-  ASSERT_EQ(num_levels, static_cast<int>(compaction->num_input_levels()));
+  ASSERT_EQ(1, static_cast<int>(compaction->num_input_levels()));
   ASSERT_EQ(num_levels - 1, compaction->output_level());
 }
 
@@ -253,7 +253,7 @@ TEST_F(CompactionPickerTest, Level0TriggerDynamic2) {
   ASSERT_EQ(2U, compaction->num_input_files(0));
   ASSERT_EQ(1U, compaction->input(0, 0)->fd.GetNumber());
   ASSERT_EQ(2U, compaction->input(0, 1)->fd.GetNumber());
-  ASSERT_EQ(num_levels - 1, static_cast<int>(compaction->num_input_levels()));
+  ASSERT_EQ(1, static_cast<int>(compaction->num_input_levels()));
   ASSERT_EQ(num_levels - 2, compaction->output_level());
 }
 
@@ -278,7 +278,7 @@ TEST_F(CompactionPickerTest, Level0TriggerDynamic3) {
   ASSERT_EQ(2U, compaction->num_input_files(0));
   ASSERT_EQ(1U, compaction->input(0, 0)->fd.GetNumber());
   ASSERT_EQ(2U, compaction->input(0, 1)->fd.GetNumber());
-  ASSERT_EQ(num_levels - 2, static_cast<int>(compaction->num_input_levels()));
+  ASSERT_EQ(1, static_cast<int>(compaction->num_input_levels()));
   ASSERT_EQ(num_levels - 3, compaction->output_level());
 }
 
@@ -306,10 +306,11 @@ TEST_F(CompactionPickerTest, Level0TriggerDynamic4) {
   ASSERT_EQ(2U, compaction->num_input_files(0));
   ASSERT_EQ(1U, compaction->input(0, 0)->fd.GetNumber());
   ASSERT_EQ(2U, compaction->input(0, 1)->fd.GetNumber());
-  ASSERT_EQ(2U, compaction->num_input_files(num_levels - 3));
-  ASSERT_EQ(5U, compaction->input(num_levels - 3, 0)->fd.GetNumber());
-  ASSERT_EQ(6U, compaction->input(num_levels - 3, 1)->fd.GetNumber());
-  ASSERT_EQ(num_levels - 2, static_cast<int>(compaction->num_input_levels()));
+  ASSERT_EQ(2U, compaction->num_input_files(1));
+  ASSERT_EQ(num_levels - 3, compaction->level(1));
+  ASSERT_EQ(5U, compaction->input(1, 0)->fd.GetNumber());
+  ASSERT_EQ(6U, compaction->input(1, 1)->fd.GetNumber());
+  ASSERT_EQ(2, static_cast<int>(compaction->num_input_levels()));
   ASSERT_EQ(num_levels - 3, compaction->output_level());
 }
 

db/db_impl.cc

@@ -1399,7 +1399,7 @@ Status DBImpl::CompactFilesImpl(
     return s;
   }
 
-  autovector<CompactionInputFiles> input_files;
+  std::vector<CompactionInputFiles> input_files;
   s = cfd->compaction_picker()->GetCompactionInputsFromFileNumbers(
       &input_files, &input_set, version->storage_info(), compact_options);
   if (!s.ok()) {
@@ -1420,12 +1420,10 @@ Status DBImpl::CompactFilesImpl(
   unique_ptr<Compaction> c;
   assert(cfd->compaction_picker());
   c.reset(cfd->compaction_picker()->FormCompaction(
-        compact_options, input_files,
-        output_level, version->storage_info(),
-        *cfd->GetLatestMutableCFOptions()));
+      compact_options, input_files, output_level, version->storage_info(),
+      *cfd->GetLatestMutableCFOptions(), output_path_id));
   assert(c);
   c->SetInputVersion(version);
-  c->SetOutputPathId(static_cast<uint32_t>(output_path_id));
   // deletion compaction currently not allowed in CompactFiles.
   assert(!c->IsDeletionCompaction());
 

db/db_test.cc

@@ -12457,6 +12457,7 @@ TEST_F(DBTest, CompressLevelCompaction) {
 }  // namespace rocksdb
 
 int main(int argc, char** argv) {
+  rocksdb::port::InstallStackTraceHandler();
   ::testing::InitGoogleTest(&argc, argv);
   return RUN_ALL_TESTS();
 }