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.

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

scheduler: hotspot: add pending influence #1982

Merged
merged 14 commits into from
Dec 6, 2019

Conversation

Luffbee
Copy link
Contributor

@Luffbee Luffbee commented Dec 2, 2019

What problem does this PR solve?

Schedulers may overscheduled because the influence of some operators is not observed.

What is changed and how it works?

This PR introduce "pending influence" which records operators and their influence, then amend the statistics according to operator status and influence.

Check List

Tests

  • Unit test

@Luffbee Luffbee added type/enhancement The issue or PR belongs to an enhancement. component/schedule Scheduling logic. labels Dec 2, 2019
@Luffbee Luffbee added this to the v4.0.0-beta milestone Dec 2, 2019
@codecov-io
Copy link

codecov-io commented Dec 2, 2019

Codecov Report

Merging #1982 into master will increase coverage by 0.23%.
The diff coverage is 88.42%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1982      +/-   ##
==========================================
+ Coverage   77.35%   77.59%   +0.23%     
==========================================
  Files         178      178              
  Lines       18149    18240      +91     
==========================================
+ Hits        14040    14154     +114     
+ Misses       3020     3009      -11     
+ Partials     1089     1077      -12
Impacted Files Coverage Δ
server/schedule/operator/create_operator.go 87.73% <ø> (-0.05%) ⬇️
server/cluster/coordinator.go 88.12% <100%> (+4.68%) ⬆️
server/schedulers/utils.go 88.82% <100%> (+1.02%) ⬆️
server/schedulers/hot_region.go 87.13% <86%> (-0.95%) ⬇️
client/client.go 71.28% <0%> (-0.83%) ⬇️
server/grpc_service.go 57.23% <0%> (-0.44%) ⬇️
server/cluster/cluster.go 80.83% <0%> (+0.46%) ⬆️
server/server.go 83.1% <0%> (+0.58%) ⬆️
server/core/storage.go 75.44% <0%> (+1.19%) ⬆️
... and 6 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1881cab...3d252ea. Read the comment docs.

server/schedule/operator/create_operator.go Show resolved Hide resolved
server/schedulers/hot_test.go Outdated Show resolved Hide resolved
server/schedulers/utils.go Show resolved Hide resolved
server/schedulers/utils.go Outdated Show resolved Hide resolved
server/coordinator.go Outdated Show resolved Hide resolved
}
status := op.Status()
if !operator.IsEndStatus(status) {
return 1, false
Copy link
Contributor

Choose a reason for hiding this comment

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

why 1?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The operator now is running, so we will estimate that all influence of this region is not observed by statistics.

server/schedulers/hot_region.go Show resolved Hide resolved
@@ -229,27 +259,35 @@ func (h *balanceHotRegionsScheduler) balanceHotReadRegions(cluster opt.Cluster)
const balanceHotRetryLimit = 10

func (h *balanceHotRegionsScheduler) balanceHotWriteRegions(cluster opt.Cluster) []*operator.Operator {
balancePeer := h.r.Int()%2 == 0
Copy link
Contributor

Choose a reason for hiding this comment

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

why introduce it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sometimes it will only try one balance strategy. I think that is not what we want.

Copy link
Contributor

@nolouch nolouch left a comment

Choose a reason for hiding this comment

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

LGTM

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.

LGTM. @rleungx PTAL.

Copy link
Member

@rleungx rleungx left a comment

Choose a reason for hiding this comment

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

The rest LGTM.

@Luffbee Luffbee added the status/can-merge Indicates a PR has been approved by a committer. label Dec 6, 2019
@sre-bot
Copy link
Contributor

sre-bot commented Dec 6, 2019

/run-all-tests

@sre-bot sre-bot merged commit 7811255 into tikv:master Dec 6, 2019
@Luffbee Luffbee deleted the pending-influence branch December 7, 2019 10:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component/schedule Scheduling logic. status/can-merge Indicates a PR has been approved by a committer. type/enhancement The issue or PR belongs to an enhancement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants