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

simulator: remove balance-region-scheduler util all stores put the regions to pd #5252

Closed
wants to merge 14 commits into from
Closed

simulator: remove balance-region-scheduler util all stores put the regions to pd #5252

wants to merge 14 commits into from

Conversation

bufferflies
Copy link
Contributor

@bufferflies bufferflies commented Jul 1, 2022 •
edited
Loading

Signed-off-by: bufferflies 1045931706@qq.com

What problem does this PR solve?

Issue Number: Close #5251,#5290

What is changed and how does it work?

Check List

Tests

  • Manual test (add detailed scripts or steps below)

Code changes

Side effects

Related changes

Release note

 None.

@bufferflies
add balance region
1fdf598 
Signed-off-by: bufferflies <1045931706@qq.com>
@bufferflies
remove startSchedulerTick
12dc12c 
Signed-off-by: bufferflies <1045931706@qq.com>
@ti-chi-bot
Copy link
Member

[REVIEW NOTIFICATION]

This pull request has not been approved.

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.

lhy1024
lhy1024 reviewed Jul 1, 2022
View reviewed changes
Copy link
Contributor

@lhy1024 lhy1024 left a comment

Choose a reason for hiding this comment

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

There are typos in the title, which should be "simulator: remove balance-region-scheduler until all stores put the regions to pd"

@bufferflies bufferflies changed the title simulator: remove balance-region-scheduler util all store put the regions to pd simulator: remove balance-region-scheduler until all stores put the regions to pd Jul 1, 2022
@codecov
Copy link

codecov bot commented Jul 4, 2022 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 75.88%. Comparing base (a5c00c0) to head (6a38843).
Report is 1299 commits behind head on master.

Additional details and impacted files 
@@            Coverage Diff             @@
##           master    #5252      +/-   ##
==========================================
+ Coverage   75.75%   75.88%   +0.12%     
==========================================
  Files         312      312              
  Lines       30962    30963       +1     
==========================================
+ Hits        23456    23496      +40     
+ Misses       5498     5479      -19     
+ Partials     2008     1988      -20
Flag Coverage Δ
unittests 75.88% <100.00%> (+0.12%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@bufferflies
simulator
9b9f1fa 
Signed-off-by: bufferflies <1045931706@qq.com>
@bufferflies
simulator/io-limiter
2bb7cf1 
Signed-off-by: bufferflies <1045931706@qq.com>
@bufferflies
add one metrics
c6f7726 
Signed-off-by: bufferflies <1045931706@qq.com>
@bufferflies
remove unused files
d206fad 
Signed-off-by: bufferflies <1045931706@qq.com>
@bufferflies
revert
c8ef65b 
Signed-off-by: bufferflies <1045931706@qq.com>
@bufferflies
Merge branches 'master' and 'master' of github.com:tikv/pd
efc783c
@bufferflies
Merge branch 'master' into simulator/io-limiter
54cf7de
@bufferflies bufferflies changed the title simulator: remove balance-region-scheduler until all stores put the regions to pd simulator: remove balance-region-scheduler util all stores put the regions to pd Jul 18, 2022
@bufferflies bufferflies requested a review from lhy1024 July 20, 2022 07:26
CabinfeverB
CabinfeverB reviewed Aug 17, 2022
View reviewed changes
Copy link
Member

@CabinfeverB CabinfeverB left a comment

Choose a reason for hiding this comment

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

It's better to pause balance-region-scheduler, hot-region-scheduler and balance-leader-scheduler together?

lhy1024
lhy1024 reviewed Sep 1, 2022
View reviewed changes
Copy link
Contributor

@lhy1024 lhy1024 left a comment

Choose a reason for hiding this comment

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

rest LGTM

tools/pd-simulator/simulator/drive.go
@@ -106,6 +110,12 @@ func (d *Driver) Prepare() error {
func (d *Driver) Tick() {
d.tickCount++
d.raftEngine.stepRegions()
// simulator don't need any schedulers util all stores send their heartbeat.
if d.tickCount <= 1 {
d.client.RemoveScheduler(balanceRegionScheduler)
Copy link
Contributor

Choose a reason for hiding this comment

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

also need to remove balance leader scheduler?

@lhy1024 lhy1024 mentioned this pull request Sep 1, 2022
Open
20 tasks
@nolouch
Copy link
Contributor

nolouch commented Sep 5, 2022

cc @bufferflies

@ti-chi-bot ti-chi-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 22, 2022
@ti-chi-bot
Copy link
Member

@bufferflies: PR needs rebase.

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.

@lhy1024
Copy link
Contributor

lhy1024 commented Oct 24, 2022

any update?

@HuSharp HuSharp mentioned this pull request Jun 5, 2024
Merged
@HuSharp HuSharp closed this Jun 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lhy1024 lhy1024 lhy1024 left review comments

@CabinfeverB CabinfeverB CabinfeverB left review comments

@rleungx rleungx Awaiting requested review from rleungx

Assignees
No one assigned
Labels
componet/simulator needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. release-note-none
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

simulator: remove balance-region-scheduler util all store put the regions to pd.
6 participants
@bufferflies @ti-chi-bot @nolouch @lhy1024 @CabinfeverB @HuSharp