From 09d982f9e0978cb4dddc7e48de6056e7ce3b9e66 Mon Sep 17 00:00:00 2001 From: Andres Notzli Date: Tue, 25 Aug 2015 12:29:44 -0700 Subject: [PATCH] Fix compact_files_example Summary: See task #7983654. The example was triggering an assert in compaction job because the compaction was not marked as manual. With this patch, CompactionPicker::FormCompaction() marks compactions as manual. This patch also fixes a couple of typos, adds optimistic_transaction_example to .gitignore and librocksdb as a dependency for examples. Adding librocksdb as a dependency makes sure that the examples are built with the latest changes in librocksdb. Test Plan: make clean && cd examples && make all && ./compact_files_example Reviewers: rven, sdong, anthony, igor, yhchiang Reviewed By: yhchiang Subscribers: dhruba, leveldb Differential Revision: https://reviews.facebook.net/D45117 --- db/compaction_picker.cc | 8 ++++---- db/compaction_picker.h | 17 ++++++++--------- db/db_impl.cc | 6 +++--- db/db_universal_compaction_test.cc | 7 ++++--- examples/.gitignore | 1 + examples/Makefile | 17 ++++++++++------- examples/compact_files_example.cc | 11 +++++------ include/rocksdb/db.h | 8 ++++---- util/db_test_util.cc | 2 +- 9 files changed, 40 insertions(+), 37 deletions(-) diff --git a/db/compaction_picker.cc b/db/compaction_picker.cc index c5be487ff..02f53e546 100644 --- a/db/compaction_picker.cc +++ b/db/compaction_picker.cc @@ -261,10 +261,10 @@ Compaction* CompactionPicker::FormCompaction( mutable_cf_options.MaxGrandParentOverlapBytes(output_level + 1) : std::numeric_limits::max(); assert(input_files.size()); - return new Compaction(vstorage, mutable_cf_options, input_files, output_level, - compact_options.output_file_size_limit, - max_grandparent_overlap_bytes, output_path_id, - compact_options.compression, /* grandparents */ {}); + return new Compaction( + vstorage, mutable_cf_options, input_files, output_level, + compact_options.output_file_size_limit, max_grandparent_overlap_bytes, + output_path_id, compact_options.compression, /* grandparents */ {}, true); } Status CompactionPicker::GetCompactionInputsFromFileNumbers( diff --git a/db/compaction_picker.h b/db/compaction_picker.h index 61ed99505..1d1abe30c 100644 --- a/db/compaction_picker.h +++ b/db/compaction_picker.h @@ -8,22 +8,20 @@ // found in the LICENSE file. See the AUTHORS file for names of contributors. #pragma once -#include + #include #include +#include #include +#include -#include "db/version_set.h" #include "db/compaction.h" -#include "rocksdb/status.h" -#include "rocksdb/options.h" +#include "db/version_set.h" #include "rocksdb/env.h" +#include "rocksdb/options.h" +#include "rocksdb/status.h" #include "util/mutable_cf_options.h" -#include -#include -#include -#include namespace rocksdb { @@ -90,7 +88,8 @@ class CompactionPicker { // Returns true if any one of the specified files are being compacted bool FilesInCompaction(const std::vector& files); - // Takes a list of CompactionInputFiles and returns a Compaction object. + // Takes a list of CompactionInputFiles and returns a (manual) Compaction + // object. Compaction* FormCompaction( const CompactionOptions& compact_options, const std::vector& input_files, int output_level, diff --git a/db/db_impl.cc b/db/db_impl.cc index 411edbbc1..a41210888 100644 --- a/db/db_impl.cc +++ b/db/db_impl.cc @@ -1469,7 +1469,7 @@ Status DBImpl::CompactRange(const CompactRangeOptions& options, } else { for (int level = 0; level <= max_level_with_files; level++) { int output_level; - // in case the compaction is unversal or if we're compacting the + // in case the compaction is universal or if we're compacting the // bottom-most level, the output level will be the same as input one. // level 0 can never be the bottommost level (i.e. if all files are in // level 0, we will compact to level 1) @@ -1485,8 +1485,8 @@ Status DBImpl::CompactRange(const CompactRangeOptions& options, BottommostLevelCompaction::kIfHaveCompactionFilter && cfd->ioptions()->compaction_filter == nullptr && cfd->ioptions()->compaction_filter_factory == nullptr) { - // Skip bottommost level compaction since we dont have - // compaction filter + // Skip bottommost level compaction since we don't have a compaction + // filter continue; } output_level = level; diff --git a/db/db_universal_compaction_test.cc b/db/db_universal_compaction_test.cc index 16d758b00..da3485f33 100644 --- a/db/db_universal_compaction_test.cc +++ b/db/db_universal_compaction_test.cc @@ -335,9 +335,10 @@ TEST_P(DBTestUniversalCompaction, CompactFilesOnUniversalCompaction) { } // expect fail since universal compaction only allow L0 output - ASSERT_TRUE(!dbfull()->CompactFiles( - CompactionOptions(), handles_[1], - compaction_input_file_names, 1).ok()); + ASSERT_FALSE(dbfull() + ->CompactFiles(CompactionOptions(), handles_[1], + compaction_input_file_names, 1) + .ok()); // expect ok and verify the compacted files no longer exist. ASSERT_OK(dbfull()->CompactFiles( diff --git a/examples/.gitignore b/examples/.gitignore index 7083aa155..1c63561a6 100644 --- a/examples/.gitignore +++ b/examples/.gitignore @@ -3,3 +3,4 @@ simple_example c_simple_example compact_files_example transaction_example +optimistic_transaction_example diff --git a/examples/Makefile b/examples/Makefile index 0757f5f03..0882f83e6 100644 --- a/examples/Makefile +++ b/examples/Makefile @@ -1,29 +1,32 @@ include ../make_config.mk -.PHONY: clean +.PHONY: clean librocksdb all: simple_example column_families_example compact_files_example c_simple_example optimistic_transaction_example transaction_example -simple_example: simple_example.cc +simple_example: librocksdb simple_example.cc $(CXX) $(CXXFLAGS) $@.cc -o$@ ../librocksdb.a -I../include -O2 -std=c++11 $(PLATFORM_LDFLAGS) $(PLATFORM_CXXFLAGS) $(EXEC_LDFLAGS) -column_families_example: column_families_example.cc +column_families_example: librocksdb column_families_example.cc $(CXX) $(CXXFLAGS) $@.cc -o$@ ../librocksdb.a -I../include -O2 -std=c++11 $(PLATFORM_LDFLAGS) $(PLATFORM_CXXFLAGS) $(EXEC_LDFLAGS) -compact_files_example: compact_files_example.cc +compact_files_example: librocksdb compact_files_example.cc $(CXX) $(CXXFLAGS) $@.cc -o$@ ../librocksdb.a -I../include -O2 -std=c++11 $(PLATFORM_LDFLAGS) $(PLATFORM_CXXFLAGS) $(EXEC_LDFLAGS) .c.o: $(CC) $(CFLAGS) -c $< -o $@ -I../include -c_simple_example: c_simple_example.o +c_simple_example: librocksdb c_simple_example.o $(CXX) $@.o -o$@ ../librocksdb.a $(PLATFORM_LDFLAGS) $(EXEC_LDFLAGS) -optimistic_transaction_example: optimistic_transaction_example.cc +optimistic_transaction_example: librocksdb optimistic_transaction_example.cc $(CXX) $(CXXFLAGS) $@.cc -o$@ ../librocksdb.a -I../include -O2 -std=c++11 $(PLATFORM_LDFLAGS) $(PLATFORM_CXXFLAGS) $(EXEC_LDFLAGS) -transaction_example: transaction_example.cc +transaction_example: librocksdb transaction_example.cc $(CXX) $(CXXFLAGS) $@.cc -o$@ ../librocksdb.a -I../include -O2 -std=c++11 $(PLATFORM_LDFLAGS) $(PLATFORM_CXXFLAGS) $(EXEC_LDFLAGS) clean: rm -rf ./simple_example ./column_families_example ./compact_files_example ./c_simple_example c_simple_example.o ./optimistic_transaction_example ./transaction_example + +librocksdb: + cd .. && $(MAKE) librocksdb.a diff --git a/examples/compact_files_example.cc b/examples/compact_files_example.cc index b9e75562a..6c0456675 100644 --- a/examples/compact_files_example.cc +++ b/examples/compact_files_example.cc @@ -67,9 +67,9 @@ class FullCompactor : public Compactor { options_.target_file_size_base; } - // When flush happens, it determins whether to trigger compaction. - // If triggered_writes_stop is true, it will also set the retry - // flag of compaction-task to true. + // When flush happens, it determines whether to trigger compaction. If + // triggered_writes_stop is true, it will also set the retry flag of + // compaction-task to true. void OnFlushCompleted( DB* db, const FlushJobInfo& info) override { CompactionTask* task = PickCompaction(db, info.cf_name); @@ -108,7 +108,8 @@ class FullCompactor : public Compactor { } static void CompactFiles(void* arg) { - CompactionTask* task = reinterpret_cast(arg); + std::unique_ptr task( + reinterpret_cast(arg)); assert(task); assert(task->db); Status s = task->db->CompactFiles( @@ -124,8 +125,6 @@ class FullCompactor : public Compactor { task->db, task->column_family_name); task->compactor->ScheduleCompaction(new_task); } - // release the task - delete task; } private: diff --git a/include/rocksdb/db.h b/include/rocksdb/db.h index 0e2ffc43c..3824e5b7f 100644 --- a/include/rocksdb/db.h +++ b/include/rocksdb/db.h @@ -489,10 +489,10 @@ class DB { return SetOptions(DefaultColumnFamily(), new_options); } - // CompactFiles() inputs a list of files specified by file numbers - // and compacts them to the specified level. Note that the behavior - // is different from CompactRange in that CompactFiles() will - // perform the compaction job using the CURRENT thread. + // CompactFiles() inputs a list of files specified by file numbers and + // compacts them to the specified level. Note that the behavior is different + // from CompactRange() in that CompactFiles() performs the compaction job + // using the CURRENT thread. // // @see GetDataBaseMetaData // @see GetColumnFamilyMetaData diff --git a/util/db_test_util.cc b/util/db_test_util.cc index 5e551a237..ded303383 100644 --- a/util/db_test_util.cc +++ b/util/db_test_util.cc @@ -188,7 +188,7 @@ Options DBTestBase::CurrentOptions( Options DBTestBase::CurrentOptions( const Options& defaultOptions, const anon::OptionsOverride& options_override) { - // this redudant copy is to minimize code change w/o having lint error. + // this redundant copy is to minimize code change w/o having lint error. Options options = defaultOptions; XFUNC_TEST("", "dbtest_options", inplace_options1, GetXFTestOptions, reinterpret_cast(&options),