morningman commented on code in PR #69996:
URL: https://github.com/apache/doris/pull/69996#discussion_r4475338247
##########
be/src/service/arrow_flight/arrow_flight_batch_reader.cpp:
##########
@@ -103,6 +105,46 @@ void ArrowFlightBatchReaderBase::close(const Status&
reason) {
_statement->result_addr.port == config::brpc_port) {
env->result_mgr()->cancel_arrow_flight_query(_statement->query_id,
reason);
}
+ // A result endpoint can outlive its local fragment. Find the owning FE by
buffer ID,
+ // then use its query-wide cancellation route, including for older result
BEs.
+ for (const auto& [address, info] : env->get_running_frontends()) {
Review Comment:
This does not break the build. Clang's `-Wunused-variable` reports a
structured binding declaration only when none of its bindings is referenced,
and `address` is used in the loop. CI built this file at that head with Clang
20.1.8 and the same flags, `-Wall -Wextra -Werror` and `-Wunused` among them:
TeamCity BE UT build 1092350 compiled `arrow_flight_batch_reader.cpp.o` without
a diagnostic and stopped later at master's own `vparquet_writer_test.cpp` break
(fixed by #70209), and the COMPILE build passed. branch-4.1 and branch-4.2 have
had the same line since #68630, and master has many loops that bind an element
they never read, for example `vtablet_writer_v2.cpp:484` and
`exchange_source_operator.h:125`. With 35aece4ee7f, the change for the other
comment on this loop, `info` is read now: the FEs are ordered by its process
UUID.
##########
fe/fe-core/src/main/java/org/apache/doris/qe/StmtExecutor.java:
##########
@@ -1920,6 +1921,12 @@ public void executeAndSendResult(boolean isOutfileQuery,
boolean isSendFields,
if (!context.isReturnResultFromLocal()) {
profile.getSummaryProfile().setTempStartTime();
+ if (coordBase == coord) {
+ // A BE whose DoGet stream the client aborts reports it
through cancelFlightQuery;
+ // keep the query's backends so that call can cancel the
query on every one of
+ // them after the coordinator below is closed.
+ FlightSqlQueryCancellation.INSTANCE.register(context,
coord);
Review Comment:
Fixed in 0193881a505, and c528cf8a940 goes a step further. Agreed that such
a route can never be used: the request hands out no ticket, so no BE can report
an early close of its results. A GetFlightInfo that fails after it began, at
the schema fetch or because a prepared statement's result schema no longer
matches, now drops the routes of its endpoints before it answers; while routes
belonged to the session, its next request dropped them. Only the failing
request's routes go: a request that fails before it begins (an empty query)
finds the previous request's endpoints on the session and leaves their routes,
since those streams may still be open.
Such a failure also left the query running when it was not deferred: its
coordinator was closed already, so nothing cancelled it before the BE's result
buffer timer (execution_timeout + 5 s). c528cf8a940 sends the cancellation
through the route to every BE of such a query before the route goes. A deferred
query is still cancelled through its coordinator, and not a second time; the
prepared-statement branch has cancelled its query through the executor already,
so it only drops the route. The failure path does not wait for the BEs'
answers: the route goes whatever they are, and the client is waiting for the
error.
The effect of the stale route was smaller than described, though. A query
read to its end keeps its route until it expires as well, since a BE does not
report EOF to the FE, so a failed request took no more room than a successful
one; the bound fills only at about 110 registrations a second sustained
(default timeout, up to about 10 BEs a query), failed or not, and a query whose
route is pushed out ends at its timeout, as before this PR. I left the
registration where it is instead of moving it after the schema check: that is
where the coordinator is closed or deferred, and the statement and
prepared-statement paths would each need a registration of their own.
Tests:
`DorisFlightSqlProducerTest#testGetFlightInfoCancelsQueryAndDropsRouteWhenSchemaFetchFails`
(both BEs of the query get the cancellation and the route goes),
`#testGetFlightInfoLeavesDeferredQueryToItsCoordinator`,
`#testFailedRequestKeepsPreviousRequestRoute` (an empty query leaves the
previous request's route and cancels nothing),
`DorisFlightSqlSchemaTest#executionSchemaDriftExpiresHandleAndCleansResults`
(the prepared-statement case), and
`FlightSqlQueryCancellationTest#unregisterDropsWholeRoutes` and
`#failedRequestCancelsWithoutWaitingAndDropsRoutes`.
##########
be/src/service/arrow_flight/arrow_flight_batch_reader.cpp:
##########
@@ -103,6 +105,46 @@ void ArrowFlightBatchReaderBase::close(const Status&
reason) {
_statement->result_addr.port == config::brpc_port) {
env->result_mgr()->cancel_arrow_flight_query(_statement->query_id,
reason);
}
+ // A result endpoint can outlive its local fragment. Find the owning FE by
buffer ID,
+ // then use its query-wide cancellation route, including for older result
BEs.
+ for (const auto& [address, info] : env->get_running_frontends()) {
+ Status status;
+ // One retry: the FE keeps the route when an attempt fails (for
example a BE missed the
+ // FE's one-second deadline), and a cached connection to the FE may
have gone stale.
+ // Neither applies to an FE that cannot be connected to, so that one
gets no retry.
+ for (int attempt = 0; attempt < 2; ++attempt) {
+ // An FE that sorts before the one that planned the query delays
every early close on
+ // every BE. So the connection comes from a cache that holds only
transports opened
+ // with this 2 s timeout, and acquiring it does not back off
(max_retries = 0).
+ FrontendServiceConnection
client(env->flight_cancel_frontend_client_cache(), address,
Review Comment:
Fixed in 35aece4ee7f. The FEs the master reaches are now asked first, and
those it reports with process UUID 0 (unreachable, and kept on a BE's list
until they are dropped) after them. The FE that planned the query is reachable
unless it has lost the master's heartbeat since, and then it is still asked,
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 until the master's heartbeat to it fails.
I did not add an overall deadline: with this order it would only cut those
cases short, and in the last one the early close would lose its cancellation
instead of waiting for it. Probing the FEs concurrently would take a thread per
FE for every early close, since these thrift clients are synchronous. Sending
the cancellation only to the planning FE needs the ticket to name that FE,
which is left to a follow-up.
Test: `ArrowFlightRemoteCancelTest#CloseAsksReachableFrontendsFirst` puts an
FE that accepts connections and never answers, reported with process UUID 0,
ahead of the planning FE, and requires the close to finish within 1 s with one
call to the planning FE. It takes 14 ms; with the FEs asked in address order it
took 4.1 s.
--
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]
