address comment

pingcap · Sep 13, 2022 · b0a4070 · b0a4070
1 parent 9490589
commit b0a4070
Show file tree

Hide file tree

Showing 3 changed files with 14 additions and 10 deletions.
diff --git a/dbms/src/Storages/DeltaMerge/DeltaMergeStore_InternalBg.cpp b/dbms/src/Storages/DeltaMerge/DeltaMergeStore_InternalBg.cpp
@@ -253,7 +253,7 @@ enum Type
 {
     Unknown,
     TooManyDeleteRange,
-    TooLargeDTFileRange,
+    TooMuchOutOfRange,
     TooManyInvalidVersion,
 };
 
@@ -263,8 +263,8 @@ static std::string toString(Type type)
     {
     case TooManyDeleteRange:
         return "TooManyDeleteRange";
-    case TooLargeDTFileRange:
-        return "TooLargeDTFileRange";
+    case TooMuchOutOfRange:
+        return "TooMuchOutOfRange";
     case TooManyInvalidVersion:
         return "TooManyInvalidVersion";
     default:
@@ -317,7 +317,7 @@ bool shouldCompactDeltaWithStable(const DMContext & context, const SegmentSnapsh
     return (delete_rows >= stable_rows * invalid_data_ratio_threshold) || (delete_bytes >= stable_bytes * invalid_data_ratio_threshold);
 }
 
-bool shouldCompactStableWithTooLargeDTFileRange(const SegmentSnapshotPtr & snap, double invalid_data_ratio_threshold, const LoggerPtr & log)
+bool shouldCompactStableWithTooMuchDataOutSegmentRange(const SegmentSnapshotPtr & snap, double invalid_data_ratio_threshold, const LoggerPtr & log)
 {
     auto valid_rows = snap->stable->getRows();
     auto valid_bytes = snap->stable->getBytes();
@@ -428,10 +428,10 @@ UInt64 DeltaMergeStore::onSyncGc(Int64 limit)
             else if (!segment->isValidDataRatioChecked())
             {
                 segment->setValidDataRatioChecked();
-                if (GC::shouldCompactStableWithTooLargeDTFileRange(segment_snap, invalid_data_ratio_threshold, log))
+                if (GC::shouldCompactStableWithTooMuchDataOutSegmentRange(segment_snap, invalid_data_ratio_threshold, log))
                 {
                     should_compact = true;
-                    gc_type = GC::Type::TooLargeDTFileRange;
+                    gc_type = GC::Type::TooMuchOutOfRange;
                 }
             }
             else if (!should_compact && (segment->getLastCheckGCSafePoint() < gc_safe_point))

diff --git a/dbms/src/Storages/DeltaMerge/Segment.h b/dbms/src/Storages/DeltaMerge/Segment.h
@@ -427,6 +427,10 @@ class Segment : private boost::noncopyable
     const StableValueSpacePtr stable;
 
     bool split_forbidden = false;
+    // After logical split, it is very possible that only half of the data in the segment's DTFile is valid for this segment.
+    // So we want to do merge delta on this kind of segment to clean out the invalid data.
+    // This involves to check the valid data ratio in the background gc thread,
+    // and to avoid doing this check repeatedly, we add this flag to indicate whether the valid data ratio has already been checked.
     std::atomic<bool> check_valid_data_ratio = false;
 
     LoggerPtr log;

diff --git a/dbms/src/Storages/DeltaMerge/tests/gtest_segment.cpp b/dbms/src/Storages/DeltaMerge/tests/gtest_segment.cpp
@@ -42,7 +42,7 @@ namespace DM
 {
 namespace GC
 {
-bool shouldCompactStableWithTooLargeDTFileRange(const SegmentSnapshotPtr & snap, double invalid_data_ratio_threshold, const LoggerPtr & log);
+bool shouldCompactStableWithTooMuchDataOutSegmentRange(const SegmentSnapshotPtr & snap, double invalid_data_ratio_threshold, const LoggerPtr & log);
 }
 namespace tests
 {
@@ -457,7 +457,7 @@ try
     {
         auto segment = segments[DELTA_MERGE_FIRST_SEGMENT_ID];
         auto snap = segment->createSnapshot(*dm_context, /* for_update */ true, CurrentMetrics::DT_SnapshotOfDeltaMerge);
-        ASSERT_FALSE(GC::shouldCompactStableWithTooLargeDTFileRange(snap, invalid_data_ratio_threshold, log));
+        ASSERT_FALSE(GC::shouldCompactStableWithTooMuchDataOutSegmentRange(snap, invalid_data_ratio_threshold, log));
     }
 
     FailPointHelper::enableFailPoint(FailPoints::force_segment_logical_split);
@@ -467,12 +467,12 @@ try
     {
         auto segment = segments[DELTA_MERGE_FIRST_SEGMENT_ID];
         auto snap = segment->createSnapshot(*dm_context, /* for_update */ true, CurrentMetrics::DT_SnapshotOfDeltaMerge);
-        ASSERT_TRUE(GC::shouldCompactStableWithTooLargeDTFileRange(snap, invalid_data_ratio_threshold, log));
+        ASSERT_TRUE(GC::shouldCompactStableWithTooMuchDataOutSegmentRange(snap, invalid_data_ratio_threshold, log));
     }
     {
         auto segment = segments[new_seg_id_opt.value()];
         auto snap = segment->createSnapshot(*dm_context, /* for_update */ true, CurrentMetrics::DT_SnapshotOfDeltaMerge);
-        ASSERT_TRUE(GC::shouldCompactStableWithTooLargeDTFileRange(snap, invalid_data_ratio_threshold, log));
+        ASSERT_TRUE(GC::shouldCompactStableWithTooMuchDataOutSegmentRange(snap, invalid_data_ratio_threshold, log));
     }
 }
 CATCH