Repository navigation
[fix](be) Cancel Arrow Flight queries on the result BE when a result stream closes early - #69945
Conversation
…stream closes early ### What problem does this PR solve? Issue Number: None Related PR: apache#68630, apache#67644 Problem Summary: Port the BE-local half of branch-4.x apache#68630 to master. An Arrow Flight DoGet stream that the client cancels or closes before EOF left its query running. The local reader waited on an empty result buffer without looking at the RPC's cancellation state, and closing the stream did not cancel anything: the abandoned buffer kept its unread blocks until ResultBufferMgr dropped it, at execution_timeout + 5 s or result_buffer_cancelled_interval_time (300 s) after its fragments finished, and when the result did not fit it back-pressured the result sink until the query timed out. A remote reader blocked in a BRPC join that nothing could interrupt, an aborting cancel_plan_fragment left the query's unread Flight buffers in place until that delayed cleanup, and fetch_arrow_data for a buffer that was already gone never ran its done closure, so the caller waited for the BRPC timeout. The reader now checks ServerCallContext::is_cancelled between bounded 20 ms buffer waits and while a BRPC fetch is pending (cancelling it with brpc::StartCancel), and closes itself on Close(), on a read error and in its destructor. Closing a local reader before EOF cancels the query on this BE: ResultBufferMgr keeps a query -> Flight buffers index, removes every Flight buffer of the query and cancels its QueryContext outside the buffer-map lock, even after that context has expired. cancel_plan_fragment with an abort reason (not LIMIT_REACH or FINISHED) also drops the query's Flight buffers, and fetch_arrow_data completes the RPC with an error when the buffer is gone (the same line as apache#67644). The FE route that reaches the query's other BEs (cancelFlightQuery and the BE broadcast) is left to a follow-up PR. Differences from branch-4.x: the two index/map consistency checks are DORIS_CHECK, and the ArrowFlightResultBlockBuffer constructor moved to the .cpp so that varrow_flight_result_writer.h, which 13 TUs include, does not include runtime/query_context.h; only the .cpp needs its definition. ### Release note Arrow Flight SQL: cancelling or closing a result stream before its end now cancels the query on the BE that holds the stream's results and releases its unread results right away, instead of keeping them for minutes. ### Check List (For Author) - Test: Unit Test / Manual test - BE UT: ArrowFlightCancelTest (9, new), ArrowFlightRemoteCancelTest (3, new), ResultBufferMgrTest (6), ArrowResultBlockBufferTest (4) - Manual test on a local single-BE cluster: branch-4.x's test_flight_cancel_cleanup fails before and passes after; arrow_flight_sql_p0 and external_table_p0/remote_doris pass - Behavior changed: Yes. A Flight result stream closed before EOF on the BE that holds its results cancels its query on that BE, and a later DoGet with the same ticket fails instead of resuming. - Does this need documentation: No Co-authored-by: Gabriel <liwenqiang@selectdb.com> Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
|
run buildall |
|
/review |
There was a problem hiding this comment.
Static review of the full PR diff found one verified issue.
The new Arrow Flight reader state machine preserves the important local behaviors: bounded empty-buffer waits remain distinct from EOS, client cancellation interrupts local waits and outbound BRPC joins, and Close/destruction is idempotent. ResultBufferMgr's query-to-buffer index is updated under the buffer-map lock, sibling buffers are removed before cancellation runs outside that lock, and cancel_plan_fragment preserves unread results for LIMIT_REACH and FINISHED while clearing aborts. Missing-buffer fetches now complete their RPC with an error. I found no additional production issue in those paths.
[P2] The new ArrowFlightCancelTest fixture leaves a global result_buffer_block_count metric hook capturing a destroyed ResultBufferMgr after TearDown. Later metric collection can dereference freed storage, and later fixtures cannot replace the hook because registration uses emplace. This needs scoped hook cleanup or a fixture design that does not leave an unregistered manager behind.
The user supplied no additional focus points. The PR explicitly documents the FE query-wide cancellation route as a follow-up; remote DoGet cancellation currently interrupts the proxy's BRPC fetch but does not cancel result BEs reached through that route, so that remains an acknowledged scope limit.
Review convergence: false. This delivery is at the three-round cap. The required delegated subagent results were unavailable because each invocation hit a model-capacity error, so the formal all-subagents convergence condition was not met; the verified finding is still delivered. Static review only; no builds or tests were executed by this review.
Review did not converge after 3 rounds. This automated review cannot pass the review check.
Convergence was not reached at the three-round cap: all three assigned subagent invocations failed with model-capacity errors, so the required latest-round NO_NEW_VALUABLE_FINDINGS results are unavailable. The cross-BE FE cancellation route is also explicitly outside this BE-only PR and remains a documented follow-up scope limit.
| return _mgr.find_buffer(_id, found).ok(); | ||
| } | ||
| MockRuntimeState _state; | ||
| ResultBufferMgr _mgr; |
There was a problem hiding this comment.
[P2] Unregister the result-buffer metric hook before this fixture's manager is destroyed. ResultBufferMgr registers a global result_buffer_block_count callback that captures this, while TearDown() restores only ExecEnv::_result_mgr; the _mgr member is then destroyed, leaving metric scrapes with a dangling callback. Subsequent fixtures also cannot replace the hook because registration uses emplace. Scope cleanup to the hook owned by this fixture (preserving any prior hook) or avoid constructing an unregistered manager per test.
There was a problem hiding this comment.
Fixed in e5736f5. ArrowFlightCancelTest::TearDown() now calls _mgr.stop() after restoring the previous ExecEnv::_result_mgr, so the result_buffer_block_count hook and its gauge are deregistered before the fixture destroys the manager, the same way LoadChannelMgrTest::TearDown() stops its LoadChannelMgr. The fixture never calls init(), so stop() has no cleaner thread to join. Each fixture now registers its own hook and removes it again, so a later fixture no longer finds a stale one; ArrowFlightRemoteCancelTest gets the same cleanup through ArrowFlightCancelTest::TearDown().
On the impact: nothing in doris_be_test runs the server entity's hooks today. enable_metric_calculator defaults to true, so to_prometheus() and to_json() skip the hooks, and the daemon thread that calls trigger_all_hooks(true) is not started in UT. So this was latent, not an observed use-after-free. ResultBufferMgrTest on master builds six managers on the stack in the same way and never stops them; that file is outside this PR.
TPC-H: Total hot run time: 28498 ms |
TPC-DS: Total hot run time: 151416 ms |
ClickBench: Total hot run time: 24.11 s |
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
…ght cancel test fixture ends ### What problem does this PR solve? Issue Number: None Related PR: apache#69945 Problem Summary: `ResultBufferMgr`'s constructor registers the process-wide `result_buffer_block_count` hook, and the hook captures the manager. Only `ResultBufferMgr::stop()` deregisters it. `ArrowFlightCancelTest` builds a manager per test and never stopped it, so after each test the global metric entity kept a hook that pointed at a destroyed manager. A later metric collection with hooks enabled would read freed memory. Because `MetricEntity::register_hook` uses `emplace`, a later fixture could not replace a stale hook either. `TearDown()` now calls `_mgr.stop()` after restoring the previous manager, the same way `LoadChannelMgrTest` stops its `LoadChannelMgr`. The manager is never `init()`ed here, so `stop()` only removes the hook and its gauge; no cleaner thread is joined. ### Release note None ### Check List (For Author) - Test: Unit Test - `ArrowFlightCancelTest.*`, `ArrowFlightRemoteCancelTest.*`, `ResultBufferMgrTest.*` - Behavior changed: No - Does this need documentation: No Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Addressed review round 1 in
|
|
run buildall |
|
/review |
TPC-H: Total hot run time: 28697 ms |
TPC-DS: Total hot run time: 151515 ms |
ClickBench: Total hot run time: 24.48 s |
|
Codex automated review failed and did not complete. Error: Selected model is at capacity. Please try a different model. Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
…s next request ### What problem does this PR solve? Issue Number: None Related PR: apache#68630, apache#69945 Problem Summary: The FE dropped a Flight query's cancellation route when the session's next request began and when the session was torn down (FlightSqlChannel.reset). The client reads the results from the BEs, so it can still hold a stream of the query open at that point, for example through a second statement on the same connection, and close that stream early afterwards. The BE's cancelFlightQuery then found no route, and the query's other BEs kept their pipelines and result buffers until the query timeout. branch-4.x (apache#68630) drops the route at the same points. A route now belongs to no session. It goes when its cancellation succeeds, when its query's execution timeout plus 5 s has passed, and, oldest first and as a whole, when the registry would hold more than 100,000 result ids, a few hundred bytes each. Without the session as a bound, that cap is what keeps the registry from growing with the query rate times the query timeout; a query whose route was pushed out ends at its timeout, as every Flight query did before this PR. The registry is an insertion-ordered map under a lock, with the RPCs sent outside it, instead of a Caffeine cache, whose size eviction may reject a new entry instead of evicting the oldest one. StmtExecutor registers a query through FlightSqlQueryCancellation.register(ConnectContext, Coordinator), and FlightSqlChannel is back to its master form. ### Release note None ### Check List (For Author) - Test: Unit Test / Regression test - FE UT: FlightSqlQueryCancellationTest (5): a route survives the session's next request and its teardown and still cancels; the bound drops whole routes, oldest first; an expired route leaves at the next registration. With the 16 other Flight session and coordinator test classes. - Regression: test_flight_cancel_cleanup gains a mode that runs the session's next request before it aborts the stream. - Behavior changed: Yes. A stream aborted after its session moved on to another request, or was closed, still cancels its query on every BE. - Does this need documentation: No Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
|
/review |
|
Codex automated review failed and did not complete. Error: Selected model is at capacity. Please try a different model. Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
|
/review |
|
Codex automated review failed and did not complete. Error: Selected model is at capacity. Please try a different model. Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
|
/review |
There was a problem hiding this comment.
Static review of the full PR diff found no new actionable issue at the current head.
The cancellation state machine preserves bounded empty-buffer waits as distinct from EOS, polls client cancellation while reading local results, and makes Close/destruction idempotent. Local cancellation wakes the result-buffer wait and clears the query's Arrow Flight siblings; map/index updates are protected by the buffer-map lock, while query cancellation runs after the lock is released. The BRPC callback releases its mutex before StartCancel and join. cancel_plan_fragment preserves unread results for LIMIT_REACH and FINISHED and clears buffers for abort statuses, including both legacy and serialized status forms. Missing-buffer Arrow fetches now complete the RPC with a serialized error.
The existing P2 thread 4438874982 was revalidated as fixed by the fixture's _mgr.stop() teardown; it is not reposted. The PR's remote cross-BE cancellation limitation is explicitly documented as a follow-up, so it is not reported as a new finding. No additional user focus was supplied.
This was a static review only; no builds or tests were run. Review convergence: true after two bounded rounds, with all risk scopes and replacement/final-sweep scopes dispositioned and no accepted inline findings.
Review converged after 2 round(s).
|
PR approved by anyone and no changes requested. |
|
PR approved by at least one committer and no changes requested. |
…t stream closes early ### What problem does this PR solve? Issue Number: None Related PR: apache#68630, apache#69945 Problem Summary: Port the FE route of branch-4.x apache#68630 to master, on top of apache#69945 (its BE-local half). With apache#69945 alone, a DoGet stream closed before EOF cancels its query only on the BE that holds the stream's results. A query whose results are spread over several BEs (parallel result sink, one endpoint per BE) kept running on the other BEs: master closes the coordinator when GetFlightInfo returns (apache#67504), so nothing sends them a cancel, and a single-fragment query has no exchange that would carry the cancellation between BEs. Their pipelines ran until the query finished or timed out. A remote reader (a DoGet served by a BE other than the result BE) could not cancel the result BE's query at all. The FE now keeps a route for each Flight query that leaves its results on the BEs: from each result id (the fragment instance id of an endpoint) to the query id and the BRPC addresses of all its BEs (FlightSqlQueryCancellation, registered by StmtExecutor before the coordinator is closed or deferred). The route belongs to the session's FlightSqlChannel and is dropped when the session's next request begins (FlightProtocolAdapter.beginRequest resets the channel), when the session is torn down (releaseSession closes the channel), when a cancellation succeeds, and at the latest execution_timeout + 5 s after registration. The new FrontendService RPC cancelFlightQuery(result_id) sends the existing cancel_plan_fragment, which older BEs understand and which does not use the Flight read pool, to every BE of the query and waits at most 1 s for all of them; when any BE fails, the route stays so that the caller can retry. A reader closed before EOF now asks the running FEs in turn: an FE without the route answers NOT_FOUND and the BE moves on to the next one, a failed attempt is retried once, and the transport is reopened after a thrift error. This runs on the DoGet thread, so with an unreachable FE one early close can take a few seconds (two attempts with a 2 s RPC timeout per FE). Compatibility: cancelFlightQuery has the same name and signature as on branch-4.1 and branch-4.2. An FE without it fails the call; the BE logs a warning for that FE and only the local cleanup of apache#69945 takes effect. A result BE without apache#69945 cancels the query but keeps its result buffer until its usual timeout. Differences from branch-4.x: the route is registered in master's `!isReturnResultFromLocal()` branch (apache#67900), before the `mustOutliveDispatch()` deferral, so deferred queries are routed as well; FlightProtocolAdapter needs no change because the route stays in FlightSqlChannel; the BE reader includes runtime/frontend_info.h itself (exec_env.h no longer includes it, apache#66400); the regression suite keeps its table at the end (AGENTS.md). ### Release note Arrow Flight SQL: cancelling or closing a result stream before its end now cancels the query on every BE that runs it, not only on the BE that serves the stream. The FE and the BEs must both be upgraded. ### Check List (For Author) - Test: Unit Test / Regression test - FE UT: FlightSqlQueryCancellationTest (5, new), StmtExecutorTest and 15 other Flight session / coordinator test classes - BE UT: ArrowFlightCancelTest (9), ArrowFlightRemoteCancelTest (9, 6 new), ResultBufferMgrTest (6), ArrowResultBlockBufferTest (4) - Regression: arrow_flight_sql_p0/test_flight_cancel_cleanup (new) and the rest of arrow_flight_sql_p0 on a local single-BE cluster; the cross-BE path is covered by the BE UT's in-process FE and remote BE - Behavior changed: Yes. A Flight result stream closed before EOF cancels its query on every BE of the query. - Does this need documentation: No Co-authored-by: Gabriel <liwenqiang@selectdb.com> Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…s next request ### What problem does this PR solve? Issue Number: None Related PR: apache#68630, apache#69945 Problem Summary: The FE dropped a Flight query's cancellation route when the session's next request began and when the session was torn down (FlightSqlChannel.reset). The client reads the results from the BEs, so it can still hold a stream of the query open at that point, for example through a second statement on the same connection, and close that stream early afterwards. The BE's cancelFlightQuery then found no route, and the query's other BEs kept their pipelines and result buffers until the query timeout. branch-4.x (apache#68630) drops the route at the same points. A route now belongs to no session. It goes when its cancellation succeeds, when its query's execution timeout plus 5 s has passed, and, oldest first and as a whole, when the registry would hold more than 100,000 result ids, a few hundred bytes each. Without the session as a bound, that cap is what keeps the registry from growing with the query rate times the query timeout; a query whose route was pushed out ends at its timeout, as every Flight query did before this PR. The registry is an insertion-ordered map under a lock, with the RPCs sent outside it, instead of a Caffeine cache, whose size eviction may reject a new entry instead of evicting the oldest one. StmtExecutor registers a query through FlightSqlQueryCancellation.register(ConnectContext, Coordinator), and FlightSqlChannel is back to its master form. ### Release note None ### Check List (For Author) - Test: Unit Test / Regression test - FE UT: FlightSqlQueryCancellationTest (5): a route survives the session's next request and its teardown and still cancels; the bound drops whole routes, oldest first; an expired route leaves at the next registration. With the 16 other Flight session and coordinator test classes. - Regression: test_flight_cancel_cleanup gains a mode that runs the session's next request before it aborts the stream. - Behavior changed: Yes. A stream aborted after its session moved on to another request, or was closed, still cancels its query on every BE. - Does this need documentation: No Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…t stream closes early ### What problem does this PR solve? Issue Number: None Related PR: apache#68630, apache#69945 Problem Summary: Port the FE route of branch-4.x apache#68630 to master, on top of apache#69945 (its BE-local half). With apache#69945 alone, a DoGet stream closed before EOF cancels its query only on the BE that holds the stream's results. A query whose results are spread over several BEs (parallel result sink, one endpoint per BE) kept running on the other BEs: master closes the coordinator when GetFlightInfo returns (apache#67504), so nothing sends them a cancel, and a single-fragment query has no exchange that would carry the cancellation between BEs. Their pipelines ran until the query finished or timed out. A remote reader (a DoGet served by a BE other than the result BE) could not cancel the result BE's query at all. The FE now keeps a route for each Flight query that leaves its results on the BEs: from each result id (the fragment instance id of an endpoint) to the query id and the BRPC addresses of all its BEs (FlightSqlQueryCancellation, registered by StmtExecutor before the coordinator is closed or deferred). The route belongs to the session's FlightSqlChannel and is dropped when the session's next request begins (FlightProtocolAdapter.beginRequest resets the channel), when the session is torn down (releaseSession closes the channel), when a cancellation succeeds, and at the latest execution_timeout + 5 s after registration. The new FrontendService RPC cancelFlightQuery(result_id) sends the existing cancel_plan_fragment, which older BEs understand and which does not use the Flight read pool, to every BE of the query and waits at most 1 s for all of them; when any BE fails, the route stays so that the caller can retry. A reader closed before EOF now asks the running FEs in turn: an FE without the route answers NOT_FOUND and the BE moves on to the next one, a failed attempt is retried once, and the transport is reopened after a thrift error. This runs on the DoGet thread, so with an unreachable FE one early close can take a few seconds (two attempts with a 2 s RPC timeout per FE). Compatibility: cancelFlightQuery has the same name and signature as on branch-4.1 and branch-4.2. An FE without it fails the call; the BE logs a warning for that FE and only the local cleanup of apache#69945 takes effect. A result BE without apache#69945 cancels the query but keeps its result buffer until its usual timeout. Differences from branch-4.x: the route is registered in master's `!isReturnResultFromLocal()` branch (apache#67900), before the `mustOutliveDispatch()` deferral, so deferred queries are routed as well; FlightProtocolAdapter needs no change because the route stays in FlightSqlChannel; the BE reader includes runtime/frontend_info.h itself (exec_env.h no longer includes it, apache#66400); the regression suite keeps its table at the end (AGENTS.md). ### Release note Arrow Flight SQL: cancelling or closing a result stream before its end now cancels the query on every BE that runs it, not only on the BE that serves the stream. The FE and the BEs must both be upgraded. ### Check List (For Author) - Test: Unit Test / Regression test - FE UT: FlightSqlQueryCancellationTest (5, new), StmtExecutorTest and 15 other Flight session / coordinator test classes - BE UT: ArrowFlightCancelTest (9), ArrowFlightRemoteCancelTest (9, 6 new), ResultBufferMgrTest (6), ArrowResultBlockBufferTest (4) - Regression: arrow_flight_sql_p0/test_flight_cancel_cleanup (new) and the rest of arrow_flight_sql_p0 on a local single-BE cluster; the cross-BE path is covered by the BE UT's in-process FE and remote BE - Behavior changed: Yes. A Flight result stream closed before EOF cancels its query on every BE of the query. - Does this need documentation: No Co-authored-by: Gabriel <liwenqiang@selectdb.com> Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…s next request ### What problem does this PR solve? Issue Number: None Related PR: apache#68630, apache#69945 Problem Summary: The FE dropped a Flight query's cancellation route when the session's next request began and when the session was torn down (FlightSqlChannel.reset). The client reads the results from the BEs, so it can still hold a stream of the query open at that point, for example through a second statement on the same connection, and close that stream early afterwards. The BE's cancelFlightQuery then found no route, and the query's other BEs kept their pipelines and result buffers until the query timeout. branch-4.x (apache#68630) drops the route at the same points. A route now belongs to no session. It goes when its cancellation succeeds, when its query's execution timeout plus 5 s has passed, and, oldest first and as a whole, when the registry would hold more than 100,000 result ids, a few hundred bytes each. Without the session as a bound, that cap is what keeps the registry from growing with the query rate times the query timeout; a query whose route was pushed out ends at its timeout, as every Flight query did before this PR. The registry is an insertion-ordered map under a lock, with the RPCs sent outside it, instead of a Caffeine cache, whose size eviction may reject a new entry instead of evicting the oldest one. StmtExecutor registers a query through FlightSqlQueryCancellation.register(ConnectContext, Coordinator), and FlightSqlChannel is back to its master form. ### Release note None ### Check List (For Author) - Test: Unit Test / Regression test - FE UT: FlightSqlQueryCancellationTest (5): a route survives the session's next request and its teardown and still cancels; the bound drops whole routes, oldest first; an expired route leaves at the next registration. With the 16 other Flight session and coordinator test classes. - Regression: test_flight_cancel_cleanup gains a mode that runs the session's next request before it aborts the stream. - Behavior changed: Yes. A stream aborted after its session moved on to another request, or was closed, still cancels its query on every BE. - Does this need documentation: No Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…rdown ### What problem does this PR solve? Issue Number: None Related PR: apache#69945 Problem Summary: A Flight result stream closed before EOF asks the running FEs in turn to cancel its query (`cancelFlightQuery`), on the DoGet thread, from `Close()` and from the reader's destructor. The 2 s timeout it gave applied only to a connection it opened itself: `ClientCacheHelper::get_client` returns a cached transport with the timeout of whichever caller opened it, such as 60 s for profile and status reports, or the query's execution timeout (900 s by default) for the information_schema scanners. Acquiring a connection also backed off 1 + 2 + 4 s after failures, and each FE got two attempts. An FE that sorts before the one that planned the query is asked first on every BE, and a stopped FE stays in that list as long as it is not dropped (the master sends it with process UUID 0, which `get_running_frontends()` counts as running). So every early close waited about 14.6 s for such an FE when it refused connections, 32.6 s when it did not answer the connect, and up to minutes when a long-timeout transport to it sat in the cache. The other way round, this call's 2 s transports, and the 1 s ones it reopened after any thrift error (an older FE's unknown-method reply included), went back to the shared cache, where `fetchSplitBatch`, whose FE side holds the call for up to 1 s, or a status report could take them. The cancellation now keeps FE connections of its own (`ExecEnv::flight_cancel_frontend_client_cache`, at most two kept per FE, as each idle one holds an FE thrift thread), so every transport it uses was opened with its 2 s timeout and none reaches another caller. Acquiring a connection does not back off (`max_retries = 0`), a transport is reopened with the same 2 s, and an FE that cannot be connected to gets no second attempt, since neither reason for the retry (the FE's own deadline, a stale cached transport) applies to it. An FE ahead of the planning one now costs an early close about 0.1 s when it refuses connections, 3.1 s when it does not answer the connect (`thrift_connect_timeout_seconds`), and 4 s when it accepts the connection and does not answer. ### Release note None ### Check List (For Author) - Test: Unit Test - `ArrowFlightRemoteCancelTest#CloseIgnoresLongTimeoutSharedTransport` leaves a 60 s transport to an FE whose first answer stalls 5 s in the shared cache and requires the close to finish within 4 s, through the retry; `#CloseMovesPastUnreachableFrontend` puts an FE that refuses connections first and requires the close to reach the planning FE within 2 s. - Behavior changed: No - Does this need documentation: No Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… registry bound ### What problem does this PR solve? Issue Number: None Related PR: apache#69945 Problem Summary: A route of `FlightSqlQueryCancellation` keeps the BRPC addresses of all the BEs of its query, and the result ids of a query share one route, but the registry bound counted only the result ids. In parallel result mode, the default, a query has one result id, so a full registry of 100,000 routes held about 0.3 GB with 100 BEs per query and 2.9 GB with 1,000. Routes no longer end with their session, and a stream read to EOF does not drop its route, so a registry fills up whenever queries arrive faster than 100,000 per execution timeout (about 111 per second with the default 900 s). The registry now also bounds the addresses its routes hold, at 1,000,000 (about 30 MB), and drops whole routes, oldest first, while either bound is exceeded. A route's addresses leave with its last result id, also when a newer registration takes an id over, and are released only once when the route is pushed out while a cancellation through it is in flight. The registry now stays within about 55 MB whatever the cluster size. Large clusters give up retention instead: with 1,000 BEs per query it keeps the routes of the last 1,000 queries, and a query whose route was pushed out ends at its timeout, as before routes existed. A route must have a result id, which every Flight query that leaves its results on the BEs has. ### Release note None ### Check List (For Author) - Test: Unit Test - `FlightSqlQueryCancellationTest#boundCountsRetainedBackends` (addresses past the bound push out the oldest route as a whole) and `#reregisteredResultReleasesOldRouteBackends` (a route whose result id a newer registration took over releases its addresses once). The coordinator mock of `StmtExecutorTest#testCloudReplanDispatchProfileLifecycle` now adds an endpoint in `exec()`, as the coordinator does, since a route needs a result id. - Behavior changed: No - Does this need documentation: No Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… fails ### What problem does this PR solve? Issue Number: None Related PR: apache#69945 Problem Summary: A Flight query whose results stay on the BEs registers its cancellation route in `StmtExecutor`, before GetFlightInfo fetches the Arrow schema from the result BE and builds the tickets. When the request fails after that, at the schema fetch or because a prepared statement's result schema no longer matches, no ticket names the query's results, so no BE can report an early close of them, yet the route stayed in `FlightSqlQueryCancellation` until its query's timeout plus 5 s, or until newer routes pushed it out. While routes belonged to the session, its next request dropped them; since routes outlive the session, nothing did. A GetFlightInfo that fails after it began now drops the routes of its endpoints, each as a whole, and so does the prepared-statement schema check before it resets the session. A request that fails before it begins, such as an empty query, finds the previous request's endpoints on the session and leaves their routes: a client may still be pulling those results. ### Release note None ### Check List (For Author) - Test: Unit Test - `DorisFlightSqlProducerTest#testGetFlightInfoDropsRouteWhenSchemaFetchFails` (a failed schema fetch drops the route its query registered), `#testFailedRequestKeepsPreviousRequestRoute` (an empty query leaves the previous request's route), and `FlightSqlQueryCancellationTest#unregisterDropsWholeRoutes` (a route goes as a whole, with its addresses). `DorisFlightSqlSchemaTest#executionSchemaDriftExpiresHandleAndCleansResults` now also requires the route of a prepared statement whose result schema changed to go, with no second cancellation. - Behavior changed: No - Does this need documentation: No Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… their routes ### What problem does this PR solve? Issue Number: None Related PR: apache#69945 Problem Summary: When a GetFlightInfo fails after its query ran, at the Arrow schema fetch for instance, no client can pull the query's results. A deferred query is then cancelled through its coordinator, which is still open. Any other query has closed its coordinator already, and nothing cancelled it: its fragments and result buffers ran on until the BE's result buffer timer ended them, execution_timeout plus 5 s later. The query's cancellation route holds what a cancellation needs. The failure path now sends the cancellation through it to every BE of each query that no deferred coordinator cancelled, and drops the routes as before. A deferred query whose coordinator fails to cancel it falls back to its route. The failure path does not wait for the BEs' answers: the routes go whatever they are, so no retry follows, the client is waiting for the request's error, and a deferred coordinator's cancellation does not wait for them either. ### Release note None ### Check List (For Author) - Test: Unit Test - `DorisFlightSqlProducerTest#testGetFlightInfoCancelsQueryAndDropsRouteWhenSchemaFetchFails` (a query that was not deferred is cancelled on both its BEs and its route goes), `#testGetFlightInfoLeavesDeferredQueryToItsCoordinator` (a deferred query is cancelled once, through its coordinator), and `FlightSqlQueryCancellationTest#failedRequestCancelsWithoutWaitingAndDropsRoutes` (BEs that never answer do not hold the request up). - Behavior changed: No - Does this need documentation: No Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
### What problem does this PR solve? Issue Number: None Related PR: apache#69945 Problem Summary: A Flight result stream closed before EOF asks the running FEs, in address order, to cancel its query until one answers OK. Only the FE that planned the query holds the route, and the list includes the FEs the master reports with process UUID 0: those it cannot reach, which stay on a BE's list until they are dropped. Every such FE that sorted before the planning FE delayed every early close on every BE by its connection budget: about 0.1 s when it refuses connections, 3.1 s when it does not answer the connect, and 4 s when it accepts connections and never answers. The planning FE is one the master reaches, unless it has lost the master's heartbeat since. The FEs with a non-zero process UUID are now asked first and the others last, so a stopped FE delays an early close only when no reachable FE holds the route: when the route has expired or been pushed out, or when the planning FE is itself the unreachable one. An FE that hangs while the master still reaches it costs up to 4 s as before, until the master's heartbeat to it fails. No overall deadline is added: with this order it would only cut those cases short, and in the last one the early close would lose its cancellation. ### Release note None ### Check List (For Author) - Test: Unit Test - `ArrowFlightRemoteCancelTest#CloseAsksReachableFrontendsFirst`: an FE that accepts connections and never answers, reported with process UUID 0 and sorting before the planning FE, no longer delays the close (14 ms; about 4 s with the FEs asked in address order). - Behavior changed: No - Does this need documentation: No Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…t stream closes early ### What problem does this PR solve? Issue Number: None Related PR: apache#68630, apache#69945 Problem Summary: Port the FE route of branch-4.x apache#68630 to master, on top of apache#69945 (its BE-local half). With apache#69945 alone, a DoGet stream closed before EOF cancels its query only on the BE that holds the stream's results. A query whose results are spread over several BEs (parallel result sink, one endpoint per BE) kept running on the other BEs: master closes the coordinator when GetFlightInfo returns (apache#67504), so nothing sends them a cancel, and a single-fragment query has no exchange that would carry the cancellation between BEs. Their pipelines ran until the query finished or timed out. A remote reader (a DoGet served by a BE other than the result BE) could not cancel the result BE's query at all. The FE now keeps a route for each Flight query that leaves its results on the BEs: from each result id (the fragment instance id of an endpoint) to the query id and the BRPC addresses of all its BEs (FlightSqlQueryCancellation, registered by StmtExecutor before the coordinator is closed or deferred). The route belongs to the session's FlightSqlChannel and is dropped when the session's next request begins (FlightProtocolAdapter.beginRequest resets the channel), when the session is torn down (releaseSession closes the channel), when a cancellation succeeds, and at the latest execution_timeout + 5 s after registration. The new FrontendService RPC cancelFlightQuery(result_id) sends the existing cancel_plan_fragment, which older BEs understand and which does not use the Flight read pool, to every BE of the query and waits at most 1 s for all of them; when any BE fails, the route stays so that the caller can retry. A reader closed before EOF now asks the running FEs in turn: an FE without the route answers NOT_FOUND and the BE moves on to the next one, a failed attempt is retried once, and the transport is reopened after a thrift error. This runs on the DoGet thread, so with an unreachable FE one early close can take a few seconds (two attempts with a 2 s RPC timeout per FE). Compatibility: cancelFlightQuery has the same name and signature as on branch-4.1 and branch-4.2. An FE without it fails the call; the BE logs a warning for that FE and only the local cleanup of apache#69945 takes effect. A result BE without apache#69945 cancels the query but keeps its result buffer until its usual timeout. Differences from branch-4.x: the route is registered in master's `!isReturnResultFromLocal()` branch (apache#67900), before the `mustOutliveDispatch()` deferral, so deferred queries are routed as well; FlightProtocolAdapter needs no change because the route stays in FlightSqlChannel; the BE reader includes runtime/frontend_info.h itself (exec_env.h no longer includes it, apache#66400); the regression suite keeps its table at the end (AGENTS.md). ### Release note Arrow Flight SQL: cancelling or closing a result stream before its end now cancels the query on every BE that runs it, not only on the BE that serves the stream. The FE and the BEs must both be upgraded. ### Check List (For Author) - Test: Unit Test / Regression test - FE UT: FlightSqlQueryCancellationTest (5, new), StmtExecutorTest and 15 other Flight session / coordinator test classes - BE UT: ArrowFlightCancelTest (9), ArrowFlightRemoteCancelTest (9, 6 new), ResultBufferMgrTest (6), ArrowResultBlockBufferTest (4) - Regression: arrow_flight_sql_p0/test_flight_cancel_cleanup (new) and the rest of arrow_flight_sql_p0 on a local single-BE cluster; the cross-BE path is covered by the BE UT's in-process FE and remote BE - Behavior changed: Yes. A Flight result stream closed before EOF cancels its query on every BE of the query. - Does this need documentation: No Co-authored-by: Gabriel <liwenqiang@selectdb.com> Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…s next request ### What problem does this PR solve? Issue Number: None Related PR: apache#68630, apache#69945 Problem Summary: The FE dropped a Flight query's cancellation route when the session's next request began and when the session was torn down (FlightSqlChannel.reset). The client reads the results from the BEs, so it can still hold a stream of the query open at that point, for example through a second statement on the same connection, and close that stream early afterwards. The BE's cancelFlightQuery then found no route, and the query's other BEs kept their pipelines and result buffers until the query timeout. branch-4.x (apache#68630) drops the route at the same points. A route now belongs to no session. It goes when its cancellation succeeds, when its query's execution timeout plus 5 s has passed, and, oldest first and as a whole, when the registry would hold more than 100,000 result ids, a few hundred bytes each. Without the session as a bound, that cap is what keeps the registry from growing with the query rate times the query timeout; a query whose route was pushed out ends at its timeout, as every Flight query did before this PR. The registry is an insertion-ordered map under a lock, with the RPCs sent outside it, instead of a Caffeine cache, whose size eviction may reject a new entry instead of evicting the oldest one. StmtExecutor registers a query through FlightSqlQueryCancellation.register(ConnectContext, Coordinator), and FlightSqlChannel is back to its master form. ### Release note None ### Check List (For Author) - Test: Unit Test / Regression test - FE UT: FlightSqlQueryCancellationTest (5): a route survives the session's next request and its teardown and still cancels; the bound drops whole routes, oldest first; an expired route leaves at the next registration. With the 16 other Flight session and coordinator test classes. - Regression: test_flight_cancel_cleanup gains a mode that runs the session's next request before it aborts the stream. - Behavior changed: Yes. A stream aborted after its session moved on to another request, or was closed, still cancels its query on every BE. - Does this need documentation: No Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…rdown ### What problem does this PR solve? Issue Number: None Related PR: apache#69945 Problem Summary: A Flight result stream closed before EOF asks the running FEs in turn to cancel its query (`cancelFlightQuery`), on the DoGet thread, from `Close()` and from the reader's destructor. The 2 s timeout it gave applied only to a connection it opened itself: `ClientCacheHelper::get_client` returns a cached transport with the timeout of whichever caller opened it, such as 60 s for profile and status reports, or the query's execution timeout (900 s by default) for the information_schema scanners. Acquiring a connection also backed off 1 + 2 + 4 s after failures, and each FE got two attempts. An FE that sorts before the one that planned the query is asked first on every BE, and a stopped FE stays in that list as long as it is not dropped (the master sends it with process UUID 0, which `get_running_frontends()` counts as running). So every early close waited about 14.6 s for such an FE when it refused connections, 32.6 s when it did not answer the connect, and up to minutes when a long-timeout transport to it sat in the cache. The other way round, this call's 2 s transports, and the 1 s ones it reopened after any thrift error (an older FE's unknown-method reply included), went back to the shared cache, where `fetchSplitBatch`, whose FE side holds the call for up to 1 s, or a status report could take them. The cancellation now keeps FE connections of its own (`ExecEnv::flight_cancel_frontend_client_cache`, at most two kept per FE, as each idle one holds an FE thrift thread), so every transport it uses was opened with its 2 s timeout and none reaches another caller. Acquiring a connection does not back off (`max_retries = 0`), a transport is reopened with the same 2 s, and an FE that cannot be connected to gets no second attempt, since neither reason for the retry (the FE's own deadline, a stale cached transport) applies to it. An FE ahead of the planning one now costs an early close about 0.1 s when it refuses connections, 3.1 s when it does not answer the connect (`thrift_connect_timeout_seconds`), and 4 s when it accepts the connection and does not answer. ### Release note None ### Check List (For Author) - Test: Unit Test - `ArrowFlightRemoteCancelTest#CloseIgnoresLongTimeoutSharedTransport` leaves a 60 s transport to an FE whose first answer stalls 5 s in the shared cache and requires the close to finish within 4 s, through the retry; `#CloseMovesPastUnreachableFrontend` puts an FE that refuses connections first and requires the close to reach the planning FE within 2 s. - Behavior changed: No - Does this need documentation: No Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… registry bound ### What problem does this PR solve? Issue Number: None Related PR: apache#69945 Problem Summary: A route of `FlightSqlQueryCancellation` keeps the BRPC addresses of all the BEs of its query, and the result ids of a query share one route, but the registry bound counted only the result ids. In parallel result mode, the default, a query has one result id, so a full registry of 100,000 routes held about 0.3 GB with 100 BEs per query and 2.9 GB with 1,000. Routes no longer end with their session, and a stream read to EOF does not drop its route, so a registry fills up whenever queries arrive faster than 100,000 per execution timeout (about 111 per second with the default 900 s). The registry now also bounds the addresses its routes hold, at 1,000,000 (about 30 MB), and drops whole routes, oldest first, while either bound is exceeded. A route's addresses leave with its last result id, also when a newer registration takes an id over, and are released only once when the route is pushed out while a cancellation through it is in flight. The registry now stays within about 55 MB whatever the cluster size. Large clusters give up retention instead: with 1,000 BEs per query it keeps the routes of the last 1,000 queries, and a query whose route was pushed out ends at its timeout, as before routes existed. A route must have a result id, which every Flight query that leaves its results on the BEs has. ### Release note None ### Check List (For Author) - Test: Unit Test - `FlightSqlQueryCancellationTest#boundCountsRetainedBackends` (addresses past the bound push out the oldest route as a whole) and `#reregisteredResultReleasesOldRouteBackends` (a route whose result id a newer registration took over releases its addresses once). The coordinator mock of `StmtExecutorTest#testCloudReplanDispatchProfileLifecycle` now adds an endpoint in `exec()`, as the coordinator does, since a route needs a result id. - Behavior changed: No - Does this need documentation: No Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… fails ### What problem does this PR solve? Issue Number: None Related PR: apache#69945 Problem Summary: A Flight query whose results stay on the BEs registers its cancellation route in `StmtExecutor`, before GetFlightInfo fetches the Arrow schema from the result BE and builds the tickets. When the request fails after that, at the schema fetch or because a prepared statement's result schema no longer matches, no ticket names the query's results, so no BE can report an early close of them, yet the route stayed in `FlightSqlQueryCancellation` until its query's timeout plus 5 s, or until newer routes pushed it out. While routes belonged to the session, its next request dropped them; since routes outlive the session, nothing did. A GetFlightInfo that fails after it began now drops the routes of its endpoints, each as a whole, and so does the prepared-statement schema check before it resets the session. A request that fails before it begins, such as an empty query, finds the previous request's endpoints on the session and leaves their routes: a client may still be pulling those results. ### Release note None ### Check List (For Author) - Test: Unit Test - `DorisFlightSqlProducerTest#testGetFlightInfoDropsRouteWhenSchemaFetchFails` (a failed schema fetch drops the route its query registered), `#testFailedRequestKeepsPreviousRequestRoute` (an empty query leaves the previous request's route), and `FlightSqlQueryCancellationTest#unregisterDropsWholeRoutes` (a route goes as a whole, with its addresses). `DorisFlightSqlSchemaTest#executionSchemaDriftExpiresHandleAndCleansResults` now also requires the route of a prepared statement whose result schema changed to go, with no second cancellation. - Behavior changed: No - Does this need documentation: No Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… their routes ### What problem does this PR solve? Issue Number: None Related PR: apache#69945 Problem Summary: When a GetFlightInfo fails after its query ran, at the Arrow schema fetch for instance, no client can pull the query's results. A deferred query is then cancelled through its coordinator, which is still open. Any other query has closed its coordinator already, and nothing cancelled it: its fragments and result buffers ran on until the BE's result buffer timer ended them, execution_timeout plus 5 s later. The query's cancellation route holds what a cancellation needs. The failure path now sends the cancellation through it to every BE of each query that no deferred coordinator cancelled, and drops the routes as before. A deferred query whose coordinator fails to cancel it falls back to its route. The failure path does not wait for the BEs' answers: the routes go whatever they are, so no retry follows, the client is waiting for the request's error, and a deferred coordinator's cancellation does not wait for them either. ### Release note None ### Check List (For Author) - Test: Unit Test - `DorisFlightSqlProducerTest#testGetFlightInfoCancelsQueryAndDropsRouteWhenSchemaFetchFails` (a query that was not deferred is cancelled on both its BEs and its route goes), `#testGetFlightInfoLeavesDeferredQueryToItsCoordinator` (a deferred query is cancelled once, through its coordinator), and `FlightSqlQueryCancellationTest#failedRequestCancelsWithoutWaitingAndDropsRoutes` (BEs that never answer do not hold the request up). - Behavior changed: No - Does this need documentation: No Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
### What problem does this PR solve? Issue Number: None Related PR: apache#69945 Problem Summary: A Flight result stream closed before EOF asks the running FEs, in address order, to cancel its query until one answers OK. Only the FE that planned the query holds the route, and the list includes the FEs the master reports with process UUID 0: those it cannot reach, which stay on a BE's list until they are dropped. Every such FE that sorted before the planning FE delayed every early close on every BE by its connection budget: about 0.1 s when it refuses connections, 3.1 s when it does not answer the connect, and 4 s when it accepts connections and never answers. The planning FE is one the master reaches, unless it has lost the master's heartbeat since. The FEs with a non-zero process UUID are now asked first and the others last, so a stopped FE delays an early close only when no reachable FE holds the route: when the route has expired or been pushed out, or when the planning FE is itself the unreachable one. An FE that hangs while the master still reaches it costs up to 4 s as before, until the master's heartbeat to it fails. No overall deadline is added: with this order it would only cut those cases short, and in the last one the early close would lose its cancellation. ### Release note None ### Check List (For Author) - Test: Unit Test - `ArrowFlightRemoteCancelTest#CloseAsksReachableFrontendsFirst`: an FE that accepts connections and never answers, reported with process UUID 0 and sorting before the planning FE, no longer delays the close (14 ms; about 4 s with the FEs asked in address order). - Behavior changed: No - Does this need documentation: No Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
What problem does this PR solve?
Issue Number: None
Related PR: #68630, #67644
Problem Summary:
In short
This ports the BE half of branch-4.x's #68630 to master. When an Arrow Flight SQL client cancels or closes a result stream before its end, master keeps the query running: the BE thread serving the stream waits on the result buffer without ever checking whether the client is still there, and the unread result blocks stay in memory, holding back the query, until the query times out or the delayed buffer cleanup runs minutes later. With this PR the BE notices the cancelled stream within 20 ms, and when the stream is served by the BE that holds its result (the usual case), that BE cancels the query and frees all of the query's Flight result buffers there. The FE route that also reaches the query's other BEs (
cancelFlightQuery, which changesFrontendService.thrift) comes in a follow-up PR.Background
GetFlightInforeturns one endpoint per result buffer, and the client reads each endpoint with aDoGetstream. Since [fix](arrow-flight) Release a finished Flight query's coordinator instead of holding it until wait_timeout #67504 the FE closes the coordinator as soon as it has returned theFlightInfo, so it no longer cancels anything on behalf of an open stream.DoGetis served byArrowFlightBatchLocalReaderwhen the result buffer is on the same BE, or byArrowFlightBatchRemoteReader, which pulls the blocks from the result BE with thefetch_arrow_dataBRPC.ArrowFlightResultBlockBuffer) is keyed by the query id when parallel result sink is on (the default) and by the fragment instance id when it is off. When it is full, the result sink stops and the query waits for the reader.ResultBufferMgrdrops a buffer that nobody removed atexecution_timeout + 5 s, orresult_buffer_cancelled_interval_time(300 s by default) after the fragments writing it have finished, whichever comes first. Finishing with an error does not clear the unread blocks.cancel_plan_fragmentis the BRPC the FE uses to cancel a query on a BE.The problem, and what it cost
get_arrow_batchloops on 20 ms waits until a block arrives, the buffer is closed or it fails. It has no access toServerCallContext::is_cancelled(), so aDoGetcancelled while the buffer is empty keeps a Flight server thread waiting until the query produces a block or ends.RecordBatchReader::Close(), and their destructor only logs. A client that stops reading (aLIMITreached on the client side, a cancelled cursor, a dropped connection) leaves the query running and its unread blocks in the buffer. When the result does not fit in the buffer, the full buffer back-pressures the result sink and the fragments, with their memory, wait until the query times out; the unread blocks go only when the buffer is dropped.fetch_arrow_datawithbrpc::Join, which returns only when the result BE answers orarrow_flight_reader_brpc_controller_timeout_ms(300 s by default) passes.cancel_plan_fragmentcancels the fragments but not the query's Flight buffers, so their unread blocks stay until the delayed cleanup 300 s later.fetch_arrow_datacannot find the buffer, it logs and returns without runningdone, so the caller waits for its BRPC timeout instead of getting an error. [fix](be) Return missing-buffer errors for MySQL and Arrow result fetches #67644 fixes the same line.How this PR fixes it
get_arrow_batch(result, eos)waits at most 20 ms and then returns OK with no block andeos = false;eosnow marks the real end. The local reader retries in a loop and checksis_cancelled()before each wait.DoGetStatementpassescontext.is_cancelled()to both readers.ArrowFlightBatchReaderBasenow implementsReadNext(which calls each reader'sReadNextImpl) andClose(). A stream counts as aborted when it is cancelled, closed before EOF, fails while reading, or is destroyed before EOF; a stream that reached EOF is never treated as aborted.ResultBufferMgrkeeps an index from query id to the query's Flight buffers, maintained under the buffer-map lock together with the map.cancel_arrow_flight_buffersremoves every Flight buffer of the query from the map, then, outside the lock, fails each buffer (which wakes its waiting fetches and releases the back-pressure) and cancels theQueryContextit holds a weak reference to. Sibling buffers of the same query are cleared even after theirQueryContexthas expired.is_cancelled()every 20 ms and callsbrpc::StartCancelwhen the stream is gone.cancel_plan_fragmentdrops the Flight buffers of an aborted query. It callscancel_arrow_flight_buffersunless the reason isLIMIT_REACHorFINISHED, which can leave valid unread results that the client still has to read.fetch_arrow_dataanswers when the buffer is gone. It completes the RPC with the lookup error. The line is identical to the one [fix](be) Return missing-buffer errors for MySQL and Arrow result fetches #67644 adds, so the two PRs merge in either order.Differences from branch-4.x: the two consistency checks between the index and the buffer map are
DORIS_CHECK(anifand aDCHECKin 4.x), a null check on the reader's memory tracker that both constructors make unnecessary is not carried over, and theArrowFlightResultBlockBufferconstructor moved to the .cpp so thatvarrow_flight_result_writer.h, which 13 TUs include, does not includeruntime/query_context.h; only the .cpp needs its definition. The new lines also take a few clang-tidy fixes (the destructor callsArrowFlightBatchReaderBase::Close()explicitly instead of a virtual call,std::scoped_lock), and the test starts its BRPC server insideScopedLeakCheckDisabler, like the other BE tests that start one, because brpc registers process-lifetime built-in services on its first start.Not in this PR: the FE route. A remote reader's abort, and an abort on one result BE, still do not reach the query's other BEs; their fragments and buffers wait for the timeout as before. The follow-up adds
FrontendService.cancelFlightQuery(same signature as branch-4.x), the BE broadcast to the FEs and the FE routing table, together with the FE unit test and thetest_flight_cancel_cleanupregression test.DoGetwhile the result buffer is empty (local reader)cancel_plan_fragment, abort reason)LIMIT_REACH/FINISHEDstill keep themfetch_arrow_datafor a removed bufferDoGetwith the ticket of a stream aborted on its result BEResults
ArrowFlightCancelTest(9 tests, new): early close and destruction free a back-pressured buffer and cancel the query; sibling buffers are freed after theirQueryContextexpired; EOF followed by close is not a cancellation; cancellation interrupts an empty-buffer wait; empty waits return for retry and do not end the stream; a conversion error cancels the query; a real Flight server sees a client-sideCancel()throughServerCallContextand cleans up.ArrowFlightRemoteCancelTest(3 tests, new, against an in-process BRPCPInternalService): cancellation interrupts a pendingfetch_arrow_data; an abortingcancel_plan_fragmentfrees the buffer and a following fetch completes with an error inside its 1 s timeout;LIMIT_REACH/FINISHEDkeep the unread results.test_flight_cancel_cleanup(it aborts one endpoint after the first batch, by closing it and by an explicit cancel, and requires the query's pipelines to disappear within 20 s) fails on master withCancelled Flight query retained pipelines: [...: 1 pipeline fragment contexts in query ...]and passes with this PR. The suite comes with the follow-up, because on a multi-BE cluster it also needs the FE route. The 20arrow_flight_sql_p0suites and the 14external_table_p0/remote_dorissuites, which read results over Arrow Flight, pass before and after.ResultBufferMgrTest(6) andArrowResultBlockBufferTest(4) pass 10 times in a row in one ASAN process. Ten single-point mutations of the change (dropping the buffer cleanup incancel_plan_fragment, theon_failurecall, the local cancel inclose(), the cancellation check in the local read loop or in the BRPC wait, the sibling loop, the EOF flag, theQueryContextcancel, the cancel in the destructor, theeos = falseon an empty wait) each turn their target tests red.Release note
Arrow Flight SQL: cancelling or closing a result stream before its end now cancels the query on the BE that holds the stream's results and frees its unread results there right away, instead of keeping them for minutes. A stream aborted this way cannot be read again with the same ticket.
Check List (For Author)
ArrowFlightCancelTest(9, new),ArrowFlightRemoteCancelTest(3, new),ResultBufferMgrTest(6),ArrowResultBlockBufferTest(4): pass 10 times in a row (ASAN, macOS); 10 mutations each caughtbuild-support/check-build-hygiene.shpasses; clang-format 16 clean; clang-tidy reports nothing on the changed lines exceptreadability-inconsistent-ifelse-bracesinside theDORIS_CHECKmacro expansiontest_flight_cancel_cleanupfails before and passes after;arrow_flight_sql_p0(20 suites) andexternal_table_p0/remote_doris(14 suites) passtest_flight_cancel_cleanupneeds the FE route of the follow-up to pass on a multi-BE cluster, so it comes with itDoGetwith the same ticket fails instead of resuming. On a stream served by another BE, the abort only interrupts the pending fetch.Original author: @Gabriel39