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

Changes from 1 commit
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
e5fe823
*: enlarge the default value of region size.
LykxSassinator Jul 26, 2024
aaa2080
Address comments.
LykxSassinator Jul 29, 2024
1444056
Merge branch 'master' into large_region_size
LykxSassinator Jul 29, 2024
8c8a6bb
Polish codes.
LykxSassinator Jul 29, 2024
6c5156d
Merge branch 'master' into large_region_size
LykxSassinator Aug 1, 2024
94ebe5b
Merge branch 'master' into large_region_size
LykxSassinator Aug 1, 2024
4d2420e
Merge branch 'master' into large_region_size
LykxSassinator Aug 2, 2024
9d71f77
Fix config errors.
LykxSassinator Aug 2, 2024
2b7446a
Fix ut errors.
LykxSassinator Aug 2, 2024
2143d9f
Fix ut errs.
LykxSassinator Aug 2, 2024
5015166
Merge branch 'master' into large_region_size
LykxSassinator Aug 2, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion pkg/schedule/config/config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.

defaultLeaderScheduleLimit = 4
defaultRegionScheduleLimit = 2048
defaultWitnessScheduleLimit = 4
Expand Down
Loading