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

copr: MPP balance regions between TiFlash nodes with continuity. #28906

Merged
merged 11 commits into from
Oct 20, 2021
Merged

copr: MPP balance regions between TiFlash nodes with continuity. #28906

merged 11 commits into from
Oct 20, 2021

Conversation

JinheLin
Copy link
Contributor

@JinheLin JinheLin commented Oct 18, 2021 •
edited
Loading

What problem does this PR solve?

Problem Summary:
Regions balance between TiFlash nodes should take account of region’s physical continuity in TiFlash nodes for better read performance.

What is changed and how it works?

Proposal: Better read plan of region peer between TiFlash nodes

What's Changed:

Add a new balance strategy that take account of region's order:

  1. Sort regions by key ranges.
  2. Assign several regions to a store each round until all regions are assigned.

Add two TiDB variables:
1.tidb_enable_mpp_balance_with_continuous_region indicates whether take account of region's order when doing MPP balance, default is ON.
2. tidb_enable_mpp_balance_with_continuous_region_count indicates the region count assigned to a store each round when doing MPP balance, default is 20.

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

Documentation

  • Affects user behaviors
  • Contains syntax changes
  • Contains variable changes
  • Contains experimental features
  • Changes MySQL compatibility

Release note 

Optimize MPP balance strategy and improve read I/O performance of TiFlash.

Result

Disk read metric of TPCH-100 workload:

  1. Left is metrics before this PR.
  2. Right is metrics of this PR.

tpch-100

@ti-chi-bot
Copy link
Member

ti-chi-bot commented Oct 18, 2021 •
edited
Loading

[REVIEW NOTIFICATION]

This pull request has been approved by:

  • cfzjywxk
  • fzhedu

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.

@JinheLin JinheLin requested a review from a team as a code owner October 18, 2021 06:32
@ti-chi-bot 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 Oct 18, 2021
@JinheLin
Copy link
Contributor Author

@fzhedu PTAL.
cc @flowbehappy

@JinheLin
Copy link
Contributor Author

/run-all-tests

@JinheLin
Copy link
Contributor Author

@windtalker please take a look.

@ti-chi-bot ti-chi-bot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-none Denotes a PR that doesn't merit a release note. labels Oct 19, 2021
fzhedu
fzhedu approved these changes Oct 19, 2021
View reviewed changes
Copy link
Contributor

@fzhedu fzhedu left a comment

Choose a reason for hiding this comment

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

/LGTM except let the TiDBEnableMPPBalanceWithContinuity be clear to mean read continous regions.

@ti-chi-bot ti-chi-bot added the status/LGT1 Indicates that a PR has LGTM 1. label Oct 19, 2021
windtalker
windtalker reviewed Oct 19, 2021
View reviewed changes
@ti-chi-bot ti-chi-bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Oct 19, 2021
@ti-chi-bot ti-chi-bot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Oct 19, 2021
@JinheLin
Copy link
Contributor Author

JinheLin commented Oct 19, 2021 •
edited
Loading

/cc @morgo @bb7133 @cfzjywxk PTAL

@JinheLin JinheLin requested a review from cfzjywxk October 19, 2021 10:52
@ti-chi-bot 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 Oct 19, 2021
@JinheLin
Copy link
Contributor Author

/run-all-tests

@JinheLin
Copy link
Contributor Author

/run-all-tests

@JinheLin
Copy link
Contributor Author

/merge

@ti-chi-bot
Copy link
Member

@JinheLin: /merge is only allowed for the committers, you can assign this pull request to the committer in list by filling /assign @committer in the comment to help merge this pull request.

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.

@windtalker
Copy link
Contributor

/merge

@windtalker
Copy link
Contributor

/merge

@JinheLin
Copy link
Contributor Author

@cfzjywxk @fzhedu Cloud you help me merge this PR?

@windtalker
Copy link
Contributor

/merge

@ti-chi-bot
Copy link
Member

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

Commit hash: 2b28841

@ti-chi-bot ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Oct 20, 2021
@ti-chi-bot ti-chi-bot merged commit 861bc15 into pingcap:master Oct 20, 2021
@JinheLin
Copy link
Contributor Author

/cherrypick release-5.1-hotfix-tiflash-patch1

1 similar comment
@JinheLin
Copy link
Contributor Author

/cherrypick release-5.1-hotfix-tiflash-patch1

This was referenced Oct 20, 2021
Closed
Merged
@morgo
Copy link
Contributor

morgo commented Oct 25, 2021 •
edited
Loading

Add two TiDB variables:
1.tidb_enable_mpp_balance_region_with_continuity indicates whether take account of region's order when doing MPP balance, default is ON.
2. tidb_mpp_balance_continuous_region_count indicates the region count assigned to a store each round when doing MPP balance, default is 20.

@JinheLin can you please provide a docs PR for these two variables?

Also tidb_mpp_balance_continuous_region_count is listed as hidden. We have a proposal to unhide variables. Can this be converted to visible?

(Also: sorry I missed the ping. I should have made these comments during review!)

@JinheLin
Copy link
Contributor Author

@morgo

OK, I will provide a docs PR this week.

This variable can be converted to visible, it doesn't matter.

@JinheLin JinheLin mentioned this pull request Oct 27, 2021
Closed
13 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@windtalker windtalker windtalker left review comments

@flowbehappy flowbehappy flowbehappy left review comments

@fzhedu fzhedu fzhedu approved these changes

@cfzjywxk cfzjywxk cfzjywxk approved these changes

Assignees
No one assigned
Labels
release-note Denotes a PR that will be considered when it comes time to generate release notes. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. status/can-merge Indicates a PR has been approved by a committer. status/LGT2 Indicates that a PR has LGTM 2.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@JinheLin @ti-chi-bot @windtalker @morgo @flowbehappy @cfzjywxk @fzhedu