add more checks for background segment split #4406

lidezhu · 2022-03-24T03:56:09Z

What problem does this PR solve?

Issue Number: close #4408

Problem Summary: After syncing data from tikv using snapshot, there are some too large segments.

What is changed and how it works?

Add more check for background split.

Check List

Tests

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

Load tpch100 into tiflash(set tiflash replica after data loaded into tikv), and check the segments stat using query:
select rows from information_schema.tiflash_segments where table="t_144" order by rows;
Before fix:

After fix:

Side effects

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

Documentation

Release note

None

ti-chi-bot · 2022-03-24T03:56:10Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

JaySon-Huang
flowbehappy

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.

JaySon-Huang · 2022-03-24T05:00:10Z

dbms/src/Storages/DeltaMerge/DeltaMergeStore.cpp

-            || delta_bytes - delta_last_try_split_bytes >= delta_cache_limit_bytes);
+    // Do background split in the following two cases:
+    //   1. The segment is large enough, and there are some data in the delta layer. (A hot segment which is large enough)
+    //   2. The segment is too large. (A segment which is too large, although it is cold)


how do we trigger checkSegmentUpdate on a segment that is cold?

Here is some intuition for the check condition in the comments. The actual problem is here: https://docs.google.com/document/d/1tkJXERVmcKv6nwfhTXqk0qTdVm2CPh8OXZiZ8ap6sOA/edit?usp=sharing.

"Cold" means no continuous small write in this segment.

JaySon-Huang

LGTM

lidezhu · 2022-03-24T06:33:19Z

/merge

ti-chi-bot · 2022-03-24T06:33:19Z

@lidezhu: 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.

sre-bot · 2022-03-24T06:51:19Z

Coverage for changed files

Filename                                                                                                        Regions    Missed Regions     Cover   Functions  Missed Functions  Executed       Lines      Missed Lines     Cover    Branches   Missed Branches     Cover
---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
/home/jenkins/agent/workspace/tiflash-build-common/tiflash/dbms/src/Storages/DeltaMerge/DeltaMergeStore.cpp        1332               475    64.34%          64                 5    92.19%        1905               432    77.32%         780               373    52.18%
---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
TOTAL                                                                                                              1332               475    64.34%          64                 5    92.19%        1905               432    77.32%         780               373    52.18%

Coverage summary

Functions  MissedFunctions  Executed  Lines   MissedLines  Cover
17044      9571             43.85%    191827  97021        49.42%

full coverage report (for internal network access only)

lidezhu · 2022-03-24T07:09:23Z

/run-integration-tests

lidezhu · 2022-03-24T07:18:32Z

/merge

ti-chi-bot · 2022-03-24T07:18:32Z

@lidezhu: 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-03-24T07:18:37Z

@lidezhu: /merge in this pull request requires 2 approval(s).

In response to this:

/merge

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-03-24T07:29:18Z

Coverage for changed files

Filename                                                                                                        Regions    Missed Regions     Cover   Functions  Missed Functions  Executed       Lines      Missed Lines     Cover    Branches   Missed Branches     Cover
---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
/home/jenkins/agent/workspace/tiflash-build-common/tiflash/dbms/src/Storages/DeltaMerge/DeltaMergeStore.cpp        1332               466    65.02%          64                 5    92.19%        1905               428    77.53%         780               370    52.56%
---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
TOTAL                                                                                                              1332               466    65.02%          64                 5    92.19%        1905               428    77.53%         780               370    52.56%

Coverage summary

Functions  MissedFunctions  Executed  Lines   MissedLines  Cover
17044      9571             43.85%    191827  97028        49.42%

full coverage report (for internal network access only)

JaySon-Huang

LGTM

flowbehappy · 2022-03-24T07:44:07Z

/merge

ti-chi-bot · 2022-03-24T07:44:07Z

@flowbehappy: 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-03-24T07:44:13Z

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

Commit hash: aa00479

sre-bot · 2022-03-24T08:00:33Z

Coverage for changed files

Filename                                                                                                        Regions    Missed Regions     Cover   Functions  Missed Functions  Executed       Lines      Missed Lines     Cover    Branches   Missed Branches     Cover
---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
/home/jenkins/agent/workspace/tiflash-build-common/tiflash/dbms/src/Storages/DeltaMerge/DeltaMergeStore.cpp        1332               466    65.02%          64                 5    92.19%        1905               428    77.53%         780               370    52.56%
---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
TOTAL                                                                                                              1332               466    65.02%          64                 5    92.19%        1905               428    77.53%         780               370    52.56%

Coverage summary

Functions  MissedFunctions  Executed  Lines   MissedLines  Cover
17044      9571             43.85%    191827  97044        49.41%

full coverage report (for internal network access only)

ti-chi-bot · 2022-03-24T08:13:26Z

In response to a cherrypick label: new pull request created: #4416.

ti-chi-bot · 2022-03-24T08:14:03Z

In response to a cherrypick label: new pull request created: #4417.

close #4408

add more checks for background split

d48cebd

