storage: Fix GC for logical split not working #5941

breezewish · 2022-09-17T12:07:32Z

What problem does this PR solve?

Issue Number: ref #5237

I discovered that some space is not reclaimed after ingestBySplit. After investigation the problem is we are not triggering GC for these segments due to some refactoring mistake introduced in #5863

What is changed and how it works?

Fixed such GC not working

Changed merge in metrics to be merge_bg_gc so that it is more clear
Made GC logs more verbose

Check List

Tests

Unit test
Integration test
Manual test (add detailed scripts or steps below)

Space can be successfully reclaimed after using the new TiFlash binary:

No code

Side effects

Performance regression: Consumes more CPU
Performance regression: Consumes more Memory
Breaking backward compatibility

Documentation

Release note

This is a bug fix, however the bug is not release to user so there is no release note.

None

Signed-off-by: Wish <[email protected]>

…plit_tests

Signed-off-by: Wish <[email protected]>

ti-chi-bot · 2022-09-17T12:07:33Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

JaySon-Huang
lidezhu

To complete the pull request process, please ask the reviewers in the list to review by filling /cc @reviewer in the comment.
After your PR has acquired the required number of LGTMs, you can assign this pull request to the committer in the list by filling /assign @committer in the comment to help you merge this pull request.

The full list of commands accepted by this bot can be found here.

Reviewer can indicate their review by submitting an approval review.
Reviewer can cancel approval by submitting a request changes review.

lidezhu

LGTM

JaySon-Huang

lgtm

JaySon-Huang · 2022-09-17T13:50:31Z

/run-integration-test

breezewish · 2022-09-17T14:16:57Z

I guess there are some problems in the master branch:

[2022-09-17T13:54:09.190Z]   Error: use test; set @@tidb_isolation_read_engines='tiflash'; select count(distinct pk), count(id), count(value), count(1) from a;
[2022-09-17T13:54:09.190Z]   Result:
[2022-09-17T13:54:09.190Z]     ERROR 1105 (HY000) at line 1: other error for mpp stream: From MPP<query:436056319140823047,task:2>: Code: 0, e.displayText() = DB::TiFlashException: Different aggregation mode detected, e.what() = DB::TiFlashException,
[2022-09-17T13:54:09.190Z]   Expected:
[2022-09-17T13:54:09.190Z]     +--------------------+-----------+--------------+----------+
[2022-09-17T13:54:09.190Z]     | count(distinct pk) | count(id) | count(value) | count(1) |
[2022-09-17T13:54:09.190Z]     +--------------------+-----------+--------------+----------+
[2022-09-17T13:54:09.190Z]     |                  0 |         0 |            0 |        0 |
[2022-09-17T13:54:09.190Z]     +--------------------+-----------+--------------+----------+

breezewish · 2022-09-17T14:55:20Z

/merge

ti-chi-bot · 2022-09-17T14:55:21Z

@breezewish: It seems you want to merge this PR, I will help you trigger all the tests:

/run-all-tests

You only need to trigger /merge once, and if the CI test fails, you just re-trigger the test that failed and the bot will merge the PR for you after the CI passes.

If you have any questions about the PR merge process, please refer to pr process.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the ti-community-infra/tichi repository.

ti-chi-bot · 2022-09-17T14:55:23Z

This pull request has been accepted and is ready to merge.

Commit hash: 6e860bf

sre-bot · 2022-09-17T15:08:03Z

Coverage for changed files

Filename                                                    Regions    Missed Regions     Cover   Functions  Missed Functions  Executed       Lines      Missed Lines     Cover    Branches   Missed Branches     Cover
-----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
Common/TiFlashMetrics.h                                          18                 0   100.00%          11                 0   100.00%          47                 1    97.87%           8                 0   100.00%
Storages/DeltaMerge/DeltaMergeStore.h                            17                 2    88.24%          16                 2    87.50%          50                 7    86.00%           0                 0         -
Storages/DeltaMerge/DeltaMergeStore_InternalBg.cpp              478               200    58.16%          16                 2    87.50%         616               182    70.45%         218               100    54.13%
Storages/DeltaMerge/DeltaMergeStore_InternalSegment.cpp         562               339    39.68%           5                 1    80.00%         419               122    70.88%         148                80    45.95%
-----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
TOTAL                                                          1075               541    49.67%          48                 5    89.58%        1132               312    72.44%         374               180    51.87%

