[fix](remote-doris) Release a remote Doris scan's Flight SQL session on the statement paths #68338 missed - #68353
morningman wants to merge 4 commits into
Conversation
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
|
run buildall |
…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>
789cd83 to
aefca17
Compare
|
run buildall |
TPC-H: Total hot run time: 27542 ms |
TPC-DS: Total hot run time: 152352 ms |
ClickBench: Total hot run time: 24.15 s |
… its connection, and stop Awaitility blaming the awaiting suite P0 build 1054369 (on this PR) failed test_partial_update_insert_schema_change with `Suite test_active_queries is over, but thread Thread-3029 it left running still asks for a connection`: test_active_queries and test_backend_active_tasks return at once and leave a daemon thread polling their system table for five minutes on purpose; the refusal aefca17 added killed those threads on their first statement, and Awaitility - which by default installs every await() as the JVM's default uncaught-exception handler and rethrows, from the awaiting thread, what any thread threw uncaught meanwhile - handed the exception to the unrelated suite that happened to be awaiting a schema change (the sibling thread's exception the same second reached nobody and went to stderr). - OpenedDorisConnections (new): the table of a suite's connections with the threads that opened them, each kept exactly as long as its thread runs; add and drain are serialized. The suite-end drain closes the connections of finished threads and hands those of running threads to STRAY, the table shared by all suites, where a registration after the drain goes too; every statement of any suite closes the strays whose thread has finished, and RegressionTest closes the rest after the last run. - RegressionTest.initGroovyEnv: Awaitility.doNotCatchUncaughtExceptionsByDefault() next to pollInSameThread(). A thread a suite started itself is that suite's to join and check; Suite.thread() reports through its future. Test: OpenedDorisConnectionsTest. Locally, two scratch suites (a daemon like test_active_queries's, a neighbour inside Awaitility.await()) fail with the first cut's framework the way the P0 run did and pass with this one, the daemon's connection closed 100ms after its thread ended. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
run buildall |
TPC-H: Total hot run time: 27350 ms |
TPC-DS: Total hot run time: 152869 ms |
ClickBench: Total hot run time: 24.19 s |
|
/review |
There was a problem hiding this comment.
Request changes: two P1 correctness gaps remain.
Findings
- Streaming-task cancellation does not give the new statement cleanup a post-quiescence owner. STOPPED skips it, and PAUSED can run it before planning finishes, so the pre-rewrite remote Doris Flight SQL session can still leak until remote
wait_timeout. - The process-wide Awaitility setting removes uncaught raw-worker failures from every suite verdict before the framework has owner-local propagation, allowing product failures to turn green.
Critical checkpoint conclusions
- Goal and proof: The PR closes remote Doris sessions on several previously missed statement paths, but does not accomplish that goal for canceled streaming attempts. The direct cleanup test does not exercise STOPPED or cancellation racing planning.
- Scope and clarity: The FE cleanup changes are focused; the Awaitility switch is process-wide and therefore much broader than the connection-ownership fix.
- Concurrency: The streaming scheduler task thread races the PAUSE/STOP control thread over
ctxandstmtExecutor; the job lock does not quiescebefore(). Regression suites also use raw worker threads;join()provides ordering but no throwable propagation. Connection-map add/drain/remove synchronization otherwise survived the interleaving review, with heavy closes outside its monitor. No new lock-order or deadlock defect was found. - Lifecycle: Prepared execution closes the correct statement generation, Arrow-deferred scans are handed to the coordinator, AutoClose owners finish synchronous use before teardown, and legacy/Nereids coordinators now attempt every scan-node stop. The remaining broken lifecycle is the canceled streaming attempt described inline. No static-initialization issue applies.
- Configuration and compatibility: No configuration, wire/storage format, symbol, rolling-upgrade, persistence, transaction, data-write, or FE-BE variable-passing change is introduced.
- Parallel and conditional paths: Direct/forwarded prepared execution, normal/Arrow ownership, AutoClose callers, legacy/Nereids coordinators, success/failure/retry, PAUSED/STOPPED, and regression suite/STRAY connection ownership were checked. Per-node cleanup isolation and connection-transfer conditions are sound; the canceled-task cleanup suppression is not sound with late planning.
- Tests and results: Added tests cover positive direct cleanup, coordinator failure isolation, prepared execution, and connection tracking, but omit the two cancellation paths and owner-local raw-worker failure propagation. This was a static-only review: the review prompt prohibited builds/tests, so author/CI claims were not treated as independent validation and no local test result is claimed.
- Observability: Existing logs are adequate to diagnose cleanup failures, but logging cannot replace deterministic session close or a failing suite verdict.
- Performance: No substantiated CPU, memory, or complexity regression was found in the per-node stop or connection tracking changes.
- User focus: No additional focus was supplied; the entire 13-file change set and relevant upstream/downstream call chains were reviewed.
All other candidates were either duplicates or dismissed with concrete code evidence. Overall state: changes requested pending fixes for both P1 findings and negative coverage for their failure paths.
| // released here rather than left to the remote frontend's wait_timeout. | ||
| StatementContext statementContext = ctx.getStatementContext(); | ||
| if (statementContext != null) { | ||
| statementContext.close(); |
There was a problem hiding this comment.
[P1] Close canceled attempts only after planning has quiesced
This cleanup is not guaranteed to run at a terminal task boundary. STOPPED only calls cancelAllTasks(false), which marks the task CANCELED; StreamingInsertJob.updateJobStatus force-cleans only PAUSED, while AbstractStreamingTask.execute() skips its own cleanup for every canceled task, so this line is never reached. There is also a PAUSED race before stmtExecutor is assigned: the control thread can close and clear the statement while baseCommand.initPlan(...) is still running, after which a RemoteDorisScanNode can register into the already-drained StatementContext and the canceled task again skips a final close. In both cases the pre-rewrite plan has no coordinator to close that Flight SQL session, leaving it until remote wait_timeout. Please make the task thread perform one final close after before()/run() can no longer register nodes for every terminal status, and cover STOPPED plus cancel-during-planning.
| // the handler in place is whichever suite entered an await() last, so the same exception may just | ||
| // as well reach nobody. A thread's failure reaches its suite through Suite.thread() and the future | ||
| // it returns; a thread a suite started itself is the suite's to join and check. | ||
| Awaitility.doNotCatchUncaughtExceptionsByDefault() |
There was a problem hiding this comment.
[P1] Preserve a suite verdict for raw worker failures
This avoids cross-suite misattribution by making uncaught raw-worker failures unable to fail any suite. The tree still has many Thread.start/new Thread workers that run SQL or assertions and whose parents only call join(); join() never rethrows, and the framework installs no per-suite uncaught-error collector. Suite.thread() can propagate through its future, but those existing raw-thread callers do not use it. The manual result in this PR also explicitly shows the new behavior: a failing daemon only prints to stderr and fails no suite. Please add owner-local propagation (for example, migrate these workers to futures plus get(), or add a per-suite collector checked after joins) before disabling the only process-level signal, so product regressions cannot turn green.
… task thread, for a canceled attempt too From the third review round of this PR. aefca17 closed the attempt's StatementContext from StreamingInsertTask.closeOrReleaseResources(), which misses both cancel paths and is unsafe on one of them: - STOP JOB only cancels the task (AbstractJob.updateJobStatus runs cancelAllTasks(false); StreamingInsertJob.clearRunningStreamTask runs for PAUSED alone), and AbstractStreamingTask.execute()'s finally skips the cleanup of a canceled attempt - so nothing ended the statement, and the Flight SQL session the pre-rewrite plan's remote Doris scan holds on the other frontend stayed open until its wait_timeout. - PAUSE JOB runs that cleanup on the job's control thread, at once when stmtExecutor does not exist yet, i.e. while before()'s initPlan may still be planning: the plan registers its scan nodes only when translated, after the close had drained the statement, and the canceled attempt never closed again. Worse, the planner holds the target table's read lock through translation, which a close from another thread would release from a thread that does not hold it (IllegalMonitorStateException, the lock left held by the planning thread, and the PAUSE not written to the edit log since alterJobStatus throws first). The auto-pause on a failed fetchMeta() / advanceSplitsIfNeed() comes from the job scheduler's thread, so no user action is needed to hit this. The statement is now ended by the task thread only: execute()'s finally calls a new endStatement() hook unconditionally, before the closeOrReleaseResources() a canceled attempt skips; StreamingInsertTask keeps the attempt's StatementContext in a field only the task thread touches and closes it there. closeOrReleaseResources() drops fields again, as before aefca17. Test: StreamingInsertTaskStatementCloseTest - ending an attempt stops what its plan registered; an attempt canceled by a STOP still ends its statement from execute(); a PAUSE during planning does not end the statement from the control thread, and the task thread's final close stops the node registered after it too. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
… a thread it started From the third review round of this PR. 82309eb stopped Awaitility from installing every await() as the JVM's default uncaught-exception handler, which failed whichever suite happened to be awaiting for any thread's failure. Wrong as that attribution was, it was the only thing that could turn the failure of a raw Thread a suite starts and join()s into a red build: join() never rethrows, 83 suites do this, and the framework ran no code on such a thread. After 82309eb those failures went to stderr and failed nothing. UncaughtThreadFailures is now the JVM's default handler. A thread a suite constructs inherits the suite's collector - an InheritableThreadLocal set on the suite's thread in ScriptContext.createAndRunSuite, through any depth of threads started from it - and its failure is recorded there; Suite.doLazyCheck() throws the first one once the body has returned, that is after every thread the suite joined has ended. A failure after that (a thread left running past its suite's end, as test_active_queries intends) or on a thread no suite started fails nothing; every one is logged with the thread and suite names. Test: UncaughtThreadFailuresTest. Manually, four scratch suites against a local cluster with -suiteParallel 4: the suite whose raw joined thread fails a statement now fails, with "Thread Thread-3 of suite joined_worker_fails died with an uncaught exception"; the suite whose joined thread (and a thread started from it) runs fine passes; the suite that returns at once and leaves a daemon whose statement fails 2s later passes, the failure logged as "started by suite stray_daemon whose verdict is already taken; it fails no suite"; and the suite spending those seconds inside Awaitility.await() - the one P0 build 1054369 blamed - passes. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
What problem does this PR solve?
Related PR: #68338 (follow-up: the review of that PR found the paths below), #68101 / #68266 (why a leaked Flight SQL session costs the catalog user's connection quota on the remote FE)
Problem Summary:
Context. A Remote Doris catalog with
use_arrow_flight = truereads another Doris cluster over Arrow Flight SQL:RemoteDorisScanNodeopens a Flight SQL session on a remote FE while the plan is translated (getSplits), runs the query there, and hands the endpoints to the local BE. #68338 made that session end with the local query: the coordinator'sclose()/cancel()stops the scan node, which sendsCloseSession; and because the session is opened before any coordinator exists and not every plan gets one, the node also registers itself with theStatementContext, whoseclose()stops what is still registered - the fallback for a statement that fails between planning and dispatch, or a plan probed and discarded.That fallback assumes that whoever ran the statement ends it with
StatementContext.close().ConnectProcessordoes (the per-statement finally of aCOM_QUERY, and of a forwarded request on the master),TaskProcessorandMTMVTaskdo. Three owners of a statement do not, and one loop can skip the stop.1. The problem, and what it cost
COM_STMT_EXECUTE(MysqlConnectProcessor.handleExecute) catches a failure and only finalizes the response; it never closes the per-executionStatementContextthatExecuteCommandallocates, and the next execution'snextStatementContext()only resets the connector scope before dropping it. A prepared query over a Remote Doris table that fails after planning (a SQL block rule on the scan,checkBlockRulesByScanruns afterplan()) leaves its session on the remote FE untilwait_timeout, once per execution. The forwardedCOM_STMT_EXECUTEwas already covered.AutoCloseConnectContext- an EXPORT'sSELECT INTO OUTFILE(external tables can be exported), an ANALYZE's statistics query, and a dozen other internal owners - ends withConnectContext.clear(), which nulls theStatementContextwithout closing it. The same post-plan failure leaves the session behind.StreamingInsertTask) is run by the streaming scheduler, not byTaskProcessor:before()runsinitPlan(..., false)once only to rewrite the TVF - a full plan, whose scan nodes open their sessions and whose coordinator is built and never executed or closed - and then plans the rewritten command again;closeOrReleaseResources()only nulls fields. A streamingINSERT ... SELECTthat joins the TVF with a Remote Doris table leaks one session per attempt, success, retry or cancel. (The first cut of this PR closed the statement fromcloseOrReleaseResources(), which the review found insufficient: a STOP of the job only cancels the task -AbstractJob.updateJobStatusrunscancelAllTasks(false), andexecute()'s finally skips the cleanup of a canceled attempt - and a PAUSE runs that cleanup on the job's control thread, possibly whilebefore()is still planning: before the plan has registered its scan nodes, and while the planner holds the target table's read lock, which aStatementContext.close()from another thread would try to release.)Coordinator.close()andNereidsCoordinator.close()wrapped the whole scan-node loop in one try, and bothcancel()loops had none.SplitAssignment.stop()rethrows the failure of its asynchronous split generation, so a batch-mode external scan planned before a Remote Doris scan could end the loop and skip it. On the normal path the statement's fallback (which is per node) still stopped it; on the deferred path - an Arrow Flight SQL client query, whose scan nodes are handed over to the coordinator kept alive for DoGet - the coordinator is the only owner, and the session had nobody left to close it. Acancel()that threw there also never sent its cancel RPCs.getConnection(), so a suite in thearrow_flight_sqlgroup (whosesqlgoes throughgetArrowFlightSqlConnection()) or one usingmaster_sqlnever ran it and kept every finished thread's connection until teardown; and the teardown drain (snapshot + clear) was not serialized with registration, so a thread still running at the suite's end could register a connection between the two and have it dropped unclosed. The first cut of this PR refused such a thread a connection (IllegalStateException), and P0 build 1054369 showed what that costs:test_active_queriesandtest_backend_active_tasksreturn at once and leave a daemon thread polling their system table for five minutes on purpose, so their threads died on their first statement - and the exception was reported as a failure oftest_partial_update_insert_schema_change, an unrelated suite that happened to be insideAwaitility.await(). Awaitility by default installs everyawait()as the JVM's default uncaught-exception handler and rethrows, from the awaiting thread, whatever any thread threw uncaught meanwhile; with ten suites in parallel that is the wrong suite by construction, and since the handler in place is whichever suite entered anawait()last, sometimes no suite at all (the sibling thread's exception the same second went to stderr).2. What this PR does, and why it helps
StatementContext.close(), the wayConnectProcessordoes:MysqlConnectProcessor.handleExecutein a finally per execution (idempotent: an execution that ran its coordinator has nothing left to release; a forwarded execution is closed twice, the second a no-op),AutoCloseConnectContext.close()beforeclear()(the planner releases its table locks at the end ofplan(), the connector scope close is close-once, and only a Remote Doris scan registers a node, so an owner whose statement scanned none sees a no-op), andStreamingInsertTask.endStatement(), fromAbstractStreamingTask.execute()'s per-attempt finally on the task thread, for every outcome of the attempt - a canceled one included, since nothing else ends it then. The statement's reference lives in a field only the task thread touches;closeOrReleaseResources(), which the control thread runs on PAUSE, drops fields only, so no thread ever closes a statement another thread is still planning.Coordinator.stopScanNodesstops the nodes one by one, a failure logged per node, and both coordinators'close()andcancel()use it. Acancel()now always reaches its cancel RPCs.OpenedDorisConnectionstable (connection → thread), and each lives exactly as long as its thread: registration and the suite-end drain are serialized on the table; the drain closes the connections of threads that have finished and hands those of threads still running - with a warning naming the suite and the threads - toOpenedDorisConnections.STRAY, the table shared by all suites, where a registration after the drain goes too; every statement of any suite closes the strays whose thread has finished, and the end of the run closes what is left. A thread that outlives its suite thus keeps working, as those two suites intend, and its connection is closed within one statement of its end instead of never (or, in the first cut, of failing it). AndAwaitility.doNotCatchUncaughtExceptionsByDefault()next topollInSameThread(), withUncaughtThreadFailuresinstalled as the JVM's default uncaught-exception handler in its place: a thread a suite constructs inherits the suite's collector (anInheritableThreadLocalset on the suite's thread, through any depth of threads started from it), the failure is recorded there, andSuite.doLazyCheck()throws the first one once the body has returned - that is, after every thread the suite joined has ended. So a rawThreada suite starts andjoin()s (83 suites do;join()never rethrows) now fails its own suite, deterministically, instead of whichever suite happened to be awaiting, or none. A failure after the verdict (a thread left running past the suite's end) or on a thread no suite started fails nothing and is logged.What it buys: the invariant #68338 introduced - a Remote Doris scan's session on the remote FE lives exactly as long as the local statement, whoever ran it - now holds for prepared statements, EXPORT/ANALYZE-style internal statements and streaming insert jobs, and survives a scan node whose
stop()throws.3. The classes, and how they call each other
MysqlConnectProcessor.handleExecute(directCOM_STMT_EXECUTE):executor.execute()→ExecuteCommand.run→PreparedStatementContext.nextStatementContext()(a freshStatementContextper execution) → plan / dispatch; the new finally closesctx.getStatementContext().PreparedStatementContext.nextStatementContext()keeps its connector-scope reset (it also drops the pinned writer schemas, whichclose()keeps); its comment is updated.AutoCloseConnectContext.close():StatementContext.close()→ConnectContext.clear()→ConnectContext.remove(). Users:ExportTaskExecutor, the statistics tasks (StatisticsUtil.buildConnectContext),InternalSchemaInitializer, the cloud load/restore tasks,StreamingJobUtils, ...AbstractStreamingTask.endStatement()(new hook, no-op by default), called unconditionally fromexecute()'s per-attempt finally, before thecloseOrReleaseResources()a canceled attempt skips.StreamingInsertTask.endStatement(): closes theStatementContextbefore()recorded inattemptStatement(task thread only) and clears it.StreamingInsertTask.closeOrReleaseResources()(task thread for a finished attempt; the control thread viaStreamingInsertJob.clearRunningStreamTaskfor a paused one): drops fields, as before.Coordinator.stopScanNodes(List<ScanNode>)(new, protected static): the per-node loop; called fromCoordinator.close()/cancel()andNereidsCoordinator.close()/cancel().OpenedDorisConnections(new):add(false once drained),remove,closeThoseOfFinishedThreads,drain,closeAll;STRAYis the instance shared by all suites.SuiteContext:getConnection/getMasterConnection/getArrowFlightSqlConnection→closeConnectionsOfFinishedThreads()(sweeps the suite's table andSTRAY);trackDorisConnection→add, orSTRAY.addafter the drain;close()→closeLeftoverDorisConnections()→drain().RegressionTest.initGroovyEnv:Awaitility.doNotCatchUncaughtExceptionsByDefault()+UncaughtThreadFailures.installAsDefaultHandler();RegressionTest.main:STRAY.closeAll()after the last run.UncaughtThreadFailures(new):OWNER(theInheritableThreadLocal),add(false once the verdict is taken),takeAll,installAsDefaultHandler/uncaughtException.ScriptContext.createAndRunSuitesetsOWNERtosuite.threadFailureson the suite's thread for the run;Suite.doLazyCheck()→takeAll(), and throws the first.RemoteDorisScanNode/RemoteDorisFlightSession(the session,stop(), the registration),StatementContext.close()itself, the deferral gate.Release note
None
Check List (For Author)
Test
Unit tests:
RemoteDorisScanNodeTest(+2: a coordinator closes the session of the scan after one whosestop()throws; a statement run underAutoCloseConnectContextends with the block),MysqlConnectProcessorExecuteCloseTest(aCOM_STMT_EXECUTEfailing after planning stops the scan node its plan registered),StreamingInsertTaskStatementCloseTest(ending an attempt stops the scan nodes its plan registered; an attempt canceled by a STOP still ends its statement fromexecute(); a PAUSE during planning leaves the final close to the task thread, which stops the node registered after the control thread's cleanup too); the neighbouringConnectorStatementScopeTest,ConnectProcessorForwardProtocolTest,ConnectProcessorRetryTest,CoordinatorTest,NereidsCoordinatorTest,OldCoordinatorTest,StreamingInsertTaskAuditTest,StmtExecutorTest,MysqlConnectProcessorCursorFetchTest,ArrowFlightDeferralGateTestpass unchanged. Framework:OpenedDorisConnectionsTest(a connection is closed once the thread that opened it has finished; a drain hands everything over and refuses later registrations;closeAllcloses the connections of running threads too; a close that throws does not stop the others),UncaughtThreadFailuresTest(a failure on a thread the suite started, or on a thread started from it, is the suite's; one after the verdict is not recorded; a thread no suite started fails nothing).Manual test: two scratch suites against a local cluster, run with
-parallel 2 -suiteParallel 2- one returns at once and leaves a daemon thread that runs five statements after a 2s sleep (liketest_active_queries), the other spends 12s insideAwaitility.await().until { sql "select 1"; ... }. With the framework of the first cut the awaiting suite fails withSuite stray_daemon is over, but thread Thread-2 it left running still asks for a connection, as in P0 build 1054369; with this one both pass, the daemon's five statements run, and its connection is closed 100ms after the thread ends (Closed 1 connection(s) of finished threads (threads that outlived their suite)). With the collector (run again with-suiteParallel 4), the daemon variant whose statement fails 2s after its suite returned is logged asUncaught exception in thread Thread-5, started by suite stray_daemon whose verdict is already taken; it fails no suite, and fails nothing - the awaiting neighbour passes; two more scratch suites start a rawThread,join()it and return: the one whose thread fails a statement now fails, withThread Thread-3 of suite joined_worker_fails died with an uncaught exception, and the one whose thread (and a thread started from it) runs fine passes.Behavior changed:
COM_STMT_EXECUTE) closes itsStatementContextwhen it ends, like aCOM_QUERYdoes; an EXPORT / ANALYZE / otherAutoCloseConnectContextstatement and a streaming insert attempt do the same.stop()throws no longer keeps a coordinator from stopping the nodes after it, nor acancel()from sending its cancel RPCs.Awaitility.await()at that moment; a thread left running past its suite's end fails nothing.Does this need documentation?
Check List (For Reviewer who merge this PR)