From 99404df8b22bab445b3ecb536e05d381901fcb6b Mon Sep 17 00:00:00 2001 From: Mingyu Chen Date: Mon, 30 Nov 2020 22:02:03 +0800 Subject: [PATCH] [Bug][Compaction] Fix bug that output rowset is not deleted after compaction failure (#4964) This CL fix 2 bugs: 1. When the compaction fails, we must explicitly delete the output rowset, otherwise the GC logic cannot process these rows. 2. Base compaction failed if compaction process include some delete version in SegmentV2, Because the number of filtered rows is wrong. --- be/src/olap/base_compaction.cpp | 9 +------ be/src/olap/base_compaction.h | 4 +-- be/src/olap/compaction.cpp | 27 ++++++++++++------- be/src/olap/compaction.h | 10 ++++--- be/src/olap/cumulative_compaction.cpp | 9 +------ be/src/olap/cumulative_compaction.h | 4 +-- be/src/olap/olap_common.h | 3 +++ be/src/olap/reader.h | 4 ++- .../doris/catalog/CreateFunctionTest.java | 2 +- 9 files changed, 34 insertions(+), 38 deletions(-) diff --git a/be/src/olap/base_compaction.cpp b/be/src/olap/base_compaction.cpp index 3418891007..5e4869ad96 100644 --- a/be/src/olap/base_compaction.cpp +++ b/be/src/olap/base_compaction.cpp @@ -28,13 +28,6 @@ BaseCompaction::BaseCompaction(TabletSharedPtr tablet, const std::string& label, BaseCompaction::~BaseCompaction() {} -OLAPStatus BaseCompaction::compact() { - RETURN_NOT_OK(prepare_compact()); - RETURN_NOT_OK(execute_compact()); - - return OLAP_SUCCESS; -} - OLAPStatus BaseCompaction::prepare_compact() { if (!_tablet->init_succeeded()) { return OLAP_ERR_INPUT_PARAMETER_ERROR; @@ -56,7 +49,7 @@ OLAPStatus BaseCompaction::prepare_compact() { return OLAP_SUCCESS; } -OLAPStatus BaseCompaction::execute_compact() { +OLAPStatus BaseCompaction::execute_compact_impl() { MutexLock lock(_tablet->get_base_lock(), TRY_LOCK); if (!lock.own_lock()) { LOG(WARNING) << "another base compaction is running. tablet=" << _tablet->full_name(); diff --git a/be/src/olap/base_compaction.h b/be/src/olap/base_compaction.h index 8b4c33ed19..54088ea48d 100644 --- a/be/src/olap/base_compaction.h +++ b/be/src/olap/base_compaction.h @@ -33,10 +33,8 @@ public: const std::shared_ptr& parent_tracker); ~BaseCompaction() override; - OLAPStatus compact() override; - OLAPStatus prepare_compact() override; - OLAPStatus execute_compact() override; + OLAPStatus execute_compact_impl() override; std::vector get_input_rowsets() { return _input_rowsets; } diff --git a/be/src/olap/compaction.cpp b/be/src/olap/compaction.cpp index e5534def35..719f06bf00 100644 --- a/be/src/olap/compaction.cpp +++ b/be/src/olap/compaction.cpp @@ -37,6 +37,20 @@ Compaction::Compaction(TabletSharedPtr tablet, const std::string& label, Compaction::~Compaction() {} +OLAPStatus Compaction::compact() { + RETURN_NOT_OK(prepare_compact()); + RETURN_NOT_OK(execute_compact()); + return OLAP_SUCCESS; +} + +OLAPStatus Compaction::execute_compact() { + OLAPStatus st = execute_compact_impl(); + if (st != OLAP_SUCCESS) { + gc_output_rowset(); + } + return st; +} + OLAPStatus Compaction::do_compaction(int64_t permits) { TRACE("start to do compaction"); _tablet->data_dir()->disks_compaction_score_increment(permits); @@ -165,17 +179,10 @@ void Compaction::modify_rowsets() { _tablet->save_meta(); } -OLAPStatus Compaction::gc_unused_rowsets() { - StorageEngine* storage_engine = StorageEngine::instance(); - if (_state != CompactionState::SUCCESS) { - storage_engine->add_unused_rowset(_output_rowset); - return OLAP_SUCCESS; +void Compaction::gc_output_rowset() { + if (_state != CompactionState::SUCCESS && _output_rowset != nullptr) { + StorageEngine::instance()->add_unused_rowset(_output_rowset); } - for (auto& rowset : _input_rowsets) { - storage_engine->add_unused_rowset(rowset); - } - _input_rowsets.clear(); - return OLAP_SUCCESS; } // Find the longest consecutive version path in "rowset", from begining. diff --git a/be/src/olap/compaction.h b/be/src/olap/compaction.h index a2fedce0a9..c84105a317 100644 --- a/be/src/olap/compaction.h +++ b/be/src/olap/compaction.h @@ -41,17 +41,19 @@ class Merger; // 1. pick rowsets satisfied to compact // 2. do compaction // 3. modify rowsets -// 4. gc unused rowsets +// 4. gc output rowset if failed class Compaction { public: Compaction(TabletSharedPtr tablet, const std::string& label, const std::shared_ptr& parent_tracker); virtual ~Compaction(); - virtual OLAPStatus compact() = 0; + // This is only for http CompactionAction + OLAPStatus compact(); virtual OLAPStatus prepare_compact() = 0; - virtual OLAPStatus execute_compact() = 0; + OLAPStatus execute_compact(); + virtual OLAPStatus execute_compact_impl() = 0; protected: virtual OLAPStatus pick_rowsets_to_compact() = 0; @@ -62,7 +64,7 @@ protected: OLAPStatus do_compaction_impl(int64_t permits); void modify_rowsets(); - OLAPStatus gc_unused_rowsets(); + void gc_output_rowset(); OLAPStatus construct_output_rowset_writer(); OLAPStatus construct_input_rowset_readers(); diff --git a/be/src/olap/cumulative_compaction.cpp b/be/src/olap/cumulative_compaction.cpp index 8eed075536..7f084b91ca 100644 --- a/be/src/olap/cumulative_compaction.cpp +++ b/be/src/olap/cumulative_compaction.cpp @@ -30,13 +30,6 @@ CumulativeCompaction::CumulativeCompaction(TabletSharedPtr tablet, const std::st CumulativeCompaction::~CumulativeCompaction() {} -OLAPStatus CumulativeCompaction::compact() { - RETURN_NOT_OK(prepare_compact()); - RETURN_NOT_OK(execute_compact()); - - return OLAP_SUCCESS; -} - OLAPStatus CumulativeCompaction::prepare_compact() { if (!_tablet->init_succeeded()) { return OLAP_ERR_CUMULATIVE_INVALID_PARAMETERS; @@ -64,7 +57,7 @@ OLAPStatus CumulativeCompaction::prepare_compact() { return OLAP_SUCCESS; } -OLAPStatus CumulativeCompaction::execute_compact() { +OLAPStatus CumulativeCompaction::execute_compact_impl() { MutexLock lock(_tablet->get_cumulative_lock(), TRY_LOCK); if (!lock.own_lock()) { LOG(INFO) << "The tablet is under cumulative compaction. tablet=" << _tablet->full_name(); diff --git a/be/src/olap/cumulative_compaction.h b/be/src/olap/cumulative_compaction.h index f5ada614d6..6382acee6f 100644 --- a/be/src/olap/cumulative_compaction.h +++ b/be/src/olap/cumulative_compaction.h @@ -31,10 +31,8 @@ public: const std::shared_ptr& parent_tracker); ~CumulativeCompaction() override; - OLAPStatus compact() override; - OLAPStatus prepare_compact() override; - OLAPStatus execute_compact() override; + OLAPStatus execute_compact_impl() override; std::vector get_input_rowsets() { return _input_rowsets; } diff --git a/be/src/olap/olap_common.h b/be/src/olap/olap_common.h index 50690ea9d5..64f17fe3b1 100644 --- a/be/src/olap/olap_common.h +++ b/be/src/olap/olap_common.h @@ -248,6 +248,9 @@ struct OlapReaderStatistics { int64_t rows_bf_filtered = 0; // Including the number of rows filtered out according to the Delete information in the Tablet, // and the number of rows filtered for marked deleted rows under the unique key model. + // This metric is mainly used to record the number of rows filtered by the delete condition in Segment V1, + // and it is also used to record the replaced rows in the Unique key model in the "Reader" class. + // In segmentv2, if you want to get all filtered rows, you need the sum of "rows_del_filtered" and "rows_conditions_filtered". int64_t rows_del_filtered = 0; // the number of rows filtered by various column indexes. int64_t rows_conditions_filtered = 0; diff --git a/be/src/olap/reader.h b/be/src/olap/reader.h index 88edae1f04..d096b7478b 100644 --- a/be/src/olap/reader.h +++ b/be/src/olap/reader.h @@ -123,7 +123,9 @@ public: uint64_t merged_rows() const { return _merged_rows; } - uint64_t filtered_rows() const { return _stats.rows_del_filtered; } + uint64_t filtered_rows() const { + return _stats.rows_del_filtered + _stats.rows_conditions_filtered; + } const OlapReaderStatistics& stats() const { return _stats; } OlapReaderStatistics* mutable_stats() { return &_stats; } diff --git a/fe/fe-core/src/test/java/org/apache/doris/catalog/CreateFunctionTest.java b/fe/fe-core/src/test/java/org/apache/doris/catalog/CreateFunctionTest.java index d13c64dd45..00248b0486 100644 --- a/fe/fe-core/src/test/java/org/apache/doris/catalog/CreateFunctionTest.java +++ b/fe/fe-core/src/test/java/org/apache/doris/catalog/CreateFunctionTest.java @@ -79,7 +79,7 @@ public class CreateFunctionTest { "\"symbol\" = \"_ZN9doris_udf6AddUdfEPNS_15FunctionContextERKNS_9StringValE\",\n" + "\"prepare_fn\" = \"_ZN9doris_udf13AddUdfPrepareEPNS_15FunctionContextENS0_18FunctionStateScopeE\",\n" + "\"close_fn\" = \"_ZN9doris_udf11AddUdfCloseEPNS_15FunctionContextENS0_18FunctionStateScopeE\",\n" + - "\"object_file\" = \"http://nmg01-inf-dorishb00.nmg01.baidu.com:8456/libcmy_udf.so\"\n" + + "\"object_file\" = \"http://127.0.0.1:8008/libcmy_udf.so\"\n" + ");"; CreateFunctionStmt createFunctionStmt = (CreateFunctionStmt) UtFrameUtils.parseAndAnalyzeStmt(createFuncStr, ctx);