Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

*: enlarge the default value of max-merge-region-size. #8445

Merged
merged 11 commits into from
Aug 2, 2024
Merged

*: enlarge the default value of max-merge-region-size. #8445

merged 11 commits into from
Aug 2, 2024

Conversation

LykxSassinator
Copy link
Contributor

@LykxSassinator LykxSassinator commented Jul 26, 2024 •
edited
Loading

This pr is used to enlarge the region size from default value 96MB to 256MB, compatible to the requirement of the stability of large cluster.

What problem does this PR solve?

Issue Number: close #8484, ref tikv/tikv#17309

What is changed and how does it work?

This pr is used to enlarge the region size from default value `96MB` to `256MB`, compatible to the requirement of the stability of large cluster.

Check List

Tests

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

Code changes

Side effects

  • Possible performance regression
  • Increased code complexity
  • Breaking backward compatibility

Related changes

Release note

Enlarge the region size from `96MB` to `256MB` as default.

@LykxSassinator
*: enlarge the default value of region size.
e5fe823 
This pr is used to enlarge the region size from default value `96MB`
to `256MB`, compatible to the requirement of the stability of large
cluster.

Signed-off-by: lucasliang <nkcs_lykx@hotmail.com>
@ti-chi-bot ti-chi-bot bot added do-not-merge/needs-linked-issue 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/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Jul 26, 2024
nolouch
nolouch reviewed Jul 26, 2024
View reviewed changes
pkg/schedule/config/config.go Outdated
@@ -30,7 +30,7 @@ const (
DefaultMaxReplicas = 3
defaultMaxSnapshotCount = 64
defaultMaxPendingPeerCount = 64
defaultMaxMergeRegionSize = 20
defaultMaxMergeRegionSize = 96
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to add more comments about which version changed and the related issue.

Copy link
Member

@rleungx rleungx Jul 26, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will it cause lots of merge operations when upgrading?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1, 96 is too aggressive, it might cause a lot of merging after upgrading. We increase the default region size from 96MB to 256MB in TiKV side, it is 2.67x larger than before. But here 20 -> 96 is nearly 5x larger than before, probably 40MB is better.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about using 54MB(=> 20MB * 256 / 96) as the recommended value ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will it cause lots of merge operations when upgrading?

Nope, the old value of max-merge-region-size is already persisted in the cluster and it will be reloaded after upgrading. Only the newly created cluster will use the new default value.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, right. But for those existing clusters, do we still use the old max-merge-region-size and let the user change the config? If so, then LGTM.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO, it should be.

Since each user's cluster may have a unique self-defined max-merge-region-size, manual adjustments should be checked and executed if the user requires it.

@LykxSassinator LykxSassinator changed the title *: enlarge the default value of region size. *: enlarge the default value of max-merge-region-size. Jul 29, 2024
@ti-chi-bot ti-chi-bot bot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Jul 29, 2024
@ti-chi-bot ti-chi-bot bot added the needs-1-more-lgtm Indicates a PR needs 1 more LGTM. label Aug 1, 2024
zhangjinpeng87
zhangjinpeng87 approved these changes Aug 2, 2024
View reviewed changes
Copy link
Member

@zhangjinpeng87 zhangjinpeng87 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@ti-chi-bot ti-chi-bot
Copy link
Contributor

ti-chi-bot bot commented Aug 2, 2024

@zhangjinpeng87: adding LGTM is restricted to approvers and reviewers in OWNERS files.

In response to this:

LGTM

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.

@zhangjinpeng87
Copy link
Member

/approve

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

ti-chi-bot bot commented Aug 2, 2024

[LGTM Timeline notifier]

Timeline:

  • 2024-08-01 12:11:39.300422852 +0000 UTC m=+5096.519184455: ☑️ agreed by rleungx.
  • 2024-08-02 01:40:58.627838519 +0000 UTC m=+53655.846600127: ☑️ agreed by niubell.

@LykxSassinator
Merge branch 'master' into large_region_size
4d2420e
@LykxSassinator
Fix config errors.
9d71f77 
Signed-off-by: lucasliang <nkcs_lykx@hotmail.com>
@LykxSassinator
Fix ut errors.
2b7446a 
Signed-off-by: lucasliang <nkcs_lykx@hotmail.com>
@ti-chi-bot ti-chi-bot
Copy link
Contributor

ti-chi-bot bot commented Aug 2, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: niubell, rleungx, zhangjinpeng87

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

The pull request process is described here

Needs approval from an approver in each of these files:

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

@LykxSassinator
Fix ut errs.
2143d9f 
Signed-off-by: lucasliang <nkcs_lykx@hotmail.com>
@LykxSassinator
Copy link
Contributor Author

/test

@ti-chi-bot ti-chi-bot
Copy link
Contributor

ti-chi-bot bot commented Aug 2, 2024

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

  • /test build

The following commands are available to trigger optional jobs:

  • /test pull-integration-copr-test
  • /test pull-integration-realcluster-test

Use /test all to run the following jobs that were automatically triggered:

  • tikv/pd/ghpr_build
  • tikv/pd/pull_integration_realcluster_test

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
Copy link
Contributor Author

/test all

@ti-chi-bot ti-chi-bot bot merged commit aa85b6c into tikv:master Aug 2, 2024
21 checks passed
@HuSharp HuSharp mentioned this pull request Aug 6, 2024
Closed
6 tasks
LykxSassinator added a commit to LykxSassinator/pd that referenced this pull request Aug 16, 2024
@LykxSassinator
Revert "*: enlarge the default value of max-merge-region-size. (tik…
3ec6a99 
…v#8445)"

This reverts commit aa85b6c.

Signed-off-by: lucasliang <nkcs_lykx@hotmail.com>
ti-chi-bot bot pushed a commit that referenced this pull request Aug 16, 2024
@LykxSassinator
Revert "*: enlarge the default value of max-merge-region-size. (#8445…
8b35fcd 
…)" (#8541)

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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@niubell niubell niubell approved these changes

@zhangjinpeng87 zhangjinpeng87 zhangjinpeng87 approved these changes

@rleungx rleungx rleungx approved these changes

@nolouch nolouch Awaiting requested review from nolouch

Assignees
No one assigned
Labels
approved dco-signoff: yes Indicates the PR's author has signed the dco. lgtm release-note Denotes a PR that will be considered when it comes time to generate release notes. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

region: larger region size for the compatibility to larger clusters.
5 participants
@LykxSassinator @zhangjinpeng87 @nolouch @niubell @rleungx