internal/locate: probe follower once when the cached leader keeps rejecting leader reads with ServerIsBusy(0) (#2041) - #2044
Merged
ti-chi-bot[bot] merged 5 commits intoAug 6, 2026
Conversation
…ecting leader reads with ServerIsBusy(0) ref tikv#2028 When a TiKV store's unified read pool is wedged, it rejects leader reads with ServerIsBusy(EstimatedWaitMs=0) at the pool entrance, so the request never reaches the raft layer and no NotLeader error is returned even if PD has already moved the leader away. The replica selector then retries the cached leader forever and hammers the half-dead store. After 2 consecutive such rejections on the cached leader within one selector, mark the leader replica with a new suspectNotLeaderFlag so that the next attempt skips it in the leader strategy and probes a follower via the mixed strategy with the leader-read semantics of the request unchanged. The follower replies NotLeader with the real leader hint, which heals the shared region cache through the existing onNotLeader/updateLeader path. The probe fires at most once per selector; if the store is still the leader, the hint points back to it, onUpdateLeader clears the flag, and the only cost is one rejected RPC. Signed-off-by: Ziqian Qin <eke@fastmail.com>
…der when no probe target, tie busy count to the cached leader Address review on tikv#2041, ref tikv#2028. - When the mixed strategy finds no candidate while the leader is skipped only due to suspectNotLeaderFlag (single-replica region, or all followers unreachable/stale/exhausted, or the probe turns out fruitless), restore the leader and fall back to the plain backoff-retry behavior instead of invalidating the region and reloading it from PD for nothing. - Tie leaderBusyCount to the new leaderBusyPeerID: whenever the cached leader changes (e.g. switched by a NotLeader hint), restart the count so the new leader won't be marked after inheriting the old leader's count. Signed-off-by: Ziqian Qin <eke@fastmail.com>
The trigger is the 2nd ServerIsBusy(0) from the same cached leader within one selector, not necessarily on consecutive attempts; the count restarts only when the cached leader changes. Comment-only change. Signed-off-by: Ziqian Qin <eke@fastmail.com>
…hreshold Address review on tikv#2041, ref tikv#2028. - Hoist the GetLeaderPeerID read out of the probe trigger condition so it is read once and shared by the condition and the count-reset logic, closing a TOCTOU window against concurrent cached-leader changes (double-read review comment by zyguan). - Name the probe trigger threshold as leaderBusyProbeThreshold instead of a literal 2. - Add contract tests: the busy count is cumulative per cached leader (busy(0) interleaved with other errors still counts), and when both probed followers reply NotLeader without a hint, the leader is restored with the region kept valid instead of being eagerly invalidated. Signed-off-by: Ziqian Qin <eke@fastmail.com>
ti-chi-bot
added
dco-signoff: yes
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
tidb-8.5 does not have the config.NextGen build constant, so the stale read / follower read cases in TestReplicaSelectorLeaderBusyProbe must run unconditionally. Also adjust the stale-read access path expectation: tidb-8.5 falls back to replica read after the first ServerIsBusy(0), unlike master which keeps the stale-read semantics. Signed-off-by: Ziqian Qin <eke@fastmail.com>
zyguan
approved these changes
Aug 6, 2026
ti-chi-bot
Bot
added
needs-1-more-lgtm
cfzjywxk
approved these changes
Aug 6, 2026
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: cfzjywxk, zyguan The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
2 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This is an automated cherry-pick of #2041
Close #2028
Problem
When a TiKV store becomes half-dead (e.g. unified read pool wedged, tikv/tikv#18491),
leader reads are rejected at the read-pool entrance with
ServerIsBusyandEstimatedWaitMs == 0, before the request reaches the raft layer. Even after PD hasmoved the leader away, the store keeps answering gRPC with
ServerIsBusy(0)instead ofNotLeader. The client only marks the store slow and backs off, retrying the samecached leader indefinitely — a production incident lasted ~25 minutes and ended only
when the store became fully unreachable.
Fix
"Sleep first, probe once":
ServerIsBusy(0)on the cached leader behaves exactly as today (mark slow +backoff + retry leader) — transient busyness costs nothing.
ServerIsBusy(0)from the same cached leader within one selector,mark the leader replica with a new dedicated
suspectNotLeaderFlag(at most onceper selector; the count restarts only when the cached leader changes).
follower with unchanged request semantics (
req.ReplicaReadstays false). A followercan only reject a leader read with
NotLeader+ leader hint — never serve it — sothere is no stale-read risk.
onNotLeader→updateLeaderpath, healing the sharedregion cache for all subsequent requests.
onUpdateLeaderclears the flag, so amisjudgment (the store is still the leader) costs exactly one rejected RPC and the
request reverts to today's behavior.
busy/exhausted), the selector restores the cached leader when the mixed strategy
finds no candidate, instead of invalidating the region: it falls back to the plain
backoff-and-retry loop, keeping essentially today's retry shape in an all-busy
meltdown and adding no PD traffic. Region invalidation / PD reload still happen only
through the pre-existing paths (e.g. after the leader exhausts its attempts).
No read/write semantics change, no extra PD traffic on the busy path, no new state
machines/timers/configs. Bounded extra cost: the probe fires at most once per
selector — when it succeeds (or is a misjudgment) it costs one rejected RPC; in the
worst case (all followers also busy) each follower is contacted once before the leader
is restored. Applies to reads and writes alike — a write rejected by a follower gets
the same NotLeader treatment.
What it does NOT cover
The window where the wedged store is still the leader (before PD eviction): the hint
points back to the same store and behavior degrades to status quo; that window is
covered by PD slow-store eviction. Complementary to the server-side fix
tikv/tikv#19932 (return NotLeader at the rejection gate): that one heals old clients
on new TiKV, this one heals old TiKV on new clients.
The forwarding (proxy) path is out of scope:
EnableForwardingdefaults to false, andthis change is designed and tested for the default selection path only — its
interaction with
ReplicaSelectLeaderWithProxyStrategyis not covered.A compound failure — wedged old leader AND all followers also rejecting with
ServerIsBusy(0) AND PD has already moved the leader — is intentionally not cured
eagerly: the probe cannot obtain a hint, and invalidating/reloading would tax PD in
the (much more common) case where the cache is actually correct. The request falls
back to today's behavior; the cure arrives once any follower can answer again.
Tests
TestReplicaSelectorLeaderBusyProbe: no probe on a single busy; probe on the 2nd busywith leader-read semantics kept; cache healed via hint and a new selector goes
straight to the new leader; misjudgment restores the leader with no further probing;
no-hint goes through the region-scheduling backoff path; stale/follower/leaderOnly
reads never probe; a single-replica region (no probe target) restores the leader
without invalidating; the busy count restarts when the cached leader changes.
Two existing access-path tests were updated: their expectations encoded the old
behavior this PR removes (leader hammered until exhausted before trying a follower).
The all-busy meltdown case now ends with the leader restored — twelve accesses ending
in success with the region cache intact, essentially the same retry shape as before
this PR, with only one early fruitless probe of each follower.
Summary by CodeRabbit