morningman commented on code in PR #69867:
URL: https://github.com/apache/doris/pull/69867#discussion_r4432228191
##########
fe/fe-core/src/main/java/org/apache/doris/arrowflight/DorisFlightSqlProducer.java:
##########
@@ -368,19 +392,126 @@ public FlightInfo getFlightInfoStatement(final
CommandStatementQuery request, fi
@Override
public FlightInfo getFlightInfoPreparedStatement(final
CommandPreparedStatementQuery command,
final CallContext context, final FlightDescriptor descriptor) {
- String[] handleParts =
command.getPreparedStatementHandle().toStringUtf8().split(":");
- String executedPeerIdentity = handleParts[0];
- String preparedStatementId = handleParts[1];
- ConnectContext connectContext =
flightSessionsManager.getConnectContext(executedPeerIdentity);
+ String preparedStatementId = preparedStatementId(context,
command.getPreparedStatementHandle());
+ ConnectContext connectContext =
flightSessionsManager.getConnectContext(context.peerIdentity());
return
FlightProtocolAdapter.of(connectContext).callCommand(connectContext,
- () -> executeQueryStatement(executedPeerIdentity,
connectContext,
- connectContext.getPreparedQuery(preparedStatementId),
descriptor));
+ () -> executePreparedStatement(context.peerIdentity(),
connectContext, preparedStatementId,
Review Comment:
Fixed in be929ac2e82. Confirmed: `executePreparedStatement` ran
`preparedQuery()` and `checkPreparedSchema()` before `executeQueryStatement()`
reached `FlightProtocolAdapter.beginRequest()`, and
`getFlightInfoPreparedStatement` has no catch of its own. A handle refused
there (missing, prepared in another namespace, a changed schema, a failed
re-analysis) therefore left the previous request's deferred coordinator, its
frontend-side result and its endpoints in place. Without this PR, every
prepared GetFlightInfo reached `executeQueryStatement()`; even a missing handle
failed inside its try, and the catch cancelled and finalized the previous
request's deferred coordinator.
`executePreparedStatement` now calls `beginRequest()` before the checks. A
prepared GetFlightInfo is the session's next request whatever the checks find,
and the call in `executeQueryStatement()` then finds nothing left. The call is
not in the command lock (`runCommand` / `callCommand`) or in `preparedQuery()`:
Prepare, GetSchema and the other metadata requests go through those too, and
they must leave the previous request alone (`FlightProtocolAdapter`'s class
comment, `FlightSessionOptionsTest#testTheOptionsLeaveTheLastRequestAlone`).
Two parts of the description go further than what happens:
- The state is not kept until the idle timeout whatever comes next. The
session's next statement, or its next prepared execution that passes the
checks, drops it, and so does session teardown. Only when neither arrives does
the timeout checker finalize the deferred coordinator, and its bound is
`arrow_flight_deferred_query_idle_timeout_second` (3600 s) or the query's own
execution timeout, whichever is longer, counted from that query's start, not
`wait_timeout`.
- Only a query whose coordinator must outlive GetFlightInfo, a batch-mode
external-table scan or a remote Doris scan (`coord.mustOutliveDispatch()`),
leaves a deferred coordinator; every other query closes its coordinator before
GetFlightInfo returns. As for the `sql_mode` example: `SET sql_mode = ...` is
itself a GetFlightInfo, which already dropped the previous query's coordinator
and leaves only its own OK result. For a schema change to leave a coordinator
behind, it has to come from outside the session's requests, such as
SetSessionOptions or another session's DDL or REVOKE.
The same commit expires a checked handle whose statement can no longer be
described when it is re-analyzed, for example after a column grant was revoked
so that the analyzed plan is denied, or after a DDL that turned a result column
into an untyped NULL. Such a handle answered UNIMPLEMENTED and was kept, so
every later execution failed the same way. The check now compares the schema
Prepare would advertise now, which for such a statement is the placeholder: the
handle is removed, the request answers NOT_FOUND "Prepared statement schema
changed; prepare again", and the statement prepared again keeps the placeholder
and runs unchecked.
Tests: `DorisFlightSqlSchemaTest` adds one test for each way the checks
refuse a handle: a missing handle, a namespace change, a sql_mode drift, a
dropped view (INVALID_ARGUMENT, the handle is kept) and a revoked column grant
(NOT_FOUND, the handle is removed, and preparing again gives the placeholder).
Each sends GetSchema and GetFlightInfo after a previous request left a deferred
executor, a frontend-side result and an endpoint, and checks the status,
whether the handle is kept, and that GetFlightInfo finalized and dropped all
three while GetSchema left them. Without the new `beginRequest()` call, the
five tests fail at GetFlightInfo; with the call in `preparedQuery()` instead,
they fail at GetSchema; with the previous UNIMPLEMENTED for a re-analysis that
cannot describe the result, only the revoked-grant test fails. branch-4.1 and
branch-4.2 check the handle in the same order.
--
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]