Coverage summary

Functions  MissedFunctions  Executed  Lines   MissedLines  Cover
18866      8120             56.96%    219592  83615        61.92%

full coverage report (for internal network access only)

sre-bot · 2022-09-19T04:20:41Z

Coverage for changed files

Filename                                                    Regions    Missed Regions     Cover   Functions  Missed Functions  Executed       Lines      Missed Lines     Cover    Branches   Missed Branches     Cover
-----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
Common/TiFlashMetrics.h                                          18                 0   100.00%          11                 0   100.00%          47                 1    97.87%           8                 0   100.00%
Storages/DeltaMerge/DeltaMergeStore.h                            17                 2    88.24%          16                 2    87.50%          50                 7    86.00%           0                 0         -
Storages/DeltaMerge/DeltaMergeStore_InternalBg.cpp              478               200    58.16%          16                 2    87.50%         616               182    70.45%         218               100    54.13%
Storages/DeltaMerge/DeltaMergeStore_InternalSegment.cpp         562               339    39.68%           5                 1    80.00%         419               122    70.88%         148                80    45.95%
-----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
TOTAL                                                          1075               541    49.67%          48                 5    89.58%        1132               312    72.44%         374               180    51.87%

Coverage summary

Functions  MissedFunctions  Executed  Lines   MissedLines  Cover
18863      8117             56.97%    219543  83621        61.91%

full coverage report (for internal network access only)

breezewish · 2022-09-19T04:53:48Z

/merge

ti-chi-bot · 2022-09-19T04:53:49Z

@breezewish: It seems you want to merge this PR, I will help you trigger all the tests:

/run-all-tests

You only need to trigger /merge once, and if the CI test fails, you just re-trigger the test that failed and the bot will merge the PR for you after the CI passes.

If you have any questions about the PR merge process, please refer to pr process.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the ti-community-infra/tichi repository.

JaySon-Huang · 2022-09-19T04:58:18Z

/rebuild

JaySon-Huang · 2022-09-19T04:58:30Z

/run-integration-test

sre-bot · 2022-09-19T05:11:33Z

Coverage for changed files

Filename                                                    Regions    Missed Regions     Cover   Functions  Missed Functions  Executed       Lines      Missed Lines     Cover    Branches   Missed Branches     Cover
-----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
Common/TiFlashMetrics.h                                          18                 0   100.00%          11                 0   100.00%          47                 1    97.87%           8                 0   100.00%
Storages/DeltaMerge/DeltaMergeStore.h                            17                 2    88.24%          16                 2    87.50%          50                 7    86.00%           0                 0         -
Storages/DeltaMerge/DeltaMergeStore_InternalBg.cpp              478               200    58.16%          16                 2    87.50%         616               182    70.45%         218               100    54.13%
Storages/DeltaMerge/DeltaMergeStore_InternalSegment.cpp         562               339    39.68%           5                 1    80.00%         419               122    70.88%         148                80    45.95%
-----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
TOTAL                                                          1075               541    49.67%          48                 5    89.58%        1132               312    72.44%         374               180    51.87%

Coverage summary

Functions  MissedFunctions  Executed  Lines   MissedLines  Cover
18863      8115             56.98%    219543  83628        61.91%

full coverage report (for internal network access only)

ti-chi-bot · 2022-09-19T05:35:09Z

@breezewish: Your PR was out of date, I have automatically updated it for you.

At the same time I will also trigger all tests for you:

/run-all-tests

If the CI test fails, you just re-trigger the test that failed and the bot will merge the PR for you after the CI passes.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the ti-community-infra/tichi repository.

sre-bot · 2022-09-19T06:01:48Z

Coverage for changed files

Filename                                                    Regions    Missed Regions     Cover   Functions  Missed Functions  Executed       Lines      Missed Lines     Cover    Branches   Missed Branches     Cover
-----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
Common/TiFlashMetrics.h                                          18                 0   100.00%          11                 0   100.00%          47                 1    97.87%           8                 0   100.00%
Storages/DeltaMerge/DeltaMergeStore.h                            17                 2    88.24%          16                 2    87.50%          50                 7    86.00%           0                 0         -
Storages/DeltaMerge/DeltaMergeStore_InternalBg.cpp              478               200    58.16%          16                 2    87.50%         616               182    70.45%         218               100    54.13%
Storages/DeltaMerge/DeltaMergeStore_InternalSegment.cpp         562               339    39.68%           5                 1    80.00%         419               122    70.88%         148                80    45.95%
-----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
TOTAL                                                          1075               541    49.67%          48                 5    89.58%        1132               312    72.44%         374               180    51.87%

