internal/locate: probe follower once when the cached leader keeps rejecting leader reads with ServerIsBusy(0) - #2041
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>
ti-chi-bot
Bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
ti-chi-bot
Bot
added
the
dco-signoff: yes
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe replica selector tracks repeated zero-wait ChangesLeader busy probe
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant ReplicaSelector
participant CachedLeader
participant Follower
participant RegionCache
ReplicaSelector->>CachedLeader: Send leader read
CachedLeader-->>ReplicaSelector: Return ServerIsBusy(0)
ReplicaSelector->>CachedLeader: Send second leader read
CachedLeader-->>ReplicaSelector: Return ServerIsBusy(0)
ReplicaSelector->>Follower: Send follower probe
Follower-->>ReplicaSelector: Return NotLeader hint
ReplicaSelector->>RegionCache: Update leader information
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
ti-chi-bot
Bot
added
the
size/XL
ti-chi-bot
Bot
removed
the
do-not-merge/work-in-progress
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/locate/replica_selector.go (1)
250-259: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winExclude suspect-leader routing in the proxy strategy.
ReplicaSelectLeaderWithProxyStrategy.nextreturns the cached leader astargetwhen the leader’s store is reachable, and it can also return the cached leader with any reachable follower asproxy. The subsequent caller then falls through toReplicaSelectLeaderStrategyonly if the proxy strategy does not return both a target and a proxy, sosuspectNotLeaderFlagis ignored in the forwarding path. Apply the same suspect-leader check before returningleader, proxyso the probe still occurs when forwarding is enabled.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/locate/replica_selector.go` around lines 250 - 259, The proxy forwarding path must also exclude leaders marked with suspectNotLeaderFlag. In ReplicaSelectLeaderWithProxyStrategy.next, apply the same isLeaderCandidate(leader) and !leader.hasFlag(suspectNotLeaderFlag) validation before returning the cached leader with a reachable follower as proxy, allowing fallback to ReplicaSelectLeaderStrategy for probing.
🧹 Nitpick comments (1)
internal/locate/replica_selector.go (1)
594-612: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAlign the wording with the counter semantics, and name the threshold.
leaderBusyCountis never reset. A non-busy response between twoServerIsBusy(0)responses does not clear it, so the counter is cumulative per selector, not consecutive. The comment here, the field comment at lines 43-47, and the test comments all state "consecutive". Fix the wording, or reset the counter when the cached leader returns anything other thanServerIsBusy(0). Also extract the literal2into a named constant.♻️ Proposed wording and constant change
- // half-dead store indefinitely. After 2 consecutive such rejections on the cached + // half-dead store indefinitely. After maxLeaderBusyBeforeProbe such rejections on the cached // leader, mark it suspect-not-leader so that the next attempt probes a follower // with the leader read (req.ReplicaRead is kept unchanged). The follower replies // NotLeader with the real leader hint, which heals the shared region cache via // onNotLeader/updateLeader. Probe 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. if s.replicaReadType == kv.ReplicaReadLeader && !s.isStaleRead && !s.option.leaderOnly && s.target != nil && s.target.peer.Id == s.region.GetLeaderPeerID() && !s.leaderBusyProbed { s.leaderBusyCount++ - if s.leaderBusyCount >= 2 { + if s.leaderBusyCount >= maxLeaderBusyBeforeProbe { s.target.addFlag(suspectNotLeaderFlag) s.leaderBusyProbed = true } }Declare the constant near the selector types:
// maxLeaderBusyBeforeProbe is the number of ServerIsBusy(0) rejections on the cached // leader that triggers a single follower probe per selector. const maxLeaderBusyBeforeProbe = 2🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/locate/replica_selector.go` around lines 594 - 612, Update the leader-busy handling around leaderBusyCount to match its cumulative semantics by removing “consecutive” wording from the associated comments and tests, and replace the literal threshold 2 with a named maxLeaderBusyBeforeProbe constant declared near the selector types. Keep the existing probe behavior unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@internal/locate/replica_selector.go`:
- Around line 250-259: The proxy forwarding path must also exclude leaders
marked with suspectNotLeaderFlag. In ReplicaSelectLeaderWithProxyStrategy.next,
apply the same isLeaderCandidate(leader) and
!leader.hasFlag(suspectNotLeaderFlag) validation before returning the cached
leader with a reachable follower as proxy, allowing fallback to
ReplicaSelectLeaderStrategy for probing.
---
Nitpick comments:
In `@internal/locate/replica_selector.go`:
- Around line 594-612: Update the leader-busy handling around leaderBusyCount to
match its cumulative semantics by removing “consecutive” wording from the
associated comments and tests, and replace the literal threshold 2 with a named
maxLeaderBusyBeforeProbe constant declared near the selector types. Keep the
existing probe behavior unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 193f308f-365e-45d6-9754-fd7a42646564
📒 Files selected for processing (3)
internal/locate/region_request.gointernal/locate/replica_selector.gointernal/locate/replica_selector_test.go
There was a problem hiding this comment.
Pull request overview
This PR introduces a targeted workaround in the TiKV region replica selector to avoid repeatedly retrying a cached leader that keeps rejecting leader reads with ServerIsBusy where EstimatedWaitMs == 0, by probing a follower once to obtain a NotLeader hint and heal the shared region cache (#2028).
Changes:
- Add per-selector tracking to detect repeated
ServerIsBusy(0)on the cached leader and trigger a one-time follower probe via a newsuspectNotLeaderFlag. - Update leader-selection logic to skip a leader replica flagged as suspect so the mixed strategy can choose a follower for the probe.
- Extend and adjust tests to cover the new probe behavior and update expectations for access-path behavior.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| internal/locate/replica_selector.go | Adds per-selector busy tracking and skips suspect leader to trigger follower probing after repeated ServerIsBusy(0). |
| internal/locate/replica_selector_test.go | Updates existing access-path expectations and adds a dedicated test covering the new busy-probe behavior. |
| internal/locate/region_request.go | Adds suspectNotLeaderFlag and clears it when leader is updated via a NotLeader hint. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if s.replicaReadType == kv.ReplicaReadLeader && !s.isStaleRead && !s.option.leaderOnly && | ||
| s.target != nil && s.target.peer.Id == s.region.GetLeaderPeerID() && !s.leaderBusyProbed { | ||
| s.leaderBusyCount++ | ||
| if s.leaderBusyCount >= 2 { | ||
| s.target.addFlag(suspectNotLeaderFlag) |
|
/retest |
1 similar comment
|
/retest |
…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>
| s.leaderBusyCount = 0 | ||
| } | ||
| s.leaderBusyCount++ | ||
| if s.leaderBusyCount >= 2 { |
There was a problem hiding this comment.
What is the impact for case the leader is busy for real? Backoff would be skipped for some times?
There was a problem hiding this comment.
It won't be skipped. The sequence would be leader: busy -> leader: busy -> follower: not leader(leader is the original one) -> leader... The test case at line 2585 covered such situation
ti-chi-bot
Bot
added
needs-1-more-lgtm
…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>
| } else { | ||
| // Mark the server is busy (the next incoming READs could be redirected to expected followers.) | ||
| ctx.Store.healthStatus.markAlreadySlow() | ||
| // Workaround for tikv/client-go#2028: if the store's read pool is wedged, leader |
There was a problem hiding this comment.
ServerIsBusy.estimated_wait_ms = 0 seems must not be interpreted as “the store is healthy” or “expected wait time is zero.” in this case, could we add more comments for ServerIsBusy.estimated_wait_ms = 0
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: AndreMouche, cfzjywxk 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 |
[LGTM Timeline notifier]Timeline:
|
|
/cherry-pick tidb-8.5 |
|
@ekexium: new pull request created to branch DetailsIn 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 ti-community-infra/tichi repository. |
…ecting leader reads with ServerIsBusy(0) (#2041) (#2044) close #2028, fix tikv/tikv#19932\n\nSigned-off-by: Ziqian Qin <eke@fastmail.com>\n\nCo-authored-by: Ziqian Qin <eke@fastmail.com>
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