[fix](remote-doris) End a remote Doris scan's Flight SQL session with the query; stop the regression framework leaking connections - #68338
Conversation
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
|
run buildall |
TPC-H: Total hot run time: 28114 ms |
TPC-DS: Total hot run time: 153046 ms |
ClickBench: Total hot run time: 24.15 s |
…opens on the remote frontend A Remote Doris catalog with use_arrow_flight reads the other cluster over Arrow Flight SQL: for every scan RemoteDorisScanNode performs a handshake against a remote frontend, runs the query there and hands the endpoints to the BE, which reads the rows from the remote BEs with DoGet. The handshake opens a session on the remote frontend, and the scan closed the gRPC channel but never sent CloseSession, so every scan left one session behind until wait_timeout. Since apache#68101 a Flight session is a connection of the remote frontend's one pool, counted against the catalog user's max_user_connections; since apache#68266 nothing caps them any more. A hundred scans within eight hours and the catalog user, MySQL clients included, is refused there with "Reach limit of connections". The external regression hit exactly that on 2026-09-21: the remote_doris suites point the catalog at the frontend under test as root, and 48 leaked sessions took half of root's quota. RemoteDorisFlightSession is the session as an object: handshake, execute, and close() = CloseSession (bounded to five seconds, so a remote frontend that stopped answering cannot hang the local query's teardown) followed by the channel and the allocator. RemoteDorisScanNode keeps it from getSplits until stop(), which the coordinator calls when the local query closes or is cancelled - when the BE is done with the remote query's endpoints. It cannot be closed right after GetFlightInfo: the remote frontend cancels whatever a closed session was still running, and when the remote table is itself an external table in batch mode the remote query is deferred there while the BE reads. A query that fails closes its session at once, so the retry on the next node leaves nothing behind either. For the same reason the local coordinator has to outlive dispatch when the local query is an Arrow Flight SQL query, or apache#67503 would close it, and the session with it, right after exec() while the local BE may still be reading. The deferral gate's predicate is generalized from "has a batch split source" to "the BE still depends on this scan after dispatch": ScanNode.coordinatorMustOutliveDispatch() and Coordinator.mustOutliveDispatch(), with the open session as the second reason. Batch-mode external scans behave as before. RemoteDorisScanNodeTest drives the scan node against an in-process Flight SQL server that counts the sessions it is asked to close; the regression suite test_remote_doris_flight_session scans through a catalog logging in as a user of its own and asserts after each scan that the processlist holds no ArrowFlightSQL session of that user. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… and close the connections a suite's own threads leave behind
A suite's Doris connections are ThreadLocal to the thread that opened them,
and only the suite thread and the threads Suite.thread() runs close theirs
in closeThreadLocal(). Awaitility.await()...until { sql ... } evaluated the
condition on Awaitility's own thread, which dies with the await(), so every
call opened a connection nothing closed: it stayed on the frontend until
the client JVM garbage-collected it (the frontend logs those as "No more
data to be read. Close connection"). 230 suites call Awaitility.await()
directly; in the external regression of 2026-09-21 one of them opened 22
such connections in 46 seconds, and with the Flight sessions of the
remote_doris scans counted into the same quota since apache#68101, root reached
max_user_connections and 14 connections were refused. A P0 run of the same
day shows the scale: 76 such connections in one minute, next to 89 opened
on threads suites start themselves (Thread.start { streamLoad }; join, one
pair per step), all of them left to the garbage collector.
Awaitility.pollInSameThread() at framework start-up makes every until { }
run on the suite thread and reuse the suite's connection, as
Suite.awaitUntil already did. The trade-off, that atMost() cannot
interrupt a condition that blocks, is bounded by the timeouts of the
statements a condition runs.
SuiteContext also records every connection its thread-local accessors
open, with the thread that opened it. On every statement it closes the
connections of threads that have finished, so a suite that starts a thread
per step holds at most the connections of the threads still running -
deterministically, where before the population depended on when the JVM
next collected garbage - and when the suite ends it closes whatever is
left, with a warning naming the suite. The docker helpers close the
connection their action opened before putting the original one back (they
dropped it before), and the multi-cluster helper types that original
connection as the ConnectionInfo it is.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
8b63d98 to
821fb3d
Compare
|
skip buildall |
|
/review |
…when no coordinator ever owns its plan The session a RemoteDorisScanNode opens on the remote frontend was released only by the coordinator's stop(), and it is opened while the plan is translated, before any coordinator exists. Every plan that never got a coordinator, or got one nobody closed, leaked it exactly as before: a statement failing between planning and dispatch (a SQL block rule on the scan, an INSERT whose transaction cannot begin - a re-used WITH LABEL, the per-db txn limit), the plan INSERT OVERWRITE and every materialized-view refresh run only to locate the sink and then discard, a load job created from the plan. And the same-plan retry of handleQueryWithRetry re-dispatched the endpoints of a session the failed attempt's cancel() had just closed - the remote frontend tears down a query it had deferred with the session. The statement is now the fallback owner: keepFlightSession registers the node with the StatementContext, whose close() (ConnectProcessor's per-statement finally, TaskProcessor, MTMVTask) stops what is still registered; the deferral gate hands the nodes over to the deferred coordinator first, so an Arrow Flight query kept alive for DoGet is untouched, and stop() is idempotent, so a coordinator that already closed costs nothing. INSERT OVERWRITE releases its probe plan's scan nodes as soon as the plan has been read, before the real insert opens its own. A retry is refused, with the original error, when a scan node reports cannotBeRedispatched() - a remote Doris scan once stop() ended its session. RemoteDorisScanNodeTest covers the statement-end release, the handover to a deferred coordinator and the predicate; test_remote_doris_flight_session adds an INSERT OVERWRITE and a re-used WITH LABEL and asserts that neither leaves a session behind. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…themselves now that Awaitility polls on the calling thread Awaitility.pollInSameThread() runs a condition to completion before atMost() is checked, so the two SuiteCluster waits that put the whole doris-compose subprocess wait inside a condition (runCmd, runCmdList) had lost their only time bound: a hung docker daemon would hang the docker suite thread for as long as the subprocess lived instead of failing the suite after timeoutSecond, and with dockerSuiteParallel = 1 every later docker suite of the run with it. They now wait on a helper thread joined with the command's timeout and destroy the process on expiry; waitForProcessOutput joins the stream readers, so the output is complete once the thread has ended. The comment at the global default now says what the trade-off really is: a condition that runs statements is bounded by their timeouts, one that waits on anything else has to bound that wait itself. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
aaba58f
|
run buildall |
|
/review |
|
Codex automated review failed and did not complete. Error: Review recovery stopped: PR base/head or open state changed; refusing to resume stale context Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
TPC-H: Total hot run time: 27687 ms |
TPC-DS: Total hot run time: 153084 ms |
ClickBench: Total hot run time: 24.07 s |
There was a problem hiding this comment.
Request changes: I found eight blocking resource-lifecycle and regression-framework issues.
Completion status: CAPPED / INCOMPLETE. Round 3 produced two new valuable findings (streaming INSERT and external-table EXPORT owners), and the required three-round limit prevents another convergence round. The live head/base still match the reviewed bundle, and no current inline thread duplicates these reports.
Checkpoint conclusions:
- Goal and tests: The normal remote-Doris session lifecycle is improved and the new unit/regression tests cover ordinary, failed-query, discarded-plan, and deferred cases, but the goal is not fully met: direct prepared execution, streaming tasks, EXPORT error cleanup, and exception-interrupted handoff still leak sessions. The tests also omit multi-scan timeout aggregation and those owners.
- Scope and clarity: The product-side ownership model is reasonably focused, but the process-wide Awaitility default is broader than its SQL-specific motivation and changes hundreds of unrelated waits.
- Concurrency: Remote scan publication/stop state is synchronized and heavy close work is outside its monitor. However, coordinator cleanup is not isolated per scan node; close latency compounds on the single timeout-checker thread; and the regression connection registry races teardown. No new lock-order deadlock was established.
- Lifecycle and parallel paths: Normal query, forwarded execution, discarded INSERT OVERWRITE, ordinary scheduled INSERT, MTMV, cancellation, retry, and Arrow deferral were traced. The direct COM_STMT_EXECUTE, streaming scheduler, AutoCloseConnectContext EXPORT, and throwing-stop paths are incomplete as detailed inline.
- Configuration and compatibility: No new configuration, wire/storage format, persisted metadata, or FE-BE variable is introduced. The coordinator predicate rename is internal; no rolling-upgrade compatibility defect was found.
- Conditions, error handling, and observability: The retry guard and idempotent stop conditions are explained and reachable ordering is sound. Close failures are logged, but the remaining leaks/stalls are not covered by tests or metrics.
- Transactions, persistence, and data writes: INSERT/EXPORT transaction paths were inspected; the reported defects concern cleanup ownership, not committed-data atomicity. No EditLog/replay change is present.
- Performance: Sequential five-second remote closes make cancellation O(number of remote scans), and same-thread Awaitility can turn short deadlines into multi-minute stalls.
- Other paths and tests: Arrow/master accessors miss finished-thread scavenging, while suite teardown can lose a concurrent registration. Static review only; per the review contract I did not build, run tests, or edit product source.
- User focus: No additional focus was supplied; the entire authoritative 15-file diff and the relevant lifecycle call chains were reviewed.
| // The coordinator outlives this statement, and with it what its scan nodes hold | ||
| // for the BE: the statement's own end must not stop them (StatementContext.close | ||
| // is the fallback for a plan no coordinator owns), the coordinator's close does. | ||
| statementContext.handOverScanNodesToDeferredCoordinator(planner.getScanNodes()); |
There was a problem hiding this comment.
[P1] This handoff removes every planner scan node from the statement fallback, but the deferred coordinator is not guaranteed to attempt every stop. Coordinator.close() and NereidsCoordinator.close() wrap the whole loop in one try; an earlier batch file scan can throw from SplitAssignment.stop() after an asynchronous split failure, so a following RemoteDorisScanNode is skipped and its session now has no remaining owner. Please isolate failures per scan node (including the analogous cancel loops) and cover a deferred mixed-scan query where the first stop throws.
| // frontend's query_timeout, the framework's socketTimeout); a condition that waits on | ||
| // anything else has to bound that wait itself, as SuiteCluster does for its doris-compose | ||
| // subprocesses. | ||
| Awaitility.pollInSameThread() |
There was a problem hiding this comment.
[P1] This process-wide setting makes atMost unable to stop a blocking predicate, but existing non-SQL predicates have not all been bounded. For example the 3-second awaits in the partial-update fault-injection suites call be_get_compaction_status, whose curl helper can perform ten 10-second attempts with 5-second sleeps. A failed BE can now turn that 3-second check into a multi-minute stall. Please scope same-thread polling to the SQL helpers that need ThreadLocal reuse, or independently cap every blocking predicate before changing the global default.
| // openedDorisConnections). The warning names the suite: a `sql` on a thread the suite created | ||
| // itself and left running (an Executors pool it never shut down) is what leaves them behind. | ||
| private void closeLeftoverDorisConnections() { | ||
| List<Connection> leftover = new ArrayList<>(openedDorisConnections.keySet()) |
There was a problem hiding this comment.
[P1] The snapshot and clear are not atomic with trackDorisConnection(). A still-running suite worker can register between them, in which case clear drops the connection without closing it, or just after clear, in which case it remains after the only teardown drain. Please serialize registration with teardown and mark the context closing so a late registration is closed/rejected immediately; a barrier-based race test would make this deterministic.
FE UT Coverage ReportIncrement line coverage |
FE Regression Coverage ReportIncrement line coverage |
…on the statement paths apache#68338 missed Follow-up to apache#68338, from its review. Three owners of a statement never ran StatementContext.close(), so what a plan opened for a coordinator that never came - the Flight SQL session of a remote Doris scan, when the statement failed between planning and dispatch - stayed open on the remote frontend until its wait_timeout: - a direct COM_STMT_EXECUTE: MysqlConnectProcessor.handleExecute only finalized the response (the forwarded one already closed the context in its finally); - a statement run under AutoCloseConnectContext (an EXPORT's SELECT INTO OUTFILE, an ANALYZE's statistics query): close() nulled the context through ConnectContext.clear() without closing it; - a streaming insert task's attempt: StreamingInsertTask is run by the streaming scheduler, not by TaskProcessor; before() plans once only to rewrite the TVF, a plan no coordinator ever takes, and closeOrReleaseResources() only nulled fields. Each now ends its statement with StatementContext.close(). The coordinator stops its scan nodes one by one (Coordinator.stopScanNodes, used by both coordinators' close() and cancel()): a batch split source rethrows the failure of its asynchronous split generation from stop(), which ended the loop and skipped the nodes after it - on the deferred (Arrow Flight) path, where the coordinator is the session's only owner, nobody was left to close that session. cancel() no longer aborts before its cancel RPCs when a stop() throws. Regression framework: the sweep of the connections opened by finished threads runs from all three thread-local accessors (an arrow_flight_sql group suite never ran it); registration and the final drain are serialized, and a thread a suite left running past its end is refused a connection instead of registering into a drained table. Tests: RemoteDorisScanNodeTest (a coordinator closes the session of the scan after one whose stop() throws; a statement under AutoCloseConnectContext ends with the block), MysqlConnectProcessorExecuteCloseTest, StreamingInsertTaskStatementCloseTest. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
What problem does this PR solve?
Related PR: #68101 (the one connection pool, which made the leak visible), #68266 (the bearer token as the session's credential, which removed its last cap), #67503 / #62259 (the Arrow Flight deferral gate this generalizes)
Problem Summary:
Context. A Remote Doris catalog with
use_arrow_flight = truereads another Doris cluster over Arrow Flight SQL: for every scan,RemoteDorisScanNodeon the local FE performs a Flight SQL handshake against a remote FE (authenticateBasicToken), runs the query there (GetFlightInfo), and hands the endpoints - a ticket per remote BE - to the local BE, which reads the rows withDoGetstraight from the remote BEs.The handshake is not free on the remote side: since #68101 a Flight SQL session is a connection in the remote FE's one connection pool, counted against
qe_max_connection, the Arrow Flight SQL sub-quota and the catalog user'smax_user_connections; since #68266 the bearer token is that session's name in the pool and nothing else, so the session ends only onCloseSession,KILL CONNECTION,wait_timeout(8h by default) or an FE restart.The regression framework has a leak of its own of the same shape: a suite's Doris connections are
ThreadLocalto the thread that opened them, and only the suite thread andSuite.thread()close theirs; asqlon any other thread opens a connection nobody closes.1. The problem, and what it cost
RemoteDorisScanNode.executeFlightSqlQueryclosed the gRPC channel and the allocator in a try-with-resources but never sentCloseSession. Every scan of a Remote Doris table therefore left one Flight SQL session behind on the remote FE, under the catalog user, untilwait_timeout. Before [refactor](arrow-flight) One connection pool for MySQL connections and Arrow Flight SQL sessions #68101 this was invisible: Flight sessions had a pool of their own, were not counted per user, and a per-user LRU ofmax_user_connections / 2tokens evicted the oldest. After [refactor](arrow-flight) One connection pool for MySQL connections and Arrow Flight SQL sessions #68101 the leaked sessions eat the catalog user's quota on the remote FE; after [refactor](arrow-flight) Make the bearer token the credential of exactly one session #68266 nothing caps them at all. A hundred scans within 8h and the user - MySQL clients included - is refused there withReach limit of connections.remote_dorissuites point the catalog at the FE under test with userroot; 49 scans left 48 Flight sessions (Arrow Flight SQL: 512 (current: 48)in the refusal), which took half of root's 100; the other half was taken by the framework leak below, and 14 MySQL connections were refused in a six-second window.Awaitility.await()...until { sql ... }evaluates the condition on Awaitility's own thread, which dies with theawait(). Each call leaked one root connection until the client JVM garbage-collected it (the FE logs those asNo more data to be read. Close connection). 230 suites callAwaitility.await()directly; in the failing run one suite opened 22 such connections in 46 seconds, and 29 of them were collected in one GC at the moment the refusals stopped. Suites that callsqlfrom threads of their own (Thread.start { streamLoad }, anExecutorspool) leak the same way - a P0 run of the same day shows 76 Awaitility connections and 89 own-thread connections opened by root within one minute, all left to the garbage collector - and the two docker helpers dropped the connection their action opened without closing it.2. What this PR does, and why it helps
FE:
RemoteDorisFlightSession(new): the session as an object - handshake,execute, andclose()=CloseSession(bounded to 5s so a remote FE that stopped answering cannot hang the local query's teardown; the session is then left to itswait_timeoutas before) followed by the channel and the allocator.open()leaves nothing behind when the handshake is refused; a query that fails closes its session at once, so the retry on the next node leaves nothing behind either. Idempotent.RemoteDorisScanNodekeeps the session fromgetSplitsuntilstop(), which the coordinator calls when the local query closes or is cancelled - i.e. when the local BE is done with the remote query's endpoints. It cannot be closed right afterGetFlightInfo: the remote FE cancels whatever a closed session was still running, and when the remote table is itself an external table scanned in batch mode the remote query is deferred there and still running while the local BE reads.exec(), while the local BE may still be reading). The deferral gate's predicate is generalized from "has a batch split source" to "the BE still depends on this scan after dispatch":ScanNode.hasBatchSplitSource()->coordinatorMustOutliveDispatch(),Coordinator.hasBatchSplitSource()->mustOutliveDispatch();RemoteDorisScanNodeadds its open session as the second reason. Batch-mode external scans behave exactly as before.INSERTwhose transaction cannot begin - a re-usedWITH LABEL, the per-db txn limit), the planINSERT OVERWRITEand every materialized-view refresh run only to locate the sink and discard, a load job created from the plan.keepFlightSessionregisters the node with theStatementContext, whoseclose()(the per-statement finally ofConnectProcessor,TaskProcessor,MTMVTask) stops what is still registered; the deferral gate hands the nodes over to the deferred coordinator beforedeferForArrowFlight, so a Flight query kept alive for DoGet is untouched.INSERT OVERWRITEreleases its probe plan's scan nodes as soon as the plan has been read.handleQueryWithRetryre-dispatches the failed attempt's plan after itscancel()stopped the scan nodes, and the endpoints of a remote Doris scan belong to the query of a session that is then gone (the remote FE tears down a query it had deferred).ScanNode.cannotBeRedispatched()(true for a remote Doris scan oncestop()ended its session) makes the retry rethrow the original error instead.Regression framework:
Awaitility.pollInSameThread()at framework start-up: everyuntil { }now runs on the suite thread and reuses the suite's connection, asSuite.awaitUntilalready did. The trade-off: anatMost()no longer bounds a condition that blocks (the poll runs to completion before the bound is checked). A condition that runs statements is bounded by their timeouts; a condition that waits on anything else bounds the wait itself -SuiteClusternow runs itsdoris-composesubprocess waits on a helper thread joined with the command's timeout and destroys the process on expiry (they used to rely onatMost()).SuiteContextrecords every connection its thread-local accessors open, with the thread that opened it. On every statement it closes the connections of threads that have finished (a suite that starts a thread per step -Thread.start { streamLoad }; join, as the mow flexible suites do 72 times - now holds at most the connections of the threads still running, deterministically, where before the population depended on the JVM's next GC), and when the suite ends it closes whatever is left, with a warning naming the suite.docker,dockers) close the connection their action opened before restoring the original one (and the multi-cluster one now types that original as theConnectionInfoit is).Tests:
RemoteDorisScanNodeTest: an in-process Flight SQL server counts the sessions it is asked to close. The session lives from the query untilstop();stop()twice closes once; a failed query closes at once; a refused handshake opens nothing; a session handed over afterstop()is closed at once; the coordinator of a query with such a scanmustOutliveDispatch(); a session no coordinator takes ends with the statement, one handed to a deferred coordinator does not; and a node whosestop()ended a sessioncannotBeRedispatched().ArrowFlightDeferralGateTestfollows the rename.external_table_p0/remote_doris/test_remote_doris_flight_session: a catalog logging in as a user of its own scans a table five times, thenINSERT OVERWRITEs from it, then runs anINSERT ... WITH LABELtwice (the second is refused after planning), and asserts after each statement thatinformation_schema.processlistholds noArrowFlightSQLsession of that user - assertions that fail on master.What it buys: a Remote Doris scan costs the remote FE one session for exactly the duration of the local query, whatever the protocol of the local client; the catalog user's quota on the remote FE is no longer consumed by history; and the regression framework no longer manufactures the MySQL half of the pressure.
3. The classes, and how they call each other
RemoteDorisScanNode(existing):getSplits->executeQuery->executeFlightSqlQuery(host, user, password, sql, timeout):RemoteDorisFlightSession.open+execute, thenkeepFlightSession(which also registers the node withStatementContext.stopScanNodeAtClose).stop()(fromCoordinator.close()/cancel(), orStatementContext.close()as the fallback) closes the session;coordinatorMustOutliveDispatch()is true while one is held;cannotBeRedispatched()oncestop()ended one.RemoteDorisFlightSession(new):open(FlightClient +authenticateBasicToken),execute(FlightSqlClient.execute),close(closeSessionwith a 5s deadline, then client and allocator).ScanNode.coordinatorMustOutliveDispatch()(renamed fromhasBatchSplitSource):splitAssignment != null, overridable.ScanNode.cannotBeRedispatched()(new): false by default.Coordinator.mustOutliveDispatch()(renamed): any scan node'scoordinatorMustOutliveDispatch().StmtExecutor.executeAndSendResult: the deferral gate now readscoord.mustOutliveDispatch(); the deferral gate callsStatementContext.handOverScanNodesToDeferredCoordinatorbeforedeferForArrowFlight;handleQueryWithRetryrethrows instead of retrying whenplanCannotBeRedispatched().StatementContext:stopScanNodeAtClose,handOverScanNodesToDeferredCoordinator, andclose()stopping what is left (after the table locks, before the connector scope).InsertOverwriteTableCommand.run: stops the scan nodes of the plan it probes and discards.SuiteCluster.waitForDorisCompose: the bounded subprocess waitrunCmd/runCmdListuse instead ofAwaitility.await().atMost(...).DorisFlightSqlProducer.closeSession->FlightSessionsInConnectPool.closeConnectContext->ConnectContext.cleanup()+cancelQuery.RegressionTest.initGroovyEnv:Awaitility.pollInSameThread().SuiteContext:openedDorisConnections(connection -> opening thread),trackDorisConnection,closeConnectionsOfFinishedThreads(fromgetConnection(), i.e. every statement),closeDorisConnection,closeLeftoverDorisConnections(fromclose());Suite.dockerImpl/dockerscallcloseDorisConnection.Release note
None
Check List (For Author)
Test
Behavior changed:
SHOW PROCESSLISTno longer accumulatesArrowFlightSQLsessions of the catalog user.handleQueryWithRetry): the failed attempt's cancel ended the session its endpoints belong to, so the original error is returned instead of a retry that would read a query the remote FE may have torn down.INSERT OVERWRITE(and an MTMV refresh) over a Remote Doris table no longer leaves the session of its probe plan behind; a statement that fails after planning (a SQL block rule, a re-used label) no longer leaves its session behind either.Does this need documentation?
Check List (For Reviewer who merge this PR)