schedule: skip slow/evicted stores in hot region scheduling#10946
schedule: skip slow/evicted stores in hot region scheduling#10946LykxSassinator wants to merge 1 commit into
Conversation
Signed-off-by: lucasliang <nkcs_lykx@hotmail.com>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (7)
📝 WalkthroughWalkthroughAdds a new hot-region target filter that rejects evicted stores, registers its filter type name, wires it into hot-region scheduler target selection, and adds unit and scheduler tests for eviction cases. ChangesHot-region target eviction filtering
Sequence Diagram(s)sequenceDiagram
participant scheduler as "grantHotRegionScheduler.transfer"
participant filter as "hotRegionEvictedTargetFilter"
participant store as "core.StoreInfo"
scheduler->>filter: Target(store)
filter->>store: EvictedAsSlowStore() / EvictedAsStoppingStore() / IsEvictedAsSlowTrend()
store-->>filter: eviction state
filter-->>scheduler: statusOK / statusStoreRejectLeader
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
/cc @rleungx @bufferflies |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #10946 +/- ##
==========================================
- Coverage 79.26% 79.17% -0.09%
==========================================
Files 540 541 +1
Lines 74954 75103 +149
==========================================
+ Hits 59412 59465 +53
- Misses 11371 11438 +67
- Partials 4171 4200 +29
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
|
|
||
| // Target filters stores when select them as schedule target. | ||
| func (*hotRegionEvictedTargetFilter) Target(_ config.SharedConfigProvider, store *core.StoreInfo) *plan.Status { | ||
| if store.EvictedAsSlowStore() || store.EvictedAsStoppingStore() || store.IsEvictedAsSlowTrend() { |
There was a problem hiding this comment.
This only filters stores that have already been marked as evicted; a store with a high slow score but no eviction flag can still pass.
What problem does this PR solve?
Issue Number: Close #10936
Hot-region scheduling can still choose a destination store that has already
been marked as unsuitable for hot traffic, such as slow stores, stopping
stores, or stores evicted by slow-trend detection.
What is changed and how does it work?
Check List
Tests
Code changes
Side effects
Related changes
pingcap/docs/pingcap/docs-cn:pingcap/tiup:Release note
Deep Review
Problem Summary
#10936is about hot-region scheduling still selecting destination stores that PD has already marked as unsuitable for receiving hot-region traffic.StoreStateFilterregionTargetlogic would also change unrelatedMoveRegionusers such as replica repair and generic balance-region flows.Solution Walkthrough
pkg/schedule/filter/filters.go:572-600addsNewHotRegionEvictedTargetFilter, a target-only filter that rejects stores when any of these store-state flags are set:store.EvictedAsSlowStore()store.EvictedAsStoppingStore()store.IsEvictedAsSlowTrend()pkg/schedule/schedulers/hot_region_solver.go:564-569wires the new filter into the hot-regionmovePeerdestination path, which covers hot peer relocation.pkg/schedule/schedulers/hot_region_solver.go:585-588also adds the filter to the hot-read move-leader fallback candidate set, which is the non-obvious path that can still materialize as aMoveLeader-style choice inside hot scheduling.pkg/schedule/schedulers/grant_hot_region.go:301-304applies the same filter togrant-hot-regiondestination selection.pkg/schedule/schedulers/shuffle_hot_region.go:156-160applies it toshuffle-hot-regiondestination selection.pkg/schedule/filter/filters_test.go:304-323adds direct coverage for the new filter and verifies all three eviction states.pkg/schedule/schedulers/hot_region_test.go:1333-1422adds scheduler regressions for:StoreStateFilterregionTargetbehavior, so non-hotMoveRegioncallers keep their previous semantics.Findings (ordered by severity)
Costs and Negative Impacts
MoveRegionbehavior.slow-store,stopping-store, andslow-trendeviction states, not onlyslowStoreEvicted.leaderEvicted/"leader-evicted-filter"inpkg/schedule/filter/counter.go:55-86, while the exported constructor name is nowNewHotRegionEvictedTargetFilter. This is a maintainability and observability clarity cost, not a correctness bug.Engineering Rules Check
Questions and Assumptions
Suggested Tests / Validation
pkg/schedule/filter/filters_test.go:304-323andpkg/schedule/schedulers/hot_region_test.go:1333-1422is directionally correct and exercises the two highest-risk solver paths.grant-hot-regionrejecting an evicted destination store.shuffle-hot-regionrejecting an evicted destination store.Summary by CodeRabbit
New Features
Bug Fixes