*: enlarge the default region-size. by LykxSassinator · Pull Request #17311 · tikv/tikv

LykxSassinator · 2024-07-26T06:42:08Z

What is changed and how it works?

Issue Number: ref #17309

What's Changed:

This pr make the default setting of `coprocessor.region-split-size` larger,
from `96MB` to `256MB` by default.  The new setting for `region-split-size` is
compatible to the larger cluster.

Related changes

PR to update pingcap/docs/pingcap/docs-cn:
Need to cherry-pick to the release branch

Check List

Tests

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

Side effects

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

The following charts show the positive feedbacks tested on typical workloads by larger region-size (256MB), compared with the current value (96MB):

For TPCC 20k warehouses workloads (PS: As for QPS, higher is better; As for latency diff, smaller is better)
For OLTP workload (PS: As for QPS, higher is better. And if the percent is < 0, it means "larger region-size" might give regressions. Since the fluctuations of throughput is < +-1%, it can be regarded as bias.)

Release note

Change the default region-size from 96MB to 256MB.

This pr make the default setting of `coprocessor.region-split-size` larger, from `96MB` to `256MB` by default. The new setting for `region-split-size` is compatible to the larger cluster. Signed-off-by: lucasliang <nkcs_lykx@hotmail.com>

zhangjinpeng87

LGTM

zhangjinpeng87 · 2024-07-29T20:59:22Z

@LykxSassinator Please paste some performance comparison results here.

components/raftstore/src/coprocessor/config.rs

Co-authored-by: Bisheng Huang <hbisheng@gmail.com> Signed-off-by: lucasliang <nkcs_lykx@hotmail.com>

Signed-off-by: lucasliang <nkcs_lykx@hotmail.com>

v01dstar

lgtm

ti-chi-bot · 2024-08-02T16:58:46Z

[LGTM Timeline notifier]

Timeline:

2024-07-26 21:31:57.692377188 +0000 UTC m=+41401.819437343: ☑️ agreed by zhangjinpeng87.
2024-08-02 16:58:45.520379523 +0000 UTC m=+25655.387478608: ☑️ agreed by v01dstar.

zhangjinpeng87 · 2024-08-02T17:06:34Z

/approve

ti-chi-bot · 2024-08-02T17:06:38Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: v01dstar, zhangjinpeng87

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~components/raftstore/src/coprocessor/OWNERS~~ [zhangjinpeng87]
~~etc/OWNERS~~ [zhangjinpeng87]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

overvenus · 2024-08-03T08:10:27Z

etc/config-template.toml

 ## When Backup region [a,e) size exceeds `sst-max-size`, it will be backuped into several Files [a,b),
 ## [b,c), [c,d), [d,e) and the size of [a,b), [b,c), [c,d) will be `sst-max-size` (or a
 ## little larger).
-# sst-max-size = "144MB"


Maybe it should be changed to the default sst file size (64MB)?

The default value of sst_max_size is equal to region_max_size by design where it's expected to make the size of region compatible to the split size when backup.

tikv/src/config/mod.rs

Line 2898 in 7c50908

sst_max_size: default_coprocessor.region_max_size(),

/cc @YuJuncen

In most cases, the SST size should be equal to the region size. That is we want to align the SST's boundary with the regions' because we use the SST's boundary to split regions during restoring.

This sst_max_size is used for handing abnormal sized regions -- we generate SSTs in memory, when there are huge SSTs, OOM may happen.

I guess both increase it to 384MiB or keep it unchanged are acceptable.

/cc @3pointer

The region size is uncompressed data size, but the sst size is compressed data size.

please remember ping lightning developers when region size is changed. Some E2E failed recently.

pingcap/tidb#55374 (comment)

IMO, it's better to mark this information as an relevant info to TiKV, as the developers of TiKV may not have received this information.

LykxSassinator · 2024-08-05T02:39:01Z

/test

ti-chi-bot · 2024-08-05T02:39:04Z

@LykxSassinator: The /test command needs one or more targets.
The following commands are available to trigger required jobs:

/test pull-unit-test

Use /test all to run all jobs.

Details

In response to this:

/test

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 kubernetes/test-infra repository.

LykxSassinator · 2024-08-05T02:39:24Z

/test pull-unit-test

wuhuizuo · 2024-08-05T02:59:59Z

/retest

ti-chi-bot · 2024-08-05T06:44:21Z

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

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.

Details

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.

ref tikv#17309 This pr make the default setting of `coprocessor.region-split-size` larger, from `96MB` to `256MB` by default. The new setting for `region-split-size` is compatible to the larger cluster. Signed-off-by: lucasliang <nkcs_lykx@hotmail.com> Co-authored-by: ti-chi-bot[bot] <108142056+ti-chi-bot[bot]@users.noreply.github.com> Co-authored-by: Bisheng Huang <hbisheng@gmail.com> Signed-off-by: 3AceShowHand <jinl1037@hotmail.com>

This reverts commit 74a760f.

This reverts commit 74a760f. Signed-off-by: lucasliang <nkcs_lykx@hotmail.com>

ref pingcap/tidb#55374 Revert the changes on the configuration of region-size. These changes will be delayed until v8.4 Signed-off-by: lucasliang <nkcs_lykx@hotmail.com> Co-authored-by: ti-chi-bot[bot] <108142056+ti-chi-bot[bot]@users.noreply.github.com>

This reverts commit 74a760f. Signed-off-by: lucasliang <nkcs_lykx@hotmail.com>

