-
Notifications
You must be signed in to change notification settings - Fork 719
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: solution-based key-rate-aware balance solver #2141
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2141 +/- ##
==========================================
- Coverage 76.27% 76.05% -0.23%
==========================================
Files 195 195
Lines 20577 20577
==========================================
- Hits 15696 15650 -46
- Misses 3703 3734 +31
- Partials 1178 1193 +15
Continue to review full report at Codecov.
|
if len(ops) > 0 { | ||
return ops | ||
} | ||
// prefer to balance by peer |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After removing the retry loop, will it affect the speed of hot spot scheduling?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe it won't. The the statistics data is not changed for each iteration, and new balanceSolver
will try almost all possible solutions.
server/schedulers/hot_region.go
Outdated
} | ||
} else { | ||
keyDecRatio := (dstLd.KeyRate + peer.GetKeyRate()) / (srcLd.KeyRate + 1) | ||
keyHot := peer.GetKeyRate() >= 10 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how about using a constant?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It won't be used in other place, so I don't think it is necessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The constant is easy to maintain and the meaning is much more clear.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add two constants: minHotByteRate
and minHotKeyRate
.
server/schedulers/hot_region.go
Outdated
case read: | ||
return readLeader | ||
} | ||
return resourceTypeLen |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will it cause panic?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It will only cause panic when the arguments are invalid. I think this is acceptable.
There was a problem hiding this 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 panic in this function rather than use it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
if bs.rwTy == write && bs.opTy == transferLeader { | ||
lpCmp = sliceLPCmp( | ||
minLPCmp(negLoadCmp(sliceLoadCmp( | ||
stLdRankCmp(stLdCount, stepRank(bs.maxSrc.Count, bs.rankStep.Count)), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What the meaning of rankStep
? it seems always the same in the rank compare.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is used to discretization. Map float64
to int64
. For all values in the interval [rank0 + i * step, rank0 + (i+1) * step)
, the rank the i
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it. If I understand correctly, the ranking i
(stepRank return) can only be 0 or 1. Can this distinguish those stores very well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, rank i = int64((value - rank0) / step)
can be any int64
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, sorry. I missed the stepRatio
, can u add some comments about step ratio?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comments added.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm.
/merge |
/run-all-tests |
What problem does this PR solve?
The second step of #2139 .
What is changed and how it works?
Introduce solution based balance solver.
Check List
Tests