Skip to content

locate: honor PD slow-store recovery state for replica reads#2027

Draft
LykxSassinator wants to merge 3 commits into
tikv:masterfrom
LykxSassinator:issue-2012-honor-pd-store-scheduling-state
Draft

locate: honor PD slow-store recovery state for replica reads#2027
LykxSassinator wants to merge 3 commits into
tikv:masterfrom
LykxSassinator:issue-2012-honor-pd-store-scheduling-state

Conversation

@LykxSassinator

Copy link
Copy Markdown
Contributor

What problem does this PR solve?

Replica selection currently relies on client-go's local health observations. A store can recover locally before PD's slow-store recovery-time expires, causing follower/learner/stale reads to be routed to a store that PD still evicts as slow.

Related issue: ref #2012

What is changed?

  • Consume GetStoreResponse.scheduling_state introduced by kvproto memdb: retain old version nodes of ART to satisfy snapshot read #1503 and exposed by PD #11041.
  • Use the optional pd.RPCClientExt capability to preserve source compatibility for existing pd.Client implementations and mocks.
  • Keep PD-owned scheduling state separate from StoreHealthStatus, which only records local and TiKV-reported health observations.
  • Gate every non-leader replica candidate on both local health and PD scheduling state, covering follower, learner, stale reads, and leader-read replica fallback.
  • Use an epoch-based pending confirmation gate so an older PD response cannot re-admit a store after a newer local health transition.
  • Query the PD leader for state-aware GetStore refreshes because Router intentionally does not provide dynamic scheduling state.
  • Update codec/interceptor wrappers and test mocks for the optional full-response API.

Compatibility

Tests

  • go test ./internal/locate
  • go test ./... -run '^$'
  • cd integration_tests && go test ./...

Signed-off-by: lucasliang <nkcs_lykx@hotmail.com>
@ti-chi-bot

ti-chi-bot Bot commented Jul 23, 2026

Copy link
Copy Markdown

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@ti-chi-bot ti-chi-bot Bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. dco-signoff: yes Indicates the PR's author has signed the dco. labels Jul 23, 2026
@ti-chi-bot

ti-chi-bot Bot commented Jul 23, 2026

Copy link
Copy Markdown

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign ekexium for approval. For more information see the Code Review Process.
Please ensure that each of them provides their approval before proceeding.

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

Details Needs approval from an approver in each of these files:

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

@coderabbitai

coderabbitai Bot commented Jul 23, 2026

Copy link
Copy Markdown

Important

Review skipped

Draft detected.

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 Plus

Run ID: 8aa0be68-1c50-49dc-b27f-b1fab025d820

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 added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Jul 23, 2026
@LykxSassinator

Copy link
Copy Markdown
Contributor Author

I considered adding StoreSchedulingState directly to metapb.Store so client-go could reuse pd.Client.GetStore and retain the Router-first query path. I do not recommend doing that in the current proposal.

EvictedAsSlowStore is not regular Store metadata. It is a dynamic, PD-leader-owned scheduling decision whose lifetime is governed by PD's recovery-time policy. The current PD implementation intentionally keeps it separate:

  • Store is populated from store.GetMeta().
  • SchedulingState is populated from store.EvictedAsSlowStore().
  • Router serves GetStore from BasicCluster, which does not retain this dynamic scheduling state and therefore returns it as unknown.

Moving the field into metapb.Store would remove the RPCClientExt plumbing, but would not safely remove the PD leader query:

  1. A Router response would still carry an absent or stale state unless PD first introduced a state-synchronization path from the PD leader to Router.
  2. Treating an absent Router field as false could re-admit a store that PD still evicts; treating it as unknown requires a fallback query to the PD leader.
  3. Router-first followed by a leader fallback adds an RPC without reducing the required leader query for the current Router implementation.
  4. metapb.Store is broadly reused by store metadata, registration, and watch paths. Publishing a runtime scheduling decision there expands its propagation and freshness contract substantially.
  5. metapb.proto cannot directly reference pdpb.StoreSchedulingState because pdpb.proto already depends on metapb.proto; the type would also need to move into the generic metadata package.

The response-level field keeps the contract explicit: it is PD scheduling state for this GetStore response, and an unset value means unknown, not false.

If reducing PD leader traffic becomes a goal, the follow-up should be to replicate a versioned/freshness-aware routing-state snapshot to Router. Client-go could then accept a fresh Router response and fall back to the PD leader only when the state is absent or stale. That achieves the desired offload without conflating dynamic scheduling policy with Store metadata.

Signed-off-by: lucasliang <nkcs_lykx@hotmail.com>
@ti-chi-bot ti-chi-bot Bot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jul 23, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

dco-signoff: yes Indicates the PR's author has signed the dco. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant