Uh oh!
There was an error while loading. Please reload this page.
[fix](arrow-flight) Keep coordinator alive across GetFlightInfo/DoGet for external table scan - #64799
Merged
morningman merged 6 commits intoJul 18, 2026
Conversation
hello-stephen
commented
Jun 24, 2026
Contributor
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
morningman
commented
Jun 24, 2026
ContributorAuthor
run buildall |
hello-stephen
commented
Jun 24, 2026
Contributor
TPC-H: Total hot run time: 28681 ms |
hello-stephen
commented
Jun 24, 2026
Contributor
TPC-DS: Total hot run time: 172304 ms |
hello-stephen
commented
Jun 24, 2026
Contributor
ClickBench: Total hot run time: 25.16 s |
hello-stephen
commented
Jun 24, 2026
Contributor
FE Regression Coverage ReportIncrement line coverage |
morningman
commented
Jun 25, 2026
ContributorAuthor
run buildall |
hello-stephen
commented
Jun 25, 2026
Contributor
TPC-H: Total hot run time: 29147 ms |
hello-stephen
commented
Jun 25, 2026
Contributor
TPC-DS: Total hot run time: 173041 ms |
hello-stephen
commented
Jun 25, 2026
Contributor
ClickBench: Total hot run time: 25.31 s |
hello-stephen
commented
Jun 25, 2026
Contributor
FE Regression Coverage ReportIncrement line coverage |
morningman
commented
Jun 25, 2026
ContributorAuthor
run buildall |
hello-stephen
commented
Jun 25, 2026
Contributor
TPC-H: Total hot run time: 29708 ms |
hello-stephen
commented
Jun 25, 2026
Contributor
TPC-DS: Total hot run time: 171061 ms |
hello-stephen
commented
Jun 25, 2026
Contributor
ClickBench: Total hot run time: 25.03 s |
morningmanforce-pushed
the
fix-62259-arrow-flight-split-source-lifecycle
branch
from
June 25, 2026 11:09
eecedcb to
cd50205Comparemorningman
commented
Jun 25, 2026
ContributorAuthor
run buildall |
hello-stephen
commented
Jun 25, 2026
Contributor
FE UT Coverage ReportIncrement line coverage |
hello-stephen
commented
Jun 25, 2026
Contributor
TPC-H: Total hot run time: 29023 ms |
hello-stephen
commented
Jun 25, 2026
Contributor
TPC-DS: Total hot run time: 172555 ms |
hello-stephen
commented
Jun 25, 2026
Contributor
ClickBench: Total hot run time: 25.23 s |
hello-stephen
commented
Jun 25, 2026
Contributor
FE Regression Coverage ReportIncrement line coverage |
hello-stephen
commented
Jun 26, 2026
Contributor
FE Regression Coverage ReportIncrement line coverage |
morningman
commented
Jun 26, 2026
ContributorAuthor
/review |
Contributor
There was a problem hiding this comment.
I found two correctness issues in the Arrow Flight remote-result lifecycle changes.
Critical checkpoints: the PR targets the right split-source lifetime problem, but the current cleanup trigger is still not safe for concurrent/interleaved Flight RPCs and it misses a failed-GetFlightInfo cleanup path. The change is small and focused, and it does not add configs, persistence, storage-format changes, or FE/BE protocol fields. It does involve non-trivial lifecycle and concurrency: deferred coordinators hold query queue slots and external-table split sources after GetFlightInfo, while DoGet runs independently against BE endpoints. The tests cover normal cleanup and one external-table scan, but they do not cover an overlapping second statement or schema-fetch failure after deferral.
Validation: no existing inline review threads were present; git diff --check over the GitHub changed-file list was clean. I did not run FE tests because thirdparty/installed/bin/protoc is missing in this checkout.
User focus: no additional user-provided review focus was supplied.
Subagent conclusions: OPT-1 became MAIN-1 and is submitted as an inline comment; TSC-1 became MAIN-2 and is submitted as an inline comment. No candidates were dismissed or merged as duplicates. Convergence round 1 ended with both optimizer-rewrite and tests-session-config replying NO_NEW_VALUABLE_FINDINGS for the same current ledger/comment set.
| // deferred (Arrow Flight keeps it alive across GetFlightInfo -> DoGet so the BE can | ||
| // fetch external-table splits during DoGet). By now the previous DoGet is done. #62259 | ||
| connectContext.closeFlightSqlDeferredExecutors(); | ||
| // After the previous query was executed, there was no getStreamStatement to take away the result. |
Contributor
There was a problem hiding this comment.
The new cleanup assumes that starting the next statement means the previous DoGet has finished, but Flight RPCs are independent and this method has no completion signal from the BE result stream. A client can get FlightInfo for q1, keep reading q1's BE endpoint, and issue GetFlightInfo for q2 on the same session; this call will close q1's deferred coordinator. Closing the coordinator stops the scan nodes and removes the batch split sources, while q1's BE scan can still call fetchSplitBatch, which then fails with Split source X is released. This reintroduces the issue for pipelined/interleaved clients. Please keep the deferred coordinator until the result stream is actually complete or the session is closed, or reject/serialize a new statement while a remote Arrow Flight result is still live.
ContributorAuthor
There was a problem hiding this comment.
I looked into this and don't think it's a regression this PR introduces, nor something that can be properly fixed within this PR's scope. Details:
Not a new failure mode / not a regression. Before this PR, every external-table batch-split query over Arrow Flight already failed with Split source X is released, because the coordinator was closed at the end of GetFlightInfo — sequential queries included (that is exactly #62259). After this PR the sequential case works; the interleaved case you describe fails as it did before. So nothing is "reintroduced" — interleaving never worked.
Interleaving two queries on one session is not a supported usage. A bearer token maps to a single shared ConnectContext (FlightSessionsWithTokenManager), and that context is single-query-by-construction: each GetFlightInfo resets the shared FlightSqlChannel, clears the single flightSqlEndpointsLocations list, and reuses one queryId. Two concurrent/interleaved queries on one session corrupt all of that, not just the deferred coordinator — the same way sharing one JDBC Connection across threads is unsafe. The supported pattern (drain the result, then issue the next query) makes "the previous DoGet is done by the time the next query starts" hold, since the BE has consumed the SplitSource by the time the client reaches end-of-stream.
The suggested fix needs a signal the FE does not have. For an external-table scan the result DoGet endpoint points directly at the BE; the FE is not in that data path and gets no completion signal when the BE-side stream finishes. "Keep the coordinator until the result stream completes" or "serialize/reject a new statement while a result is live" both require a new BE -> FE completion notification (or session-level serialization keyed on it), which is a separate architectural change out of scope here.
Leaks are already bounded. The "or the session is closed" fallback is implemented: FlightSqlConnectPoolMgr.unregisterConnection() -> closeFlightSqlDeferredExecutors() covers idle/query timeout, bearer-token expiry and explicit CloseSession. And the BE-side hardening in #64797 makes a stale fetchSplitBatch fail gracefully instead of crashing the BE.
If first-class concurrent Flight clients are ever needed, the right follow-up is a BE -> FE completion signal plus session serialization; I can track that separately.
| asynExecutor.finalizeQuery(); | ||
| if (!asynExecutor.isDeferredForArrowFlight()) { | ||
| asynExecutor.finalizeQuery(); | ||
| } |
Contributor
There was a problem hiding this comment.
This guard also skips cleanup when GetFlightInfo fails after a remote executor has already been marked deferred. In that flow, StmtExecutor.executeAndSendResult() adds the executor to the deferred list after coordBase.exec(), then executeQueryStatement() still has to fetch the Arrow schema. If that schema fetch times out, returns non-OK/empty/mismatched schema, or hits an RPC error, try-with-resources calls this close(), but both deferred executors are skipped and the outer catch only rethrows. No FlightInfo is returned, so no DoGet can need the coordinator, but the query registration, query queue slot, and split sources stay alive until a later query or session/token teardown. Please explicitly close the deferred executors on this error path, and add a test that fails schema fetch after deferral.
ContributorAuthor
There was a problem hiding this comment.
Confirmed — this is a real (bounded) leak. If the Arrow schema fetch in executeQueryStatement fails after the coordinator was already deferred during planning, FlightSqlConnectProcessor.close() skips the deferred executor and the outer catch only rethrows, so the coordinator (its external-table batch SplitSource, the query queue slot and the query registration) stays alive until the next query starts or the connection is torn down — even though no DoGet will ever pull this query's results.
Fixed in 41a79a6 by finalizing the deferred coordinator on the GetFlightInfo error path: executeQueryStatement's catch now calls connectContext.closeFlightSqlDeferredExecutors(). At that point the list holds only this failed query's coordinator (the previous one was already finalized at the top of the method), and it covers every post-deferral failure, not just the schema fetch.
Added DorisFlightSqlProducerTest.testGetFlightInfoFinalizesDeferredExecutorWhenSchemaFetchFails, which defers a coordinator then fails the schema fetch and asserts the deferred executor is finalized (verified to fail without the fix).
morningman
commented
Jun 28, 2026
ContributorAuthor
run buildall |
hello-stephen
commented
Jun 28, 2026
Contributor
FE UT Coverage ReportIncrement line coverage |
hello-stephen
commented
Jun 28, 2026
Contributor
TPC-H: Total hot run time: 29096 ms |
hello-stephen
commented
Jun 28, 2026
Contributor
TPC-DS: Total hot run time: 172666 ms |
Contributor
PR approved by at least one committer and no changes requested. |
Contributor
PR approved by anyone and no changes requested. |
Uh oh!
There was an error while loading. Please reload this page.
yiguolei pushed a commit
to yiguolei/incubator-doris
that referenced
this pull request
Aug 5, 2026
… for external table scan (apache#64799) Issue Number: closeapache#62259 Related PR: apache#64797 Problem Summary: Arrow Flight SQL queries against Iceberg (and other external) tables in batch split mode crashed the BE / failed with `Split source X is released`. Arrow Flight executes a query in two phases: `GetFlightInfo` (plan + submit to BE) and `DoGet` (the client pulls results from the BE). For an external table scan in batch split mode, the BE keeps scanning during `DoGet` and lazily fetches file splits from the FE via the `fetchSplitBatch` RPC, using an async `SplitSource` that the FE coordinator holds (through its scan nodes). The FE closed the coordinator at the end of `GetFlightInfo` (`StmtExecutor.executeAndSendResult`'s `finally` → `Coordinator.close()` → `ScanNode.stop()` → `SplitSourceManager.removeSplitSource()`) and also unregistered it (`FlightSqlConnectProcessor.close()` → `StmtExecutor.finalizeQuery()`). So by the time the BE called `fetchSplitBatch` during `DoGet`, the `SplitSource` was already gone. The MySQL protocol is unaffected because plan + execute share one request, so the coordinator stays alive until all results are consumed. This PR keeps the coordinator (and its `SplitSource`) alive across the two phases and cleans it up reliably: - **StmtExecutor**: for an Arrow Flight query that produces results on the BE (`coordBase == coord`), mark it deferred, register the executor on the `ConnectContext`, and skip the eager `Coordinator.close()` in the `finally`. A failed query (whose `exec()` threw) is not deferred and is closed as before. - **ConnectContext**: hold the deferred executors and add `closeFlightSqlDeferredExecutors()`, which closes their coordinators (releasing the `SplitSource` and the query queue slot) and unregisters the queries. - **FlightSqlConnectProcessor.close()**: do not finalize deferred executors. - **DorisFlightSqlProducer**: finalize the previous query's deferred coordinator when the next query starts on the connection. - **FlightSqlConnectPoolMgr.unregisterConnection()**: finalize deferred coordinators when the connection is torn down. All teardown paths (idle/query timeout, bearer token expiry, explicit `CloseSession`) reach here, so an abandoned connection cannot leak the coordinator. Non-Arrow-Flight paths (MySQL, internal tables, point queries) are unchanged: `deferredForArrowFlight` can only become true for `ARROW_FLIGHT_SQL`. The BE-side error-path hardening (so any `fetchSplitBatch` failure fails gracefully instead of crashing the BE) is handled separately in apache#64797.
16 tasks
morningman pushed a commit
that referenced
this pull request
Aug 5, 2026
wyxxxcat pushed a commit
to wyxxxcat/doris
that referenced
this pull request
Aug 17, 2026
… for external table scan (apache#64799) ### What problem does this PR solve? Issue Number: closeapache#62259 Related PR: apache#64797 Problem Summary: Arrow Flight SQL queries against Iceberg (and other external) tables in batch split mode crashed the BE / failed with `Split source X is released`. Arrow Flight executes a query in two phases: `GetFlightInfo` (plan + submit to BE) and `DoGet` (the client pulls results from the BE). For an external table scan in batch split mode, the BE keeps scanning during `DoGet` and lazily fetches file splits from the FE via the `fetchSplitBatch` RPC, using an async `SplitSource` that the FE coordinator holds (through its scan nodes). The FE closed the coordinator at the end of `GetFlightInfo` (`StmtExecutor.executeAndSendResult`'s `finally` → `Coordinator.close()` → `ScanNode.stop()` → `SplitSourceManager.removeSplitSource()`) and also unregistered it (`FlightSqlConnectProcessor.close()` → `StmtExecutor.finalizeQuery()`). So by the time the BE called `fetchSplitBatch` during `DoGet`, the `SplitSource` was already gone. The MySQL protocol is unaffected because plan + execute share one request, so the coordinator stays alive until all results are consumed. This PR keeps the coordinator (and its `SplitSource`) alive across the two phases and cleans it up reliably: - **StmtExecutor**: for an Arrow Flight query that produces results on the BE (`coordBase == coord`), mark it deferred, register the executor on the `ConnectContext`, and skip the eager `Coordinator.close()` in the `finally`. A failed query (whose `exec()` threw) is not deferred and is closed as before. - **ConnectContext**: hold the deferred executors and add `closeFlightSqlDeferredExecutors()`, which closes their coordinators (releasing the `SplitSource` and the query queue slot) and unregisters the queries. - **FlightSqlConnectProcessor.close()**: do not finalize deferred executors. - **DorisFlightSqlProducer**: finalize the previous query's deferred coordinator when the next query starts on the connection. - **FlightSqlConnectPoolMgr.unregisterConnection()**: finalize deferred coordinators when the connection is torn down. All teardown paths (idle/query timeout, bearer token expiry, explicit `CloseSession`) reach here, so an abandoned connection cannot leak the coordinator. Non-Arrow-Flight paths (MySQL, internal tables, point queries) are unchanged: `deferredForArrowFlight` can only become true for `ARROW_FLIGHT_SQL`. The BE-side error-path hardening (so any `fetchSplitBatch` failure fails gracefully instead of crashing the BE) is handled separately in apache#64797.
yiguolei added a commit
to yiguolei/incubator-doris
that referenced
this pull request
Aug 18, 2026
) ### What problem does this PR solve? pick apache#64799apache#66437
16 tasks
yiguolei added a commit
to yiguolei/incubator-doris
that referenced
this pull request
Aug 19, 2026
) ### What problem does this PR solve? pick apache#64799apache#66437
yiguolei added a commit
that referenced
this pull request
Aug 20, 2026
…66871) ### What problem does this PR solve? pick #64799#66437 ### What problem does this PR solve? Issue Number: close #xxx Related PR: #xxx Problem Summary: ### Release note None ### Check List (For Author) - Test <!-- At least one of them must be included. --> - [ ] 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 <!-- Add your reason? --> - Behavior changed: - [ ] No. - [ ] Yes. <!-- Explain the behavior change --> - Does this need documentation? - [ ] No. - [ ] Yes. <!-- Add document PR link here. eg: apache/doris-website#1214 --> ### Check List (For Reviewer who merge this PR) - [ ] Confirm the release note - [ ] Confirm test cases - [ ] Confirm document - [ ] Add branch pick label <!-- Add branch pick label that this PR should merge into -->
morningman added a commit
to raghav-reglobe/doris
that referenced
this pull request
Sep 5, 2026
…instead of killing the session Follow-up to the first revision of apache#67504 after review: - Narrow the apache#64799 deferral gate: only a coordinator that still hands out splits to the BE (an external-table scan in batch mode, see the new ScanNode/Coordinator.hasBatchSplitSource) outlives GetFlightInfo. Every other Arrow Flight query closes its coordinator at the end of GetFlightInfo again, releasing the workload-group queue slot and the active_queries entry right away. Finalizing the FE side does not cancel BE execution, so DoGet is unaffected. - Replace arrow_flight_session_idle_timeout_second by arrow_flight_deferred_query_idle_timeout_second: the connection timeout checker now finalizes the deferred executors of a sleeping Flight session and leaves the session alive; wait_timeout still governs the session. A killed session would have made the client's next call fail with "UserSession expire after access". - Freeze the execution timeout when the executor is deferred, so a SET_VAR query_timeout hint (reverted at the end of execute()) still floors the bound. - Tests: FlightSqlDeferredQueryIdleTimeoutTest drives checkTimeout, ArrowFlightDeferralGateTest covers the predicates, StmtExecutorTest covers the frozen timeout; regression cases for the internal-table release (arrow_flight_sql_p0) and the idle reaper on a batch-mode Iceberg scan. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01ApxvNkMK8TiNycv44P8Ezk
morningman added a commit
to raghav-reglobe/doris
that referenced
this pull request
Sep 5, 2026
…instead of killing the session Follow-up to the first revision of apache#67504 after review: - Narrow the apache#64799 deferral gate: only a coordinator that still hands out splits to the BE (an external-table scan in batch mode, see the new ScanNode/Coordinator.hasBatchSplitSource) outlives GetFlightInfo. Every other Arrow Flight query closes its coordinator at the end of GetFlightInfo again, releasing the workload-group queue slot and the active_queries entry right away. Finalizing the FE side does not cancel BE execution, so DoGet is unaffected. - Replace arrow_flight_session_idle_timeout_second by arrow_flight_deferred_query_idle_timeout_second: the connection timeout checker now finalizes the deferred executors of a sleeping Flight session and leaves the session alive; wait_timeout still governs the session. A killed session would have made the client's next call fail with "UserSession expire after access". - Freeze the execution timeout when the executor is deferred, so a SET_VAR query_timeout hint (reverted at the end of execute()) still floors the bound. - Tests: FlightSqlDeferredQueryIdleTimeoutTest drives checkTimeout, ArrowFlightDeferralGateTest covers the predicates, StmtExecutorTest covers the frozen timeout; regression cases for the internal-table release (arrow_flight_sql_p0) and the idle reaper on a batch-mode Iceberg scan. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01ApxvNkMK8TiNycv44P8Ezk
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What problem does this PR solve?
Issue Number: close#62259
Related PR: #64797
Problem Summary:
Arrow Flight SQL queries against Iceberg (and other external) tables in batch split mode crashed the BE / failed with
Split source X is released.Arrow Flight executes a query in two phases:
GetFlightInfo(plan + submit to BE) andDoGet(the client pulls results from the BE). For an external table scan in batch split mode, the BE keeps scanning duringDoGetand lazily fetches file splits from the FE via thefetchSplitBatchRPC, using an asyncSplitSourcethat the FE coordinator holds (through its scan nodes).The FE closed the coordinator at the end of
GetFlightInfo(StmtExecutor.executeAndSendResult'sfinally→Coordinator.close()→ScanNode.stop()→SplitSourceManager.removeSplitSource()) and also unregistered it (FlightSqlConnectProcessor.close()→StmtExecutor.finalizeQuery()). So by the time the BE calledfetchSplitBatchduringDoGet, theSplitSourcewas already gone. The MySQL protocol is unaffected because plan + execute share one request, so the coordinator stays alive until all results are consumed.This PR keeps the coordinator (and its
SplitSource) alive across the two phases and cleans it up reliably:coordBase == coord), mark it deferred, register the executor on theConnectContext, and skip the eagerCoordinator.close()in thefinally. A failed query (whoseexec()threw) is not deferred and is closed as before.closeFlightSqlDeferredExecutors(), which closes their coordinators (releasing theSplitSourceand the query queue slot) and unregisters the queries.CloseSession) reach here, so an abandoned connection cannot leak the coordinator.Non-Arrow-Flight paths (MySQL, internal tables, point queries) are unchanged:
deferredForArrowFlightcan only become true forARROW_FLIGHT_SQL.The BE-side error-path hardening (so any
fetchSplitBatchfailure fails gracefully instead of crashing the BE) is handled separately in #64797.Release note
Fix Arrow Flight SQL queries against external tables (e.g. Iceberg) failing with
Split source X is releasedor crashing the BE in batch split mode.Check List (For Author)
Added
regression-test/suites/external_table_p0/iceberg/test_iceberg_arrow_flight_split_source.groovy. It forces batch split mode on the Arrow Flight session (num_files_in_batch_mode=1), asserts viaexplainthat the scan really uses the batchSplitSourcepath (approximate) so it cannot silently pass on the non-batch path, then scansformat_v2.sample_cow_orcover Arrow Flight and checks all rows come back. The test runs in the external (docker) pipeline and is skipped when the Iceberg env or the Arrow Flight endpoint is not configured.Behavior changed:
SplitSource) is now kept alive until the next query starts on the connection or the connection is torn down, instead of being closed at the end ofGetFlightInfo.Does this need documentation?
🤖 Generated with Claude Code