Skip to content

schedule: skip slow/evicted stores in hot region scheduling (#10946)#10972

Open
ti-chi-bot wants to merge 1 commit into
tikv:release-8.5from
ti-chi-bot:cherry-pick-10946-to-release-8.5
Open

schedule: skip slow/evicted stores in hot region scheduling (#10946)#10972
ti-chi-bot wants to merge 1 commit into
tikv:release-8.5from
ti-chi-bot:cherry-pick-10946-to-release-8.5

Conversation

@ti-chi-bot

Copy link
Copy Markdown
Member

This is an automated cherry-pick of #10946

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?

add a hot-region-specific target filter that rejects stores evicted as
slow-store, stopping-store, or slow-trend

apply the filter to hot-region destination selection in the balance-hot-region
solver, grant-hot-region scheduler, and shuffle-hot-region scheduler

add unit tests for the new filter and scheduler regressions covering hot-read
move-leader fallback and hot-read peer scheduling

Check List

Tests

  • Unit test
  • Integration test
  • Manual test (add detailed scripts or steps below)
  • No code

Code changes

Side effects

  • Possible performance regression
  • Increased code complexity
  • Breaking backward compatibility

Related changes

Release note

Hot-region schedulers now skip stores evicted by slow-store, stopping-store,
or slow-trend checks when selecting scheduling targets.

Deep Review

Problem Summary

  • Issue #10936 is about hot-region scheduling still selecting destination stores that PD has already marked as unsuitable for receiving hot-region traffic.
  • The practical requirement is narrower than generic region movement: hot-region schedulers should not move hot peers or hot leaders onto stores evicted due to slow-store, stopping-store, or slow-trend signals.
  • The earlier broader fix path was risky because putting this policy into shared StoreStateFilter regionTarget logic would also change unrelated MoveRegion users such as replica repair and generic balance-region flows.

Solution Walkthrough

  • pkg/schedule/filter/filters.go:572-600 adds NewHotRegionEvictedTargetFilter, 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-569 wires the new filter into the hot-region movePeer destination path, which covers hot peer relocation.
  • pkg/schedule/schedulers/hot_region_solver.go:585-588 also adds the filter to the hot-read move-leader fallback candidate set, which is the non-obvious path that can still materialize as a MoveLeader-style choice inside hot scheduling.
  • pkg/schedule/schedulers/grant_hot_region.go:301-304 applies the same filter to grant-hot-region destination selection.
  • pkg/schedule/schedulers/shuffle_hot_region.go:156-160 applies it to shuffle-hot-region destination selection.
  • pkg/schedule/filter/filters_test.go:304-323 adds direct coverage for the new filter and verifies all three eviction states.
  • pkg/schedule/schedulers/hot_region_test.go:1333-1422 adds scheduler regressions for:
    • hot-read move-leader fallback skipping evicted targets
    • hot-read peer scheduling skipping evicted targets
  • Importantly, the patch does not modify shared StoreStateFilter regionTarget behavior, so non-hot MoveRegion callers keep their previous semantics.

Findings (ordered by severity)

  • No concrete correctness, compatibility, or robustness bugs found in the current scoped implementation.

Costs and Negative Impacts

  • Correctness:
    • The current patch fixes the hot-region scheduling gap without changing global MoveRegion behavior.
    • Coverage is broader than the original practical suggestion because it consistently honors slow-store, stopping-store, and slow-trend eviction states, not only slowStoreEvicted.
  • Security:
    • None found.
  • Compatibility:
    • The behavioral change is intentionally limited to hot-region schedulers and should not affect replica checker, balance-region, or other non-hot placement flows.
  • Robustness:
    • The main hot-region target-selection paths are covered in code and tests.
    • Residual risk is mostly future regression from missed filter wiring in less-direct hot schedulers, not a currently observed logic hole.
  • Cognitive Load:
    • Slight increase from introducing one hot-region-specific filter.
    • There is also a small naming mismatch in the internal filter counter: the new filter reports leaderEvicted / "leader-evicted-filter" in pkg/schedule/filter/counter.go:55-86, while the exported constructor name is now NewHotRegionEvictedTargetFilter. This is a maintainability and observability clarity cost, not a correctness bug.
  • CPU:
    • Negligible. The filter adds a few boolean checks per hot-region target candidate.
  • Memory:
    • Negligible.
  • Log Volume:
    • No meaningful change.

Engineering Rules Check

  • None.

Questions and Assumptions

  • Assumed the intended scope is exactly the hot-region scheduler family, based on the issue context and the user’s requested practical fix direction.
  • Assumed the current worktree diff is the authoritative patch under review.

Suggested Tests / Validation

  • The added unit coverage in pkg/schedule/filter/filters_test.go:304-323 and pkg/schedule/schedulers/hot_region_test.go:1333-1422 is directionally correct and exercises the two highest-risk solver paths.
  • Residual test gap: add a scheduler-level negative test for grant-hot-region rejecting an evicted destination store.
  • Residual test gap: add a scheduler-level negative test for shuffle-hot-region rejecting an evicted destination store.

Summary by CodeRabbit

  • New Features

    • Added support for avoiding destination stores that have been marked for hot-region eviction during hot leader and region scheduling.
    • Expanded scheduling logic so these stores are skipped when selecting target stores.
  • Bug Fixes

    • Improved hot read scheduling to prevent transfers to stores that should no longer receive leadership or peer moves.
    • Added coverage to verify scheduling behaves correctly across multiple eviction states.

Signed-off-by: lucasliang <nkcs_lykx@hotmail.com>
@ti-chi-bot ti-chi-bot added dco-signoff: yes Indicates the PR's author has signed the dco. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. type/cherry-pick-for-release-8.5 This PR is cherry-picked to release-8.5 from a source PR. labels Jul 3, 2026
@ti-chi-bot

ti-chi-bot Bot commented Jul 3, 2026

Copy link
Copy Markdown
Contributor

This cherry pick PR is for a release branch and has not yet been approved by triage owners.
Adding the do-not-merge/cherry-pick-not-approved label.

To merge this cherry pick:

  1. It must be LGTMed and approved by the reviewers firstly.
  2. For pull requests to TiDB-x branches, it must have no failed tests.
  3. AFTER it has lgtm and approved labels, please wait for the cherry-pick merging approval from triage owners.
Details

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-sigs/prow repository.

@coderabbitai

coderabbitai Bot commented Jul 3, 2026

Copy link
Copy Markdown

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: d1179800-0f4d-4338-bb07-c549fe392176

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@ti-chi-bot

ti-chi-bot Bot commented Jul 3, 2026

Copy link
Copy Markdown
Contributor

@LykxSassinator: adding LGTM is restricted to approvers and reviewers in OWNERS files.

Details

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-sigs/prow repository.

@ti-chi-bot ti-chi-bot Bot added needs-1-more-lgtm Indicates a PR needs 1 more LGTM. approved labels Jul 3, 2026
@ti-chi-bot ti-chi-bot Bot added the lgtm label Jul 3, 2026
@ti-chi-bot

ti-chi-bot Bot commented Jul 3, 2026

Copy link
Copy Markdown
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bufferflies, LykxSassinator, rleungx

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:
  • OWNERS [bufferflies,rleungx]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@ti-chi-bot ti-chi-bot Bot removed the needs-1-more-lgtm Indicates a PR needs 1 more LGTM. label Jul 3, 2026
@ti-chi-bot

ti-chi-bot Bot commented Jul 3, 2026

Copy link
Copy Markdown
Contributor

[LGTM Timeline notifier]

Timeline:

  • 2026-07-03 02:02:32.17173982 +0000 UTC m=+350493.872119233: ☑️ agreed by rleungx.
  • 2026-07-03 02:16:11.204814807 +0000 UTC m=+351312.905194220: ☑️ agreed by bufferflies.

@LykxSassinator

Copy link
Copy Markdown
Contributor

/retest

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved dco-signoff: yes Indicates the PR's author has signed the dco. do-not-merge/cherry-pick-not-approved lgtm release-note Denotes a PR that will be considered when it comes time to generate release notes. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. type/cherry-pick-for-release-8.5 This PR is cherry-picked to release-8.5 from a source PR.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants