Drop query sessions when balancer bans a node#2208
Conversation
Pessimize balancer connections when session-create RPCs return UNAVAILABLE or context.DeadlineExceeded, so client traffic avoids blackholed nodes faster. Co-authored-by: Cursor <cursoragent@cursor.com>
Purge idle pool sessions by NodeID on conn ban, mark in-use sessions for removal until discovery unbans the node, and pessimize on attach-stream errors. Co-authored-by: Cursor <cursoragent@cursor.com>
github.com/ydb-platform/ydb-go-sdk/v3/configcompatible changes(*Config).NotifyConnAllow: added summaryInferred base version: v3.141.0 |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## ban-session-create-unreachable-nodes #2208 +/- ##
=========================================================================
+ Coverage 45.32% 57.71% +12.39%
=========================================================================
Files 440 418 -22
Lines 45321 43155 -2166
=========================================================================
+ Hits 20543 24909 +4366
+ Misses 23722 16033 -7689
- Partials 1056 2213 +1157
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
asmyasnikov
added
no changelog
SLO
no tests
Drop banned in-use sessions in checkItemAndError instead of getItem to avoid double IsAlive checks; fix struct formatting and add changelog entry. Co-authored-by: Cursor <cursoragent@cursor.com>
![robot-vibe-db[bot]](https://avatars.githubusercontent.com/in/3319454?s=60&v=4){kind=small}
There was a problem hiding this comment.
AI Review Summary
Verdict: ❌ 0 critical issues found, 2 major issues found
Critical issues
No critical issues found.
Other findings
- Major | Medium: Data race on
config.onConnBan/onConnAllow— unsynchronized concurrent read/write betweenconfig.With()andNotifyConnBan/NotifyConnAllow—driver.go:507,config/config.go:339 - Major | Medium:
onNodeShutdownfires on allRecv()errors (including context cancellation from normal session close), not just transport errors as described in the PR —internal/query/session_core.go:264 - Minor | Medium: Stale
bannedNodesentries ifOnConnAllowedis never called (e.g., node permanently removed) —internal/query/client.go:839 - Minor | Medium: Test coverage for
DropIdleByNodeIDand banned-node flow does not exercise concurrent access or mixed node IDs —internal/pool/pool_test.go:2571,internal/query/client_banned_node_test.go:9 - Nit | High:
bannedNodesfield has inconsistent indentation (one tab instead of two) —internal/query/client.go:63
This review was generated automatically. Critical issues require attention; other findings are advisory.
If this comment was useful, please give it a 👍 — it helps us improve the review bot.
|
Analysis performed by claude, claude-opus-4-6. |
Register conn ban callbacks before pool start, skip attach-stream ban on session close, and extend pool tests for mixed node IDs and banned return. Co-authored-by: Cursor <cursoragent@cursor.com>
…ns-on-banned-node
|
Summary reply to AI review (c346f75):
|
There was a problem hiding this comment.
AI Review Summary
Verdict: ✅ No critical issues found
Critical issues
No critical issues found.
Other findings
- Major | Medium:
checkItemAndErrorreordering changes the behavioral contract ofmustDeleteItemFunc— it is now invoked whenerr == nil, which was previously short-circuited. All current callers handle this correctly, but the change is subtle and undocumented —pool.go:409 - Minor | High:
dropIdleSessionsByNodeIDrelies on type assertions to*pool.Pool[*Session, Session]rather than an interface method; if the pool implementation changes, idle-session dropping silently stops working —internal/query/client.go:864 - Minor | Medium: Errors from
DropIdleByNodeIDare silently discarded; consider tracing/logging for observability —internal/query/client.go:865 - Nit | High:
config.With()godoc says "makes copy of current Config" but actually mutates the receiver in place; the PR relies on this mutation-in-place behavior atdriver.go:461(no return-value assignment). Consider updating the comment to reflect the actual semantics —config/config.go:393
This review was generated automatically. Critical issues require attention; other findings are advisory.
If this comment was useful, please give it a 👍 — it helps us improve the review bot.
| return nil | ||
| } | ||
|
|
||
| if p.config.mustDeleteItemFunc(item, err) { |
There was a problem hiding this comment.
Severity: Major
Confidence: Medium
The reordering of checkItemAndError changes the contract of mustDeleteItemFunc: it is now called even when err == nil, which was previously short-circuited by the early return nil. This means every pool user (including the table service pool at internal/table/client.go:48) now has its mustDeleteItemFunc evaluated on every successful operation, not just on errors.
All current callers handle err == nil correctly (table: err != nil && ... → false; default: !item.IsAlive() → unaffected). However, this is a semantic change to a shared generic component that could surprise future implementors who assume mustDeleteItemFunc is only called on error paths.
Consider documenting this contract change in the function comment or the WithMustDeleteItemFunc godoc.
| } | ||
|
|
||
| func (c *Client) dropIdleSessionsByNodeID(ctx context.Context, nodeID uint32) { | ||
| if p, ok := c.explicitSessionPool.(*pool.Pool[*Session, Session]); ok { |
There was a problem hiding this comment.
Severity: Minor
Confidence: High
dropIdleSessionsByNodeID uses type assertions (c.explicitSessionPool.(*pool.Pool[*Session, Session])) rather than relying on an interface method. If sessionPool is ever implemented by a different type (e.g., a mock or wrapper), the assertion silently returns false and idle sessions on banned nodes are never dropped.
Consider either:
- Adding
DropIdleByNodeID(ctx context.Context, nodeID uint32) (int, error)to thesessionPoolinterface, or - Adding a comment explaining this is intentional and why
|
|
||
| func (c *Client) dropIdleSessionsByNodeID(ctx context.Context, nodeID uint32) { | ||
| if p, ok := c.explicitSessionPool.(*pool.Pool[*Session, Session]); ok { | ||
| _, _ = p.DropIdleByNodeID(ctx, nodeID) |
There was a problem hiding this comment.
Severity: Minor
Confidence: Medium
Both DropIdleByNodeID calls discard the error with _, _. While the callback has no error-return path, silently swallowing errors (e.g., context cancellation, closed pool) makes debugging harder in production.
Consider at least tracing the error:
if _, err := p.DropIdleByNodeID(ctx, nodeID); err != nil {
// trace or log
}
|
Analysis performed by claude, claude-opus-4-6. |
asmyasnikov
changed the base branch from
ban-session-create-unreachable-nodes
to
master
2 participants
Summary
NodeIDwhen the driver pessimizes a connectionMotivation
After
BanOnSessionCreate,native-querystill spikes retries during ip-blackhole because long-lived sessions in the pool keep serving requests until they fail through many retry attempts. Purging sessions on ban avoids reusing sessions bound to a dead node.Test plan
go test ./internal/pool/... -run DropIdlego test ./internal/query/... -run BannedNodeMade with Cursor