Coverage summary

Functions  MissedFunctions  Executed  Lines   MissedLines  Cover
18863      8116             56.97%    219544  83600        61.92%

full coverage report (for internal network access only)

breezewish added 4 commits September 17, 2022 14:31

Improve metrics and log for GC merge

37b3295

Signed-off-by: Wish <[email protected]>

Merge remote-tracking branch 'origin/master' into wenxuan/ingest_by_s…

daeb4e5

…plit_tests

Add verbose trace logs for merge delta GC

ea55058

Signed-off-by: Wish <[email protected]>

fix dtfile gc not working

6e860bf

Signed-off-by: Wish <[email protected]>

ti-chi-bot added release-note-none Denotes a PR that doesn't merit a release note. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Sep 17, 2022

breezewish requested review from lidezhu and JaySon-Huang September 17, 2022 12:11

lidezhu approved these changes Sep 17, 2022

View reviewed changes

ti-chi-bot added the status/LGT1 Indicates that a PR has LGTM 1. label Sep 17, 2022

JaySon-Huang approved these changes Sep 17, 2022

View reviewed changes

ti-chi-bot added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Sep 17, 2022

ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Sep 17, 2022

Merge branch 'master' into wenxuan/fix_dt_file_gc

90b4ab2

Merge branch 'master' into wenxuan/fix_dt_file_gc

f57bd34

ti-chi-bot merged commit fba461d into master Sep 19, 2022

JaySon-Huang deleted the wenxuan/fix_dt_file_gc branch September 19, 2022 07:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

storage: Fix GC for logical split not working #5941

storage: Fix GC for logical split not working #5941

breezewish commented Sep 17, 2022 •

edited

Loading

ti-chi-bot commented Sep 17, 2022 •

edited

Loading

lidezhu left a comment

JaySon-Huang left a comment

JaySon-Huang commented Sep 17, 2022

breezewish commented Sep 17, 2022

breezewish commented Sep 17, 2022

ti-chi-bot commented Sep 17, 2022

ti-chi-bot commented Sep 17, 2022

sre-bot commented Sep 17, 2022

sre-bot commented Sep 19, 2022

breezewish commented Sep 19, 2022

ti-chi-bot commented Sep 19, 2022

JaySon-Huang commented Sep 19, 2022

JaySon-Huang commented Sep 19, 2022

sre-bot commented Sep 19, 2022

ti-chi-bot commented Sep 19, 2022

sre-bot commented Sep 19, 2022

storage: Fix GC for logical split not working #5941

storage: Fix GC for logical split not working #5941

Conversation

breezewish commented Sep 17, 2022 • edited Loading

What problem does this PR solve?

What is changed and how it works?

Check List

Release note

ti-chi-bot commented Sep 17, 2022 • edited Loading

lidezhu left a comment

Choose a reason for hiding this comment

JaySon-Huang left a comment

Choose a reason for hiding this comment

JaySon-Huang commented Sep 17, 2022

breezewish commented Sep 17, 2022

breezewish commented Sep 17, 2022

ti-chi-bot commented Sep 17, 2022

ti-chi-bot commented Sep 17, 2022

sre-bot commented Sep 17, 2022

Coverage for changed files

Coverage summary

sre-bot commented Sep 19, 2022

Coverage for changed files

Coverage summary

breezewish commented Sep 19, 2022

ti-chi-bot commented Sep 19, 2022

JaySon-Huang commented Sep 19, 2022

JaySon-Huang commented Sep 19, 2022

sre-bot commented Sep 19, 2022

Coverage for changed files

Coverage summary

ti-chi-bot commented Sep 19, 2022

sre-bot commented Sep 19, 2022

Coverage for changed files

Coverage summary

breezewish commented Sep 17, 2022 •

edited

Loading

ti-chi-bot commented Sep 17, 2022 •

edited

Loading