Skip to content

[fix](remote-doris) End a remote Doris scan's Flight SQL session with the query; stop the regression framework leaking connections - #68338

Merged
morningman merged 4 commits into
apache:masterfrom
morningman:remote-doris-flight-session
Sep 22, 2026
Merged

morningman merged 4 commits into
apache:masterfrom
morningman:remote-doris-flight-session

Conversation

@morningman

@morningman morningman commented Sep 21, 2026

Copy link
Copy Markdown
Contributor

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 = true reads another Doris cluster over Arrow Flight SQL: for every scan, RemoteDorisScanNode on 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 with DoGet straight 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's max_user_connections; since #68266 the bearer token is that session's name in the pool and nothing else, so the session ends only on CloseSession, 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 ThreadLocal to the thread that opened them, and only the suite thread and Suite.thread() close theirs; a sql on any other thread opens a connection nobody closes.

1. The problem, and what it cost

  • RemoteDorisScanNode.executeFlightSqlQuery closed the gRPC channel and the allocator in a try-with-resources but never sent CloseSession. Every scan of a Remote Doris table therefore left one Flight SQL session behind on the remote FE, under the catalog user, until wait_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 of max_user_connections / 2 tokens 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 with Reach limit of connections.
  • This is what broke the external regression pipeline on 2026-09-21 (TeamCity 1053416 on [fix](expr) Canonicalize logical OR results #68308): the remote_doris suites point the catalog at the FE under test with user root; 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.
  • The framework leak: Awaitility.await()...until { sql ... } evaluates the condition on Awaitility's own thread, which dies with the await(). Each call leaked one root connection until the client JVM garbage-collected it (the FE logs those as No more data to be read. Close connection). 230 suites call Awaitility.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 call sql from threads of their own (Thread.start { streamLoad }, an Executors pool) 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, and close() = CloseSession (bounded to 5s so a remote FE that stopped answering cannot hang the local query's teardown; the session is then left to its wait_timeout as 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.
  • RemoteDorisScanNode keeps the session from getSplits until stop(), 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 after GetFlightInfo: 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.
  • For the same reason the local coordinator has to outlive dispatch when the local query is itself an Arrow Flight SQL query (otherwise [Bug] Arrow Flight SQL: an abandoned session holds its query's workload-group queue slot until wait_timeout (8h) #67503 closes 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.hasBatchSplitSource() -> coordinatorMustOutliveDispatch(), Coordinator.hasBatchSplitSource() -> mustOutliveDispatch(); RemoteDorisScanNode adds its open session as the second reason. Batch-mode external scans behave exactly as before.
  • The statement is the fallback owner. The session is opened while the plan is translated, before any coordinator exists, and not every plan gets a coordinator or gets one that is closed: a statement that fails 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 discard, a load job created from the plan. keepFlightSession registers the node with the StatementContext, whose close() (the per-statement finally of ConnectProcessor, TaskProcessor, MTMVTask) stops what is still registered; the deferral gate hands the nodes over to the deferred coordinator before deferForArrowFlight, so a Flight query kept alive for DoGet is untouched. INSERT OVERWRITE releases its probe plan's scan nodes as soon as the plan has been read.
  • A same-plan retry must not reuse a plan whose scan node released what the BE scans with: handleQueryWithRetry re-dispatches the failed attempt's plan after its cancel() 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 once stop() ended its session) makes the retry rethrow the original error instead.

Regression framework:

  • Awaitility.pollInSameThread() at framework start-up: every until { } now runs on the suite thread and reuses the suite's connection, as Suite.awaitUntil already did. The trade-off: an atMost() 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 - SuiteCluster now runs its doris-compose subprocess waits on a helper thread joined with the command's timeout and destroys the process on expiry (they used to rely on atMost()).
  • SuiteContext 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 (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.
  • The two docker helpers (docker, dockers) close the connection their action opened before restoring the original one (and the multi-cluster one now types that original as the ConnectionInfo it is).

Tests:

  • RemoteDorisScanNodeTest: an in-process Flight SQL server counts the sessions it is asked to close. The session lives from the query until stop(); stop() twice closes once; a failed query closes at once; a refused handshake opens nothing; a session handed over after stop() is closed at once; the coordinator of a query with such a scan mustOutliveDispatch(); a session no coordinator takes ends with the statement, one handed to a deferred coordinator does not; and a node whose stop() ended a session cannotBeRedispatched().
  • ArrowFlightDeferralGateTest follows the rename.
  • Regression 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, then INSERT OVERWRITEs from it, then runs an INSERT ... WITH LABEL twice (the second is refused after planning), and asserts after each statement that information_schema.processlist holds no ArrowFlightSQL session 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, then keepFlightSession (which also registers the node with StatementContext.stopScanNodeAtClose). stop() (from Coordinator.close() / cancel(), or StatementContext.close() as the fallback) closes the session; coordinatorMustOutliveDispatch() is true while one is held; cannotBeRedispatched() once stop() ended one.
  • RemoteDorisFlightSession (new): open (FlightClient + authenticateBasicToken), execute (FlightSqlClient.execute), close (closeSession with a 5s deadline, then client and allocator).
  • ScanNode.coordinatorMustOutliveDispatch() (renamed from hasBatchSplitSource): splitAssignment != null, overridable. ScanNode.cannotBeRedispatched() (new): false by default.
  • Coordinator.mustOutliveDispatch() (renamed): any scan node's coordinatorMustOutliveDispatch().
  • StmtExecutor.executeAndSendResult: the deferral gate now reads coord.mustOutliveDispatch(); the deferral gate calls StatementContext.handOverScanNodesToDeferredCoordinator before deferForArrowFlight; handleQueryWithRetry rethrows instead of retrying when planCannotBeRedispatched().
  • StatementContext: stopScanNodeAtClose, handOverScanNodesToDeferredCoordinator, and close() 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 wait runCmd / runCmdList use instead of Awaitility.await().atMost(...).
  • Remote FE, untouched: DorisFlightSqlProducer.closeSession -> FlightSessionsInConnectPool.closeConnectContext -> ConnectContext.cleanup() + cancelQuery.
  • RegressionTest.initGroovyEnv: Awaitility.pollInSameThread().
  • SuiteContext: openedDorisConnections (connection -> opening thread), trackDorisConnection, closeConnectionsOfFinishedThreads (from getConnection(), i.e. every statement), closeDorisConnection, closeLeftoverDorisConnections (from close()); Suite.dockerImpl / dockers call closeDorisConnection.
local FE                                                  remote FE                         remote BE
RemoteDorisScanNode.getSplits
  '- executeFlightSqlQuery
       |- RemoteDorisFlightSession.open ---- handshake --> openSession (pool: +1 for the catalog user)
       |- session.execute ----------------- GetFlightInfo --> runs the query ---------------> result buffered
       '- keepFlightSession                                                                   (ticket per BE)
Coordinator.exec  -> local BE ------------------------------ DoGet(ticket) --------------------> rows
  (Arrow Flight local client: coordinator kept alive, mustOutliveDispatch() == true)
Coordinator.close / cancel
  '- scanNode.stop
       '- session.close ------------------- CloseSession --> closeConnectContext (pool: -1)

Release note

None

Check List (For Author)

  • Test

    • Regression test
    • Unit Test
    • Manual test (add detailed scripts or steps below)
    • No need to test or manual test. Explain why:
      • This is a refactor/code format and no logic has been changed.
      • Previous test can cover this change.
      • No code files have been changed.
      • Other reason
  • Behavior changed:

    • No.
    • Yes.
      • A Remote Doris scan ends its Flight SQL session on the remote FE when the local query ends; the remote FE's SHOW PROCESSLIST no longer accumulates ArrowFlightSQL sessions of the catalog user.
      • An Arrow Flight SQL query on the local FE that scans a Remote Doris table keeps its coordinator until the session's next command, its teardown or the deferred-query idle reaper, like a batch-mode external scan does ([Bug] Arrow Flight SQL + Iceberg: SplitSource released before DoGet (fetchSplitBatch crash) #62259).
      • A query over a Remote Doris table whose dispatch failed with an RPC error is not retried with the same plan (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?

    • No.
    • Yes.

Check List (For Reviewer who merge this PR)

  • Confirm the release note
  • Confirm test cases
  • Confirm document
  • Add branch pick label

@hello-stephen

Copy link
Copy Markdown
Contributor

Thank you for your contribution to Apache Doris.
Don't know what should be done next? See How to process your PR.

Please clearly describe your PR:

  1. What problem was fixed (it's best to include specific error reporting information). How it was fixed.
  2. Which behaviors were modified. What was the previous behavior, what is it now, why was it modified, and what possible impacts might there be.
  3. What features were added. Why was this function added?
  4. Which code was refactored and why was this part of the code refactored?
  5. Which functions were optimized and what is the difference before and after the optimization?

@morningman

Copy link
Copy Markdown
Contributor Author

run buildall

@hello-stephen

Copy link
Copy Markdown
Contributor
TPC-H: Total hot run time: 28114 ms
machine: 'aliyun_ecs.c7a.8xlarge_32C64G'
scripts: https://github.com/apache/doris/tree/master/tools/tpch-tools
Tpch sf100 test result on commit 8b63d9889ca4e9ed505c2344042f504dd08afa3e, data reload: false

------ Round 1 ----------------------------------
============================================
q1	17784	3901	3857	3857
q2	2181	369	326	326
q3	10054	1400	802	802
q4	4681	487	350	350
q5	7474	835	558	558
q6	173	172	137	137
q7	739	780	602	602
q8	9311	1558	1520	1520
q9	5515	4267	4260	4260
q10	6837	1353	1016	1016
q11	441	287	253	253
q12	642	429	292	292
q13	18079	2596	1990	1990
q14	263	263	235	235
q15	q16	731	723	679	679
q17	1838	1124	991	991
q18	6497	5605	5600	5600
q19	1313	1210	980	980
q20	493	396	262	262
q21	5802	3527	3090	3090
q22	444	374	314	314
Total cold run time: 101292 ms
Total hot run time: 28114 ms

----- Round 2, with runtime_filter_mode=off -----
============================================
q1	4630	4691	4453	4453
q2	720	559	559	559
q3	5081	5131	4693	4693
q4	2267	2346	1490	1490
q5	4548	4403	4636	4403
q6	227	179	131	131
q7	1807	1750	1479	1479
q8	2335	2055	2004	2004
q9	7276	7272	7312	7272
q10	3677	3592	3063	3063
q11	562	368	345	345
q12	704	707	504	504
q13	2294	2573	1997	1997
q14	270	272	246	246
q15	q16	668	704	596	596
q17	7346	6727	6671	6671
q18	11906	11056	11787	11056
q19	1062	1021	1020	1020
q20	2198	2181	1915	1915
q21	5027	4150	4316	4150
q22	522	466	403	403
Total cold run time: 65127 ms
Total hot run time: 58450 ms

@hello-stephen

Copy link
Copy Markdown
Contributor
TPC-DS: Total hot run time: 153046 ms
machine: 'aliyun_ecs.c7a.8xlarge_32C64G'
scripts: https://github.com/apache/doris/tree/master/tools/tpcds-tools
TPC-DS sf100 test result on commit 8b63d9889ca4e9ed505c2344042f504dd08afa3e, data reload: false

query5	4336	603	457	457
query6	459	208	191	191
query7	4969	571	294	294
query8	338	184	190	184
query9	8786	4037	4063	4037
query10	450	304	261	261
query11	5826	3546	3219	3219
query12	154	90	85	85
query13	1261	583	386	386
query14	6542	4601	4310	4310
query14_1	4073	3970	3998	3970
query15	214	204	180	180
query16	976	474	452	452
query17	918	684	544	544
query18	2416	478	355	355
query19	204	187	147	147
query20	87	82	82	82
query21	222	140	121	121
query22	13117	12992	12740	12740
query23	14103	13170	12461	12461
query23_1	12676	12611	12637	12611
query24	7241	1111	631	631
query24_1	671	664	713	664
query25	576	442	372	372
query26	1270	330	178	178
query27	2706	532	337	337
query28	4608	1985	1958	1958
query29	1675	737	505	505
query30	305	220	176	176
query31	902	755	637	637
query32	143	97	89	89
query33	562	305	245	245
query34	1210	1109	655	655
query35	731	742	627	627
query36	835	767	723	723
query37	151	107	91	91
query38	1849	1760	1713	1713
query39	702	687	667	667
query39_1	672	653	666	653
query40	220	120	110	110
query41	68	66	65	65
query42	97	93	97	93
query43	339	351	303	303
query44	1369	718	719	718
query45	198	192	175	175
query46	1080	1229	712	712
query47	1503	1479	1392	1392
query48	410	416	302	302
query49	589	411	299	299
query50	945	350	261	261
query51	10431	10718	10572	10572
query52	87	88	81	81
query53	250	261	181	181
query54	256	206	196	196
query55	83	81	69	69
query56	244	249	223	223
query57	1367	1413	1453	1413
query58	291	263	263	263
query59	2024	2055	1858	1858
query60	292	250	237	237
query61	155	144	152	144
query62	397	325	271	271
query63	220	176	184	176
query64	2851	1032	871	871
query65	3495	3427	3404	3404
query66	1812	425	306	306
query67	19943	19985	19787	19787
query68	3045	1613	936	936
query69	429	314	251	251
query70	914	838	805	805
query71	296	229	223	223
query72	2640	2543	2237	2237
query73	826	759	420	420
query74	4613	4520	4279	4279
query75	2327	2302	1954	1954
query76	2326	1110	742	742
query77	364	405	316	316
query78	9054	9056	8461	8461
query79	1198	1209	743	743
query80	554	504	391	391
query81	530	331	291	291
query82	286	168	134	134
query83	321	231	207	207
query84	311	152	122	122
query85	956	461	390	390
query86	278	239	231	231
query87	1978	1982	1836	1836
query88	3619	2766	2761	2761
query89	329	292	249	249
query90	2054	184	186	184
query91	172	156	129	129
query92	103	82	88	82
query93	1353	1375	822	822
query94	520	342	295	295
query95	663	381	351	351
query96	1053	803	327	327
query97	2425	2420	2299	2299
query98	167	156	147	147
query99	725	746	613	613
Total cold run time: 236119 ms
Total hot run time: 153046 ms

@hello-stephen

Copy link
Copy Markdown
Contributor
ClickBench: Total hot run time: 24.15 s
machine: 'aliyun_ecs.c7a.8xlarge_32C64G'
scripts: https://github.com/apache/doris/tree/master/tools/clickbench-tools
ClickBench test result on commit 8b63d9889ca4e9ed505c2344042f504dd08afa3e, data reload: false

query1	0.01	0.01	0.01
query2	0.09	0.05	0.05
query3	0.25	0.14	0.13
query4	1.61	0.14	0.14
query5	0.26	0.22	0.22
query6	1.16	0.92	0.93
query7	0.04	0.01	0.00
query8	0.06	0.03	0.04
query9	0.40	0.33	0.34
query10	0.56	0.54	0.54
query11	0.20	0.14	0.15
query12	0.18	0.17	0.15
query13	0.47	0.46	0.46
query14	0.96	0.95	0.94
query15	0.60	0.59	0.59
query16	0.33	0.33	0.30
query17	1.10	1.10	1.08
query18	0.22	0.21	0.21
query19	2.04	1.97	1.96
query20	0.02	0.02	0.01
query21	15.48	0.21	0.14
query22	4.82	0.06	0.06
query23	16.13	0.30	0.11
query24	3.06	0.41	0.34
query25	0.12	0.05	0.04
query26	0.73	0.21	0.14
query27	0.04	0.04	0.03
query28	3.58	0.77	0.36
query29	12.48	3.95	3.19
query30	0.28	0.15	0.16
query31	2.77	0.54	0.32
query32	3.23	0.59	0.49
query33	3.17	3.24	3.27
query34	15.45	3.95	3.31
query35	3.23	3.23	3.25
query36	0.55	0.42	0.42
query37	0.09	0.07	0.06
query38	0.05	0.04	0.04
query39	0.04	0.03	0.03
query40	0.17	0.15	0.14
query41	0.08	0.03	0.03
query42	0.04	0.03	0.03
query43	0.04	0.04	0.04
Total cold run time: 96.19 s
Total hot run time: 24.15 s

morningman and others added 2 commits September 21, 2026 21:07
…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>
@morningman
morningman force-pushed the remote-doris-flight-session branch from 8b63d98 to 821fb3d Compare September 21, 2026 13:32
@morningman

Copy link
Copy Markdown
Contributor Author

skip buildall

hello-stephen
hello-stephen previously approved these changes Sep 21, 2026
@morningman

Copy link
Copy Markdown
Contributor Author

/review

924060929
924060929 previously approved these changes Sep 21, 2026
morningman and others added 2 commits September 22, 2026 00:42
…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>
@morningman

Copy link
Copy Markdown
Contributor Author

run buildall

@morningman

Copy link
Copy Markdown
Contributor Author

/review

@github-actions

Copy link
Copy Markdown
Contributor

Codex automated review failed and did not complete.

Error: Review recovery stopped: PR base/head or open state changed; refusing to resume stale context
Workflow run: https://github.com/apache/doris/actions/runs/35618711748

Please inspect the workflow logs and rerun the review after the underlying issue is resolved.

@hello-stephen

Copy link
Copy Markdown
Contributor
TPC-H: Total hot run time: 27687 ms
machine: 'aliyun_ecs.c7a.8xlarge_32C64G'
scripts: https://github.com/apache/doris/tree/master/tools/tpch-tools
Tpch sf100 test result on commit aaba58f2e6ea8f5a79145353902c359ddc2da670, data reload: false

------ Round 1 ----------------------------------
============================================
q1	17885	3818	3804	3804
q2	2112	364	300	300
q3	10134	1374	791	791
q4	4684	480	351	351
q5	7481	819	574	574
q6	180	165	134	134
q7	725	780	585	585
q8	9313	1598	1485	1485
q9	5463	4197	4189	4189
q10	6825	1355	1041	1041
q11	430	276	245	245
q12	634	418	294	294
q13	18031	2607	2036	2036
q14	262	274	234	234
q15	q16	742	713	655	655
q17	1740	1166	1044	1044
q18	6530	5648	5571	5571
q19	1333	1208	1082	1082
q20	487	400	262	262
q21	5702	2909	2711	2711
q22	425	353	299	299
Total cold run time: 101118 ms
Total hot run time: 27687 ms

----- Round 2, with runtime_filter_mode=off -----
============================================
q1	4185	4047	4061	4047
q2	713	570	516	516
q3	4528	4814	4330	4330
q4	2212	2307	1463	1463
q5	4217	4137	4114	4114
q6	220	172	126	126
q7	1688	1598	1405	1405
q8	2188	2383	2198	2198
q9	7567	7665	7650	7650
q10	3953	3651	3178	3178
q11	549	403	355	355
q12	736	733	518	518
q13	2525	2828	2157	2157
q14	294	304	262	262
q15	q16	695	705	663	663
q17	7735	7250	7114	7114
q18	11875	11336	11769	11336
q19	1173	1068	1068	1068
q20	2268	2215	1956	1956
q21	5322	4649	4494	4494
q22	543	494	403	403
Total cold run time: 65186 ms
Total hot run time: 59353 ms

@hello-stephen

Copy link
Copy Markdown
Contributor
TPC-DS: Total hot run time: 153084 ms
machine: 'aliyun_ecs.c7a.8xlarge_32C64G'
scripts: https://github.com/apache/doris/tree/master/tools/tpcds-tools
TPC-DS sf100 test result on commit aaba58f2e6ea8f5a79145353902c359ddc2da670, data reload: false

query5	4342	615	471	471
query6	440	224	216	216
query7	4936	544	286	286
query8	322	183	184	183
query9	8805	4009	4034	4009
query10	452	317	255	255
query11	5844	3548	3263	3263
query12	152	92	87	87
query13	1265	574	413	413
query14	6552	4558	4268	4268
query14_1	4015	3988	3974	3974
query15	205	200	180	180
query16	1027	504	474	474
query17	927	681	532	532
query18	2451	470	348	348
query19	202	181	143	143
query20	92	83	85	83
query21	220	134	119	119
query22	13086	13028	12787	12787
query23	14109	13180	12554	12554
query23_1	13069	12526	12490	12490
query24	7449	1172	705	705
query24_1	720	740	736	736
query25	552	414	360	360
query26	1273	326	164	164
query27	2652	564	316	316
query28	4575	1985	1974	1974
query29	1631	768	527	527
query30	292	216	175	175
query31	868	748	637	637
query32	147	90	90	90
query33	510	303	237	237
query34	1214	1170	626	626
query35	723	753	642	642
query36	778	760	698	698
query37	143	110	89	89
query38	1831	1768	1685	1685
query39	696	713	640	640
query39_1	659	659	628	628
query40	219	124	106	106
query41	101	62	61	61
query42	96	90	91	90
query43	353	350	303	303
query44	1402	700	720	700
query45	179	177	162	162
query46	1107	1159	744	744
query47	1486	1485	1398	1398
query48	401	384	301	301
query49	599	416	296	296
query50	899	331	245	245
query51	10758	10561	10709	10561
query52	89	87	76	76
query53	241	258	182	182
query54	244	205	205	205
query55	78	73	73	73
query56	247	221	225	221
query57	1548	1418	1385	1385
query58	291	265	256	256
query59	2052	2098	1890	1890
query60	281	241	230	230
query61	154	146	145	145
query62	394	318	264	264
query63	213	179	177	177
query64	2772	987	815	815
query65	3481	3423	3457	3423
query66	1797	405	299	299
query67	20127	19963	19839	19839
query68	3088	1541	939	939
query69	410	300	264	264
query70	921	800	794	794
query71	292	239	220	220
query72	2726	2470	2189	2189
query73	803	796	427	427
query74	4652	4536	4302	4302
query75	2325	2299	1931	1931
query76	2328	1110	765	765
query77	365	396	301	301
query78	8957	8987	8520	8520
query79	1209	1204	761	761
query80	516	450	377	377
query81	520	320	287	287
query82	274	165	125	125
query83	215	228	201	201
query84	284	146	113	113
query85	802	457	367	367
query86	298	247	237	237
query87	1981	1984	1851	1851
query88	3649	2750	2767	2750
query89	323	286	244	244
query90	2136	182	182	182
query91	167	152	123	123
query92	103	93	90	90
query93	1332	1478	827	827
query94	526	351	301	301
query95	653	371	339	339
query96	1072	793	340	340
query97	2436	2428	2358	2358
query98	162	153	146	146
query99	716	735	623	623
Total cold run time: 236732 ms
Total hot run time: 153084 ms

@hello-stephen

Copy link
Copy Markdown
Contributor
ClickBench: Total hot run time: 24.07 s
machine: 'aliyun_ecs.c7a.8xlarge_32C64G'
scripts: https://github.com/apache/doris/tree/master/tools/clickbench-tools
ClickBench test result on commit aaba58f2e6ea8f5a79145353902c359ddc2da670, data reload: false

query1	0.01	0.01	0.01
query2	0.08	0.05	0.06
query3	0.26	0.13	0.13
query4	1.61	0.14	0.13
query5	0.24	0.22	0.22
query6	1.15	1.00	0.95
query7	0.05	0.01	0.01
query8	0.06	0.04	0.04
query9	0.38	0.34	0.33
query10	0.56	0.55	0.53
query11	0.20	0.14	0.14
query12	0.19	0.15	0.15
query13	0.48	0.48	0.47
query14	0.95	0.95	0.94
query15	0.61	0.59	0.59
query16	0.31	0.32	0.31
query17	1.11	1.12	1.09
query18	0.21	0.21	0.20
query19	1.96	1.92	1.89
query20	0.02	0.02	0.01
query21	15.48	0.22	0.13
query22	4.65	0.04	0.05
query23	16.15	0.31	0.12
query24	2.97	0.40	0.35
query25	0.12	0.05	0.04
query26	0.72	0.20	0.15
query27	0.03	0.03	0.04
query28	3.56	0.72	0.36
query29	12.49	3.97	3.20
query30	0.27	0.15	0.15
query31	2.78	0.57	0.32
query32	3.22	0.59	0.48
query33	3.26	3.21	3.20
query34	15.46	3.91	3.29
query35	3.26	3.26	3.27
query36	0.55	0.44	0.41
query37	0.09	0.07	0.06
query38	0.05	0.04	0.03
query39	0.04	0.03	0.03
query40	0.17	0.15	0.13
query41	0.09	0.04	0.04
query42	0.04	0.03	0.03
query43	0.04	0.04	0.03
Total cold run time: 95.93 s
Total hot run time: 24.07 s

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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());

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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()

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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())

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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.

@hello-stephen

Copy link
Copy Markdown
Contributor

FE UT Coverage Report

Increment line coverage 78.63% (92/117) 🎉
Increment coverage report
Complete coverage report

@hello-stephen

Copy link
Copy Markdown
Contributor

FE Regression Coverage Report

Increment line coverage 65.81% (77/117) 🎉
Increment coverage report
Complete coverage report

@morningman
morningman merged commit 14c957e into apache:master Sep 22, 2026
52 of 68 checks passed
morningman added a commit to morningman/doris that referenced this pull request Sep 22, 2026
…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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants