autoid: fail fast after repeated not leader responses from an unchanged leader#69961
autoid: fail fast after repeated not leader responses from an unchanged leader#69961OliverS929 wants to merge 2 commits into
Conversation
ti-chi-bot
Bot
added
the
release-note
|
[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 |
ti-chi-bot
Bot
added
the
size/XXL
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAutoID client discovery now tracks leader snapshots and uses structured not-leader retry handling with bounded failure behavior, cancellation checks, connection resets, and request logging. New integration tests cover leader changes, retries, cancellation, concurrency, and recovery. ChangesAutoID not-leader handling
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Request
participant AutoIDClient
participant ClientDiscover
participant Etcd
participant AutoIDService
Request->>AutoIDClient: Alloc or rebase
AutoIDClient->>ClientDiscover: discover client and leader
ClientDiscover->>Etcd: read leader metadata
Etcd-->>ClientDiscover: leader snapshot
ClientDiscover-->>AutoIDClient: gRPC client and snapshot
AutoIDClient->>AutoIDService: issue request
AutoIDService-->>AutoIDClient: success or not-leader response
AutoIDClient->>ClientDiscover: reset and retry
AutoIDClient-->>Request: result or terminal error
Possibly related PRs
Suggested labels: Suggested reviewers: Poem
🚥 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 |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #69961 +/- ##
================================================
- Coverage 76.3192% 73.9286% -2.3906%
================================================
Files 2041 2058 +17
Lines 559918 579169 +19251
================================================
+ Hits 427325 428172 +847
- Misses 131692 150632 +18940
+ Partials 901 365 -536
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
|
🔍 Starting code review for this PR... |
ingress-bot
left a comment
ingress-bot
left a comment
There was a problem hiding this comment.
This review was generated by AI and should be verified by a human reviewer.
Manual follow-up is recommended before merge.
Summary
- Total findings: 11
- Inline comments: 6
- Summary-only findings (no inline anchor): 5
Findings (highest risk first)
🟡 [Minor] (10)
- CAS invariant comment for
versiondropped during refactor (pkg/meta/autoid/autoid_service.go:65, pkg/meta/autoid/autoid_service.go:517) - Exact-match wire contract in
isAutoIDNotLeaderErrorhas no comment explaining why it must be exact (pkg/meta/autoid/autoid_service.go:122) - Not-leader log line's field set is hand-duplicated across three methods (pkg/meta/autoid/autoid_service.go:163, pkg/meta/autoid/autoid_service.go:193, pkg/meta/autoid/autoid_service.go:213)
- Not-leader retry orchestration is duplicated between
Allocandrebase(pkg/meta/autoid/autoid_service.go:433, pkg/meta/autoid/autoid_service.go:621) handleNotLeaderError's tri-state return contract is undocumented despite two duplicated call sites (pkg/meta/autoid/autoid_service.go:261, pkg/meta/autoid/autoid_service.go:442, pkg/meta/autoid/autoid_service.go:630)autoIDLeaderSnapshot.equalcompares three fields with no comment on why address alone is insufficient (pkg/meta/autoid/autoid_service.go:82, pkg/meta/autoid/autoid_service.go:88)- Current leader address is tracked in two independently-updated copies (pkg/meta/autoid/autoid_service.go:175, pkg/meta/autoid/autoid_service.go:220, pkg/meta/autoid/autoid_service.go:288)
- Exported
ClientDiscover.GetClientis now dead code after the retry rewrite (pkg/meta/autoid/autoid_service.go:317, pkg/meta/autoid/autoid_service.go:322) - Production time-based fast-fail window (minDuration=15s) is never exercised through the real Alloc/rebase path (pkg/meta/autoid/autoid_service_test.go:151, pkg/meta/autoid/autoid_service.go:278, pkg/meta/autoid/autoid_service.go:78)
- Test cleanup uses a fixed sleep to race the background gRPC-close goroutine (pkg/meta/autoid/autoid_service_test.go:711)
ℹ️ [Info] (1)
- New fast-fail changes autoid Alloc/Rebase from retry-until-deadline to erroring after ~15s, with no release note or rollout gate (pkg/meta/autoid/autoid_service.go:247, pkg/meta/autoid/autoid_service.go:78)
Unanchored findings
🟡 [Minor] (5)
- CAS invariant comment for
versiondropped during refactor- Request: Restore a short comment on the
versionfield (or onresetConn/resetConnLocked) explaining that it is bumped on every reset so concurrent callers holding a stale version don't trigger redundant resets.
- Request: Restore a short comment on the
- Exact-match wire contract in
isAutoIDNotLeaderErrorhas no comment explaining why it must be exact- Request: Add a short comment above
isAutoIDNotLeaderErrorstating that the exact code+message pairing mirrors a specific server-side error literal, and note where that literal is produced server-side (or link to it) so the two sides are kept in sync.
- Request: Add a short comment above
- Not-leader log line's field set is hand-duplicated across three methods
- Request: Extract a helper (e.g.
(s *autoIDRequestLogState) baseFields() []zap.Field) that returns the shared prefix, and have all three log call sites append only their unique fields on top of it.
- Request: Extract a helper (e.g.
- Not-leader retry orchestration is duplicated between
Allocandrebase- Request: Factor the shared decision-and-retry orchestration (the part after
handleNotLeaderErrorreturns) into one local helper both call sites invoke, e.g. a closure that performs the backoff/goto-retry-equivalent loop and returns only the terminal error, soAllocandrebaseeach just map that single error into their own return signature.
- Request: Factor the shared decision-and-retry orchestration (the part after
handleNotLeaderError's tri-state return contract is undocumented despite two duplicated call sites- Request: Add a doc comment on
handleNotLeaderErrorstating what each return combination means (e.g.handled=falsemeans not a not-leader error;terminalErr != nilmeans the caller must return it;retryImmediatelyvs backoff-then-retry), so the duplicated call sites stay consistent.
- Request: Add a doc comment on
| createRevision int64 | ||
| } | ||
|
|
||
| func (s autoIDLeaderSnapshot) equal(other autoIDLeaderSnapshot) bool { |
There was a problem hiding this comment.
🟡 [Minor] autoIDLeaderSnapshot.equal compares three fields with no comment on why address alone is insufficient
Impact
equal drives notLeaderRetryState.observe's decision to reset vs. continue the not-leader streak, which in turn decides whether a request fast-fails after defaultNotLeaderMinConsecutive responses or gets a fresh count.
The method deliberately compares electionKey and createRevision in addition to address so that a new leadership campaign that happens to reuse the same address is treated as a different leader (per the "same address with a new campaign gets a fresh attempt" test case), but nothing in the code states that rationale.
A maintainer who later "simplifies" this to an address-only comparison (a natural-looking cleanup) would silently make the fast-fail threshold count across unrelated leader terms, changing when clients give up.
Scope
pkg/meta/autoid/autoid_service.go:82—autoIDLeaderSnapshotpkg/meta/autoid/autoid_service.go:88—autoIDLeaderSnapshot.equal
Evidence
equal (autoid_service.go:88-92) compares address, electionKey, and createRevision; autoid_service_test.go:797-812 ("leader generation changes restart the sequence") is the only place documenting that an election-key or create-revision change, even with the same address, must restart the not-leader count.
Change request
Add a comment on autoIDLeaderSnapshot or equal explaining that electionKey/createRevision are included so a re-elected leader reusing the same address is not conflated with the prior stale leader's streak.
| zap.Uint32("keyspace-id", s.keyspaceID), | ||
| zap.Int64("db-id", s.dbID), | ||
| zap.Int64("table-id", s.tableID), | ||
| zap.String("leader-address", state.leader.address), |
There was a problem hiding this comment.
🟡 [Minor] Current leader address is tracked in two independently-updated copies
Impact
autoIDRequestLogState.leaderAddress and notLeaderRetryState.leader.address both represent "the leader endpoint this request is currently dealing with", but they are written by separate code paths: observeNotLeader/setLeader update s.leaderAddress, while notLeaderRetryState.observe/reset update state.leader independently.
fastFail reads from state.leader.address (line 220) instead of the already-populated s.leaderAddress that complete and observeNotLeader use, so the two fields only agree today because handleNotLeaderError happens to call requestLog.setLeader(currentLeader) immediately before the state.leader.equal(currentLeader) check that gates the fastFail call.
Scope
pkg/meta/autoid/autoid_service.go:175—autoIDRequestLogState.setLeaderpkg/meta/autoid/autoid_service.go:220—autoIDRequestLogState.fastFailpkg/meta/autoid/autoid_service.go:288—singlePointAlloc.handleNotLeaderError
Evidence
In handleNotLeaderError, requestLog.setLeader(currentLeader) (line 288) is the only reason s.leaderAddress matches state.leader.address by the time requestLog.fastFail(*state, ...) runs on line 297; fastFail itself never reads s.leaderAddress.
Change request
Have fastFail read s.leaderAddress like the other two log methods, or drop the redundant leaderAddress field on autoIDRequestLogState and source it from notLeaderRetryState everywhere, so there is a single owner for the current leader address instead of two copies kept aligned only by call order.
| @@ -84,18 +315,28 @@ func GetAutoIDServiceLeaderEtcdPath(keyspaceID uint32) string { | |||
|
|
|||
| // GetClient gets the AutoIDAllocClient. | |||
| func (d *ClientDiscover) GetClient(ctx context.Context, keyspaceID uint32) (autoid.AutoIDAllocClient, uint64, error) { | |||
There was a problem hiding this comment.
🟡 [Minor] Exported ClientDiscover.GetClient is now dead code after the retry rewrite
Impact
Alloc and rebase used to be GetClient's only callers; this PR switches both of them to call the new unexported getClientWithLeader directly so they can also see the leader snapshot.
That leaves the exported GetClient wrapper with zero callers anywhere in the repository, so it now exists only as an unused, narrower view (it silently drops the leader address) of the real implementation.
Scope
pkg/meta/autoid/autoid_service.go:317—GetClientpkg/meta/autoid/autoid_service.go:322—getClientWithLeader
Evidence
A repo-wide search for calls to ClientDiscover.GetClient (.GetClient(ctx) matches only the method's own declaration at line 317; Alloc (line 412) and rebase (line 604) both call getClientWithLeader instead.
Change request
Remove the now-unused GetClient wrapper (or unexport/inline it) unless there is a concrete external caller that still needs the narrower 3-tuple signature; if one exists, note it in a comment so the wrapper isn't mistaken for dead code later.
| keyspaceID: uint32(tikv.NullspaceID), | ||
| notLeaderPolicy: notLeaderRetryPolicy{ | ||
| minConsecutive: 3, | ||
| minDuration: 0, |
There was a problem hiding this comment.
🟡 [Minor] Production time-based fast-fail window (minDuration=15s) is never exercised through the real Alloc/rebase path
Impact
Every end-to-end not-leader test overrides the policy to minDuration: 0, so the fast-fail is validated purely on the consecutive-count gate.
The dominant production behavior (keep retrying until roughly 15s elapse, then return the terminal error) is only covered by the isolated observe unit test with injected time, so a regression that mis-wires or drops the duration component in the real request path would still pass all the integration tests.
Scope
pkg/meta/autoid/autoid_service_test.go:151—newNotLeaderTestAllocatorpkg/meta/autoid/autoid_service.go:278—handleNotLeaderErrorpkg/meta/autoid/autoid_service.go:78—defaultNotLeaderMinDuration
Evidence
newNotLeaderTestAllocator hardcodes notLeaderPolicy{minConsecutive: 3, minDuration: 0}, so state.observe's now.Sub(firstSeen) >= policy.minDuration is trivially true in handleNotLeaderError. With the real default (defaultNotLeaderMinConsecutive=10, defaultNotLeaderMinDuration=15s) the 15s duration is the binding constraint, yet no test drives Alloc/rebase with a non-zero minDuration, and handleNotLeaderError reads time.Now() directly with no clock seam.
Change request
Add a focused test that drives Alloc/rebase with a small but non-zero minDuration (e.g. tens of milliseconds) and asserts the request keeps retrying until that window elapses before fast-failing, so the real-path duration gate is regression-covered rather than only the pure observe helper.
| defer cancel() | ||
| err := runAutoIDTestOperation(ctx, operation, allocator) | ||
| require.EqualError(t, err, "service rejected request") | ||
| require.Equal(t, int64(2), autoIDOperationCallCount(operation, service)) |
There was a problem hiding this comment.
🟡 [Minor] Test cleanup uses a fixed sleep to race the background gRPC-close goroutine
Impact
The cleanup relies on wall-clock timing to let the 200ms background connection-close goroutine finish before the package goleak check runs.
Under a loaded CI shard, scheduling latency can push that goroutine past the 250ms window, producing an intermittent leaked-goroutine failure that is unrelated to the code under test.
Scope
pkg/meta/autoid/autoid_service_test.go:711—newNotLeaderTestAllocator
Evidence
newNotLeaderTestAllocator cleanup does allocator.ResetConn(nil) then time.Sleep(250 * time.Millisecond), banking on the afterReset goroutine that sleeps 200 * time.Millisecond before grpcConn.Close(). This is the sleep-based-synchronization anti-pattern the determinism checklist calls out.
Change request
Replace the fixed sleep with a deterministic wait (e.g. poll an observable closed condition via require.Eventually, or make the close synchronous in test paths), or explicitly document that this leans on goleak's own retry budget rather than the 250ms constant.
| ) error { | ||
| elapsed := now.Sub(state.firstSeen) | ||
| // TODO: Consider enriching terminal errors with the remote TiDB identity from /info. | ||
| return ErrAutoincReadFailed.GenWithStack(fmt.Sprintf( |
There was a problem hiding this comment.
ℹ️ [Info] New fast-fail changes autoid Alloc/Rebase from retry-until-deadline to erroring after ~15s, with no release note or rollout gate
On a fully upgraded cluster, an autoid endpoint that keeps answering "not leader" now makes Alloc/Rebase return ErrAutoincReadFailed after roughly 15s instead of retrying until the caller's context deadline, so DML that previously blocked-and-retried can now surface an error.
The behavior is not behind any flag or config and the diff carries no release note or runbook entry describing the new terminal error, so operators mid-rollout have no documented signal or opt-out.
2 participants
What problem does this PR solve?
Issue Number: close #69690
Problem Summary:
When an AutoID client repeatedly reaches the same leader endpoint but receives
not leader, the existing retry path can keep resetting the connection and rediscovering the same winning candidate. Requests that need AutoIDs can remain blocked without an actionable terminal error.What changed and how does it work?
Each AutoID allocation or rebase request now tracks consecutive exact gRPC
not leaderresponses from the same leader snapshot. The snapshot includes the endpoint, election key, and winning candidate's create revision.codes.Unknownwith the exact messagenot leadercontributes to the threshold.An operation that observes
not leaderemits at most one start record and one completion record. The terminal warning is itself the completion record for a fast-failed request. Normal requests add no new feature logs.This change does not modify election state, revoke leases, remove candidates, or automatically select another leader.
Check List
Tests
Side effects
Documentation
Release note
Please refer to Release Notes Language Style Guide to write a quality release note.
Summary by CodeRabbit