ref tikv#17309 This pr make the default setting of `coprocessor.region-split-size` larger, from `96MB` to `256MB` by default. The new setting for `region-split-size` is compatible to the larger cluster. Signed-off-by: lucasliang <nkcs_lykx@hotmail.com> Co-authored-by: ti-chi-bot[bot] <108142056+ti-chi-bot[bot]@users.noreply.github.com> Co-authored-by: Bisheng Huang <hbisheng@gmail.com> (cherry picked from commit 74a760f)

ti-chi-bot bot added release-note Denotes a PR that will be considered when it comes time to generate release notes. dco-signoff: yes Indicates the PR's author has signed the dco. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jul 26, 2024

LykxSassinator changed the title *: make the default region-size larger. *: enlarge the default region-size. Jul 26, 2024

zhangjinpeng87 approved these changes Jul 26, 2024

View reviewed changes

ti-chi-bot bot added needs-1-more-lgtm Indicates a PR needs 1 more LGTM. approved labels Jul 26, 2024

hbisheng reviewed Jul 30, 2024

View reviewed changes

components/raftstore/src/coprocessor/config.rs Outdated Show resolved Hide resolved

LykxSassinator and others added 2 commits July 30, 2024 13:54

Update components/raftstore/src/coprocessor/config.rs

695008c

Co-authored-by: Bisheng Huang <hbisheng@gmail.com> Signed-off-by: lucasliang <nkcs_lykx@hotmail.com>

Calibrate the configs.

4f4b272

Signed-off-by: lucasliang <nkcs_lykx@hotmail.com>

v01dstar approved these changes Aug 2, 2024

View reviewed changes

ti-chi-bot bot added lgtm and removed needs-1-more-lgtm Indicates a PR needs 1 more LGTM. labels Aug 2, 2024

Merge branch 'master' into large_region_size

30b8caa

overvenus reviewed Aug 3, 2024

View reviewed changes

ti-chi-bot bot requested a review from YuJuncen August 5, 2024 02:55

ti-chi-bot bot requested a review from 3pointer August 5, 2024 04:14

Merge branch 'master' into large_region_size

7c8d77b

ti-chi-bot bot merged commit 74a760f into tikv:master Aug 5, 2024

ti-chi-bot bot added this to the Pool milestone Aug 5, 2024

lance6716 mentioned this pull request Aug 13, 2024

Global sort IMPORT INTO stuck after region size is larger than 96MB pingcap/tidb#55374

Closed

Yui-Song mentioned this pull request Aug 14, 2024

Enlarging the TiKV default region-size causes IndexJoin and RangeScan performance regression #17382

Open

LykxSassinator added a commit to LykxSassinator/tikv that referenced this pull request Aug 16, 2024

Revert "*: enlarge the default region-size. (tikv#17311)"

dab644b

This reverts commit 74a760f.

LykxSassinator added a commit to LykxSassinator/tikv that referenced this pull request Aug 16, 2024

Revert "*: enlarge the default region-size. (tikv#17311)"

08fb491

This reverts commit 74a760f. Signed-off-by: lucasliang <nkcs_lykx@hotmail.com>

LykxSassinator added a commit to LykxSassinator/tikv that referenced this pull request Oct 15, 2024

Revert "*: enlarge the default region-size. (tikv#17311)"

9794fb2

This reverts commit 74a760f. Signed-off-by: lucasliang <nkcs_lykx@hotmail.com>

owlsome2501 mentioned this pull request Dec 12, 2024

There is a 84.4% performance regression in tpch after PR#17311 #17988

Closed

kennedy8312 mentioned this pull request Mar 10, 2025

global-sort: reduce number of SST ingested into TiKV pingcap/tidb#59962

Closed

This was referenced Jun 4, 2025

region-size: Inherit last configured settings instead of default values #18503

Closed

[DNM] enlarge default region size from 96MB to 256MB #16765

Closed

Conversation

LykxSassinator commented Jul 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What is changed and how it works?

Related changes

Check List

Release note

Uh oh!

zhangjinpeng87 left a comment

Choose a reason for hiding this comment

Uh oh!

zhangjinpeng87 commented Jul 29, 2024

Uh oh!

Uh oh!

v01dstar left a comment

Choose a reason for hiding this comment

Uh oh!

ti-chi-bot bot commented Aug 2, 2024

[LGTM Timeline notifier]

Uh oh!

zhangjinpeng87 commented Aug 2, 2024

Uh oh!

ti-chi-bot bot commented Aug 2, 2024

Uh oh!

overvenus Aug 3, 2024

Choose a reason for hiding this comment

Uh oh!

LykxSassinator Aug 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LykxSassinator Aug 5, 2024

Choose a reason for hiding this comment

Uh oh!

YuJuncen Aug 5, 2024

Choose a reason for hiding this comment

Uh oh!

YuJuncen Aug 5, 2024

Choose a reason for hiding this comment

Uh oh!

zhangjinpeng87 Aug 9, 2024

Choose a reason for hiding this comment

Uh oh!

lance6716 Aug 13, 2024

Choose a reason for hiding this comment

Uh oh!

LykxSassinator Aug 13, 2024

Choose a reason for hiding this comment

Uh oh!

LykxSassinator commented Aug 5, 2024

Uh oh!

ti-chi-bot bot commented Aug 5, 2024

Uh oh!

LykxSassinator commented Aug 5, 2024

Uh oh!

wuhuizuo commented Aug 5, 2024

Uh oh!

ti-chi-bot bot commented Aug 5, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

LykxSassinator commented Jul 26, 2024 •

edited

Loading

LykxSassinator Aug 4, 2024 •

edited

Loading