fix(spanner): prevent fastpath tablet routing flaps - #20417
Conversation
Ignore whole group updates from older generations. At equal generation, retain unavailable tablet freshness baselines until a newer incarnation permits recovery. Refuse endpoint recreation for addresses absent from active finder membership, and serialize endpoint publication reconciliation with active-address removal.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces active address tracking in the endpointLifecycleManager and synchronizes it with the keyRangeCache to prevent routing flaps and ensure stale or removed endpoints are not incorrectly recreated. It also refactors tablet updates within cachedGroup to preserve proven addresses or skip states until incarnation advances. The review feedback highlights a subtle bug in this flap prevention logic where a stale update with an older incarnation could erroneously clear the skip state of a tablet that is currently skipped at a newer incarnation, and provides a code suggestion to address this issue.
…escalation Track overload and unavailable failures in independent capped escalation lanes, honor server RetryInfo for overload cooldowns, and repair tiers after successful routed calls. Reserve bounded probes after server-hinted delays while preserving fallback behavior for unhinted or unavailable cooldowns.
Track cooldown entries atomically so successful routed calls bypass clock and mutex work when no endpoint needs repair. Keep the counter synchronized with every map insertion and deletion, including repair, idle cleanup, and pruning.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request enhances the Spanner client's location-aware routing and endpoint cooldown tracking by introducing independent failure lanes for overload and availability issues, success-based repair mechanisms, and better active address synchronization. The review feedback is highly constructive, pointing out a sorting bug in selectCoolingDownTabletLocked that fails to compare selection costs when no failures are present, suggesting the use of status.Code(err) to simplify gRPC status checks, and recommending the removal of redundant nil-guards where non-nil invariants are guaranteed.
| sort.SliceStable(candidates, func(i, j int) bool { | ||
| firstFailure := cooldowns.lastOverloadFailure(candidates[i].tablet.serverAddress) | ||
| secondFailure := cooldowns.lastOverloadFailure(candidates[j].tablet.serverAddress) | ||
| if firstFailure.IsZero() { | ||
| return false | ||
| } | ||
| if secondFailure.IsZero() { | ||
| return true | ||
| } | ||
| if !firstFailure.Equal(secondFailure) { | ||
| return firstFailure.Before(secondFailure) | ||
| } | ||
| return candidates[i].selectionCost < candidates[j].selectionCost | ||
| }) |
There was a problem hiding this comment.
In the sort.SliceStable comparator, if both firstFailure and secondFailure are zero (meaning both replicas are unavailable but have no overload failures), the first condition if firstFailure.IsZero() will immediately return false. This prevents the comparator from falling back to comparing their selectionCost, which means their relative order will not be correctly determined by their selection costs.
To fix this, we should explicitly handle the case where both failures are zero by comparing their selection costs first.
sort.SliceStable(candidates, func(i, j int) bool {
firstFailure := cooldowns.lastOverloadFailure(candidates[i].tablet.serverAddress)
secondFailure := cooldowns.lastOverloadFailure(candidates[j].tablet.serverAddress)
if firstFailure.IsZero() && secondFailure.IsZero() {
return candidates[i].selectionCost < candidates[j].selectionCost
}
if firstFailure.IsZero() {
return false
}
if secondFailure.IsZero() {
return true
}
if !firstFailure.Equal(secondFailure) {
return firstFailure.Before(secondFailure)
}
return candidates[i].selectionCost < candidates[j].selectionCost
})| var grpcStatus *status.Status | ||
| var spannerErr *Error | ||
| if errors.As(err, &spannerErr) { | ||
| grpcStatus = status.Convert(spannerErr.Unwrap()) | ||
| } else { | ||
| grpcStatus = status.Convert(err) | ||
| } |
There was a problem hiding this comment.
Avoid implementing custom errors.As helpers to check for gRPC status codes. In Go, grpc-go's status.FromError natively walks the error chain using errors.As to inspect the GRPCStatus interface, meaning it already handles wrapped errors. We can simplify this by calling status.Code(err) directly to check the gRPC status code.
| var grpcStatus *status.Status | |
| var spannerErr *Error | |
| if errors.As(err, &spannerErr) { | |
| grpcStatus = status.Convert(spannerErr.Unwrap()) | |
| } else { | |
| grpcStatus = status.Convert(err) | |
| } | |
| statusCode := status.Code(err) |
References
- When working with gRPC in Go, use
status.Code(err)directly to check for gRPC status codes instead of implementing customerrors.Ashelpers.grpc-go'sstatus.FromErrornatively walks the error chain usingerrors.Asto inspect theGRPCStatusinterface, meaning it already handles wrapped errors (e.g., wrapped with%w).
| func (c *locationAwareSpannerClient) recordEndpointSuccess(ep channelEndpoint) { | ||
| if c == nil || ep == nil || ep.Address() == c.defaultEndpointAddress || c.endpointCooldowns == nil { | ||
| return | ||
| } | ||
| c.endpointCooldowns.recordSuccess(ep.Address()) | ||
| } |
There was a problem hiding this comment.
Do not add defensive nil-guards on variables or parameters where invariants guaranteed by construction or call sites ensure they will never be nil. At all call sites of recordEndpointSuccess, the receiver c is guaranteed to be non-nil, and ep is guaranteed to be non-nil because the calls are guarded by !usedDefaultEndpoint (which is only false when ep is non-nil).
| func (c *locationAwareSpannerClient) recordEndpointSuccess(ep channelEndpoint) { | |
| if c == nil || ep == nil || ep.Address() == c.defaultEndpointAddress || c.endpointCooldowns == nil { | |
| return | |
| } | |
| c.endpointCooldowns.recordSuccess(ep.Address()) | |
| } | |
| func (c *locationAwareSpannerClient) recordEndpointSuccess(ep channelEndpoint) { | |
| if c.endpointCooldowns == nil || ep.Address() == c.defaultEndpointAddress { | |
| return | |
| } | |
| c.endpointCooldowns.recordSuccess(ep.Address()) | |
| } |
References
- Do not add defensive nil-guards on variables or parameters where invariants guaranteed by construction or call sites ensure they will never be nil. Such guards result in dead code and can mask real bugs by silently no-oping instead of allowing a nil-panic to immediately surface the caller's mistake.
| func (m *endpointLifecycleManager) updateActiveAddresses(addresses map[string]struct{}) { | ||
| if m == nil { | ||
| return | ||
| } | ||
| if m.testHooks != nil && m.testHooks.beforeRemovalLock != nil { | ||
| m.testHooks.beforeRemovalLock() | ||
| } |
There was a problem hiding this comment.
Do not add defensive nil-guards on variables or parameters where invariants guaranteed by construction or call sites ensure they will never be nil. The receiver m is guaranteed to be non-nil at all call sites of updateActiveAddresses because they are explicitly guarded by if lifecycleManager != nil or are in tests where the manager is initialized.
| func (m *endpointLifecycleManager) updateActiveAddresses(addresses map[string]struct{}) { | |
| if m == nil { | |
| return | |
| } | |
| if m.testHooks != nil && m.testHooks.beforeRemovalLock != nil { | |
| m.testHooks.beforeRemovalLock() | |
| } | |
| func (m *endpointLifecycleManager) updateActiveAddresses(addresses map[string]struct{}) { | |
| if m.testHooks != nil && m.testHooks.beforeRemovalLock != nil { | |
| m.testHooks.beforeRemovalLock() | |
| } |
References
- Do not add defensive nil-guards on variables or parameters where invariants guaranteed by construction or call sites ensure they will never be nil. Such guards result in dead code and can mask real bugs by silently no-oping instead of allowing a nil-panic to immediately surface the caller's mistake.
…ards Use selection cost when cooling candidates lack overload timestamps, and remove redundant nil guards from call paths that already guarantee initialized receivers and endpoints. Preserve explicit Spanner error unwrapping because direct gRPC status conversion drops RetryInfo details.
b7436c6 to
38d13b6
Compare
1 participant
Ignore whole group updates from older generations. At equal generation, retain unavailable tablet freshness baselines until a newer incarnation permits recovery.
Refuse endpoint recreation for addresses absent from active finder membership, and serialize endpoint publication reconciliation with active-address removal.
Related Java PR googleapis/google-cloud-java#13803