ti-chi-bot added do-not-merge/needs-linked-issue release-note-none Denotes a PR that doesn't merit a release note. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Mar 24, 2022

JaySon-Huang reviewed Mar 24, 2022

View reviewed changes

ti-chi-bot added do-not-merge/needs-triage-completed needs-cherry-pick-release-5.4 Should cherry pick this PR to release-5.4 branch. needs-cherry-pick-release-6.0 Type: Need cherry pick to release-6.0 and removed do-not-merge/needs-linked-issue do-not-merge/needs-triage-completed labels Mar 24, 2022

lidezhu requested review from flowbehappy and JinheLin March 24, 2022 05:49

Merge branch 'master' into add-more-segment-split-check

b4bfff7

flowbehappy approved these changes Mar 24, 2022

View reviewed changes

JaySon-Huang approved these changes Mar 24, 2022

View reviewed changes

lidezhu added the status/can-merge Indicates a PR has been approved by a committer. label Mar 24, 2022

ti-chi-bot removed the status/can-merge Indicates a PR has been approved by a committer. label Mar 24, 2022

lidezhu added the status/can-merge Indicates a PR has been approved by a committer. label Mar 24, 2022

ti-chi-bot removed the status/can-merge Indicates a PR has been approved by a committer. label Mar 24, 2022

lidezhu added the status/LGT2 Indicates that a PR has LGTM 2. label Mar 24, 2022

ti-chi-bot removed the status/LGT2 Indicates that a PR has LGTM 2. label Mar 24, 2022

JaySon-Huang approved these changes Mar 24, 2022

View reviewed changes

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

Merge branch 'master' into add-more-segment-split-check

aa00479

flowbehappy self-requested a review March 24, 2022 07:43

flowbehappy approved these changes Mar 24, 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 Mar 24, 2022

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

ti-chi-bot merged commit 2e4d859 into pingcap:master Mar 24, 2022

ti-chi-bot mentioned this pull request Mar 24, 2022

add more checks for background segment split (#4406) #4416

Merged

12 tasks

ti-chi-bot mentioned this pull request Mar 24, 2022

add more checks for background segment split (#4406) #4417

Merged

12 tasks

lidezhu deleted the add-more-segment-split-check branch March 24, 2022 08:21

ti-chi-bot added a commit that referenced this pull request Mar 24, 2022

add more checks for background segment split (#4406) (#4417)

2174c58

close #4408

ti-chi-bot added a commit that referenced this pull request Apr 26, 2022

add more checks for background segment split (#4406) (#4416)

abfb038

close #4408

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add more checks for background segment split #4406

add more checks for background segment split #4406

lidezhu commented Mar 24, 2022 •

edited

Loading

ti-chi-bot commented Mar 24, 2022 •

edited

Loading

JaySon-Huang Mar 24, 2022

lidezhu Mar 24, 2022

lidezhu Mar 24, 2022

JaySon-Huang left a comment

lidezhu commented Mar 24, 2022

ti-chi-bot commented Mar 24, 2022

sre-bot commented Mar 24, 2022

lidezhu commented Mar 24, 2022

lidezhu commented Mar 24, 2022

ti-chi-bot commented Mar 24, 2022

ti-chi-bot commented Mar 24, 2022

sre-bot commented Mar 24, 2022

JaySon-Huang left a comment

flowbehappy commented Mar 24, 2022

ti-chi-bot commented Mar 24, 2022

ti-chi-bot commented Mar 24, 2022

sre-bot commented Mar 24, 2022

ti-chi-bot commented Mar 24, 2022

ti-chi-bot commented Mar 24, 2022

add more checks for background segment split #4406

add more checks for background segment split #4406

Conversation

lidezhu commented Mar 24, 2022 • edited Loading

What problem does this PR solve?

What is changed and how it works?

Check List

Release note

ti-chi-bot commented Mar 24, 2022 • edited Loading

JaySon-Huang Mar 24, 2022

Choose a reason for hiding this comment

lidezhu Mar 24, 2022

Choose a reason for hiding this comment

lidezhu Mar 24, 2022

Choose a reason for hiding this comment

JaySon-Huang left a comment

Choose a reason for hiding this comment

lidezhu commented Mar 24, 2022

ti-chi-bot commented Mar 24, 2022

sre-bot commented Mar 24, 2022

Coverage for changed files

Coverage summary

lidezhu commented Mar 24, 2022

lidezhu commented Mar 24, 2022

ti-chi-bot commented Mar 24, 2022

ti-chi-bot commented Mar 24, 2022

sre-bot commented Mar 24, 2022

Coverage for changed files

Coverage summary

JaySon-Huang left a comment

Choose a reason for hiding this comment

flowbehappy commented Mar 24, 2022

ti-chi-bot commented Mar 24, 2022

ti-chi-bot commented Mar 24, 2022

sre-bot commented Mar 24, 2022

Coverage for changed files

Coverage summary

ti-chi-bot commented Mar 24, 2022

ti-chi-bot commented Mar 24, 2022

lidezhu commented Mar 24, 2022 •

edited

Loading

ti-chi-bot commented Mar 24, 2022 •

edited

Loading