github-actions[bot] commented on code in PR #66437:
URL: https://github.com/apache/doris/pull/66437#discussion_r3711937938
##########
fe/fe-core/src/main/java/org/apache/doris/service/arrowflight/sessions/FlightSqlConnectPoolMgr.java:
##########
@@ -55,10 +56,16 @@ public int registerConnection(ConnectContext ctx) {
@Override
public void unregisterConnection(ConnectContext ctx) {
+ // All Flight SQL session teardown paths (idle/query timeout, bearer
token expiry, and
+ // explicit CloseSession) reach here. Release channel-cached Arrow
results before removing
+ // the context from the pool.
+ FlightSqlChannel flightSqlChannel = ctx.getFlightSqlChannel();
+ if (flightSqlChannel != null) {
+ flightSqlChannel.close();
+ }
Review Comment:
[P1] Let teardown finish even if channel close fails
`RootAllocator.close()` throws when outstanding bytes remain. This is
reachable even after request quiescence: `addResult()` can partially allocate
vectors before a later allocation or row-shape failure and never put them in
the cache, or a removal callback can fail to close an entry. Because this call
precedes `closeFlightSqlDeferredExecutors()`, `closeTxn()`, connection-map
removal, the count decrement, and token-map removal, the exception aborts all
mandatory cleanup; on token eviction Guava swallows the listener failure, so
`CloseSession` can still report `CLOSED` with stale pool state. Arrow marks the
allocator closed before throwing, so its close is not retried. Please
isolate/aggregate the channel-close failure and guarantee every other
teardown/bookkeeping stage runs before logging or propagating it with
connection identifiers. Add a test where `channel.close()` throws and verify
all later cleanup still completes.
##########
fe/fe-core/src/main/java/org/apache/doris/service/arrowflight/results/FlightSqlChannel.java:
##########
@@ -168,6 +168,7 @@ public void reset() {
public void close() {
reset();
+ allocator.close();
}
Review Comment:
[P1] Quiesce active Flight requests before closing this allocator
This `close()` now runs from token eviction, explicit `CloseSession`, and
idle-timeout teardown, but an already-admitted handler can still hold this
channel. `getStreamStatementResult()` borrows a cached `VectorSchemaRoot`
through `listener.start()`/`putNext()`, while local and prepared paths allocate
through the same allocator; there is no closing flag or active-request lease.
Teardown can therefore invalidate the live root while `putNext()` is unloading
it, or a request can allocate/put after the only `reset()` and allocator close
(Arrow 19 enforces `assertOpen()` only when Java assertions are enabled).
Please fence context admission/lookup, hold a lease for the entire
borrowed-root/allocation lifetime, and wait or cancel-and-join active users
before invalidating the cache and closing the allocator. A latch-based
DoGet/allocation race test would cover this.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
