schedule: fix split-merge-interval update #8405
Conversation
Signed-off-by: lhy1024 <admin@liudos.us>
ti-chi-bot
bot
added
release-note-none
do-not-merge/needs-triage-completed
dco-signoff: yes
size/XS
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #8405 +/- ##
=======================================
Coverage 77.23% 77.23%
=======================================
Files 471 471
Lines 61728 61728
=======================================
+ Hits 47674 47677 +3
Misses 10466 10466
+ Partials 3588 3585 -3
Flags with carried forward coverage won't be shown. Click here to find out more. |
Signed-off-by: lhy1024 <admin@liudos.us>
ti-chi-bot
bot
added
size/S
| @@ -88,13 +90,16 @@ func (m *MergeChecker) Check(region *core.RegionInfo) []*operator.Operator { | |||
| return nil | |||
| } | |||
|
|
|||
| // update the split cache. | |||
| // It must be called before the following merge checker logic. | |||
| m.splitCache.UpdateTTL(m.conf.GetSplitMergeInterval()) | |||
There was a problem hiding this comment.
BTW, will it have a risk? If the heartbeat is not sent in time?
There was a problem hiding this comment.
It has the same behavior as before in this case.
There was a problem hiding this comment.
At the same time, we wait until 90% of the heartbeats are collected before checking.
There was a problem hiding this comment.
There still be 10% and merge check may create lots of scheduling?
There was a problem hiding this comment.
The behavior is similar in this case. For example, if split-merge-interval is set to 1s.
In the past, it would pass the recently started check and then run UpdateTTL
Now, it will run UpdateTTL first and then pass the recently started check.
Perhaps we can add a longer time before recently started check, such as expireTime := m.startTime.Add(m.conf.GetSplitMergeInterval()+5*time.Minute)
Or we can add a filter to check whether the region reports a heartbeat after PD started. Will L109 be helpful?
|
@okJiang: adding LGTM is restricted to approvers and reviewers in OWNERS files. In response to this:
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. |
Signed-off-by: lhy1024 <admin@liudos.us>
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: nolouch, okJiang, rleungx The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
What problem does this PR solve?
Issue Number: Close #8404
What is changed and how does it work?
Check List
Tests
test will failed with master branch
Release note