github-actions[bot] commented on code in PR #64799:
URL: https://github.com/apache/doris/pull/64799#discussion_r3604763785
##########
fe/fe-core/src/main/java/org/apache/doris/qe/ConnectContext.java:
##########
@@ -934,6 +934,40 @@ public void clear() {
statementContext = null;
}
+ // Arrow Flight SQL only.
+ // Executors of already-planned queries whose results are produced on the
BE and pulled later
+ // during the DoGet phase. Their coordinators must stay alive until the BE
finishes scanning:
+ // an external-table scan in batch mode lazily fetches splits from the FE
(a batch SplitSource
+ // held by the coordinator's scan nodes), so closing the coordinator at
the end of
+ // GetFlightInfo would release the SplitSource too early and make the BE's
fetchSplitBatch fail
+ // with "Split source X is released". These executors are finalized when
the next query starts
+ // on this connection, or when the connection is torn down. See #62259.
+ private final List<StmtExecutor> flightSqlDeferredExecutors = new
ArrayList<>();
+
+ public void addFlightSqlDeferredExecutor(StmtExecutor executor) {
+ synchronized (flightSqlDeferredExecutors) {
+ flightSqlDeferredExecutors.add(executor);
Review Comment:
**[P1] Make terminal teardown atomic with deferred registration**
Token eviction or `CloseSession` can call `unregisterConnection()` on
another thread while this query is still inside `coordBase.exec()`. If teardown
drains the currently empty list and removes the context/token mapping first,
the query then returns, adds itself here, and
`FlightSqlConnectProcessor.close()` skips it because it is deferred. No later
request or timeout can retrieve that removed context, so its Qe
registration/instance accounting, queue token, coordinator, and batch
SplitSources remain indefinitely. The monitor prevents list corruption but does
not close this drain-then-add window. Please mark terminal teardown under the
same synchronization and make a late add finalize/reject the executor, with a
latch test for this ordering.
##########
fe/fe-core/src/main/java/org/apache/doris/qe/StmtExecutor.java:
##########
@@ -1470,6 +1491,22 @@ public void executeAndSendResult(boolean isOutfileQuery,
boolean isSendFields,
if (context.getConnectType().equals(ConnectType.ARROW_FLIGHT_SQL))
{
Preconditions.checkState(!context.isReturnResultFromLocal());
profile.getSummaryProfile().setTempStartTime();
+ // Defer closing the coordinator to ConnectContext (closed on
the next query or
+ // connection teardown) instead of in the finally block below.
This gate covers
+ // every Arrow Flight query whose results are produced on the
BE (coordBase ==
+ // coord) -- internal-table and external, batch or not. It is
REQUIRED only for an
+ // external-table scan in batch mode, where the BE lazily
fetches splits from the FE
+ // during the later DoGet phase, so closing the coordinator
here would release its
+ // batch SplitSource too early and break DoGet. Other
remote-result queries do not
+ // need deferral (the BE buffers their result independently)
but are captured by the
+ // same gate; the trade-off is their coordinator, query queue
slot and query
+ // registration stay held until the next query / teardown
instead of being released
+ // at the end of GetFlightInfo. Point queries use a different
coordBase (not
+ // deferred). See #62259.
+ if (coordBase == coord) {
Review Comment:
**[P1] Do not leave every BE-result query attached to the Flight session**
This gate also defers internal-table and non-batch queries even though only
an external batch scan needs a live `SplitSource`. Skipping
`Coordinator.close()` retains the workload-group token, and delaying
`unregisterQuery()` retains per-user instance accounting (and Hive transaction
deregistration). After the client fully drains `DoGet`, the FE receives no
completion callback. The Arrow Java 19 driver used by the new regression sends
`CloseSession` from `close()` only when an optional catalog is present
([source](https://github.com/apache/arrow-java/blob/v19.0.0/flight/flight-sql-jdbc-core/src/main/java/org/apache/arrow/driver/jdbc/client/ArrowFlightSqlClientHandler.java#L270-L279));
this URL has none, so without another statement the resources remain until the
default 28,800-second idle timeout. With workload-group max concurrency 1, one
completed q1 therefore leaves q2 on another session queued for up to eight
hours. Please restrict deferral to plans that actually own batch Spli
tSources and provide completion-driven cleanup/decoupled ownership for those
plans; add a two-session queue-release test.
##########
fe/fe-core/src/main/java/org/apache/doris/qe/StmtExecutor.java:
##########
@@ -1035,6 +1039,23 @@ public void finalizeQuery() {
QeProcessorImpl.INSTANCE.unregisterQuery(context.queryId());
}
+ public boolean isDeferredForArrowFlight() {
+ return deferredForArrowFlight;
+ }
+
+ // Finalize an Arrow Flight query whose coordinator was kept alive across
the
+ // GetFlightInfo -> DoGet phases: close the coordinator (releasing
external-table batch
+ // SplitSources and the query queue slot) and then unregister the query.
See #62259.
+ public void finalizeArrowFlightQuery() {
+ try {
+ if (coord != null) {
+ coord.close();
+ }
+ } finally {
+ finalizeQuery();
Review Comment:
**[P1] Unregister the deferred executor's captured query ID**
This calls `finalizeQuery()`, which removes `context.queryId()` rather than
the ID owned by this executor. `closeFlightSqlDeferredExecutors()` clears q1
from the list and finalizes it outside the monitor; if token teardown races an
already-admitted next request, q2 can see the empty list and `handleQuery()`
can reset/set the shared ID before q1 reaches this line. Q1 then remains
registered (or the null removal throws), while q2 may be unregistered and lose
status/profile/cancel routing; instance accounting and Hive transaction
deregistration use the same wrong key. Please capture the immutable query ID
when this executor is registered/deferred and finalize that exact ID, with a
blocked-q1-close/q2-ID latch test.
##########
fe/fe-core/src/main/java/org/apache/doris/service/arrowflight/FlightSqlConnectProcessor.java:
##########
@@ -196,11 +196,20 @@ public void fetchArrowFlightSchema(int timeoutMs) {
@Override
public void close() throws Exception {
ctx.setCommand(MysqlCommand.COM_SLEEP);
+ // Executors whose results are pulled from the BE keep their
coordinator alive past
+ // GetFlightInfo (registered as deferred executors on the
ConnectContext) so the BE can
+ // still fetch external-table splits during DoGet. Do NOT finalize
those here; they are
+ // finalized when the next query starts or the connection is torn
down. Executors that are
+ // not deferred (local results, or a query that already failed) are
finalized now. See #62259.
for (StmtExecutor asynExecutor : returnResultFromRemoteExecutor) {
- asynExecutor.finalizeQuery();
+ if (!asynExecutor.isDeferredForArrowFlight()) {
+ asynExecutor.finalizeQuery();
+ }
}
returnResultFromRemoteExecutor.clear();
- executor.finalizeQuery();
+ if (executor != null && !executor.isDeferredForArrowFlight()) {
Review Comment:
**[P1] Keep deferred queries reachable by cancellation**
This branch keeps q1 active for the later `DoGet`, but `ctx.clear()`
immediately nulls `ConnectContext.executor`. During `DoGet`, query-ID lookup
still finds this context; however, `KILL QUERY` calls
`FlightSqlConnectContext.kill(false)` -> `cancelQuery()`, which now sees no
executor and does nothing, after which KILL reports success. REST and
workload-policy cancellation use the same no-op path, and none falls back to
the retained Qe coordinator/private deferred list, so the BE fragments
continue. Please keep deferred executors addressable by their captured query ID
(also aligning with stable ownership in the other finding) and route
cancellation to the matching executor/coordinator; add a test that closes the
processor, cancels q1 during DoGet, and verifies coordinator cancellation plus
later single finalization.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
