morningman commented on PR #67966: URL: https://github.com/apache/doris/pull/67966#issuecomment-5673092005
Addressed in 27615029d1d (review target was 8e0fa7ebc5b; 532a7ea1613 in between only lowers a log level). Per finding: - **[P1] Refresh session activity for option-only commands** — fixed. `FlightProtocolAdapter.callCommand` now refreshes the session's start time for every command of the session (`ConnectContext.refreshStartTime`, the timestamp only, so what the last statement recorded is kept for a deferred query's profile). That covers GET, an empty SET, a SET refused before any statement runs, and also the metadata / prepared-statement actions and a frontend-side DoGet, which had the same gap. `DorisFlightSqlProducerTest.testSessionOptionActionsKeepTheSessionAlive` drives the three cases across the `wait_timeout` boundary. - **[P1] Bound prior-request resources independently of option traffic** — fixed the second way; the first is unsafe. An option action cannot call `beginRequest`: the frontend does not know whether the client's DoGet is done (JDBC's `setCatalog()` / ADBC's `set_options()` between execute and fetch would lose the cached result or release the coordinator the BE still fetches splits from). What is fixed: the idle reaper now counts from when the deferred query started (`StmtExecutor` captures it in `deferForArrowFlight`), as the config's own description says, so option traffic cannot extend it; and, found while testing this, a deferred query is now unregistered under its own query id — the SET statements an option runs give the context a new id, and `finalizeQuery` would otherwise unregister the wrong query and leak the deferred one's registration and instance count. The cached frontend-side result is bounded (one per session) and released by the next request as before. Tests: `Fl ightSqlDeferredQueryIdleTimeoutTest.testBoundCountsFromTheQuerysStartNotTheSessionsLastCommand`, `StmtExecutorTest.testDeferForArrowFlightFreezesTheQueryIdAndStartTime`, `FlightSessionOptionsTest.testTheOptionsLeaveTheLastRequestAlone` (real SET / USE after a cached result and a deferred query). - **[P1] Restrict SET to GetSessionOptions' key namespace** — fixed. A variable is an option under exactly the name `SHOW VARIABLES` / GET lists it by (`VariableMgr.getShownVarContext`, sharing `dump()`'s rule): another case, an experimental variable without its prefix (or a GA one with it), a retired (`REMOVED`) or invisible variable are `INVALID_NAME`. Tested in `FlightSessionOptionsTest.testAVariableIsNamedAsItIsReadBackAndOnlySo` and in the regression suite; verified with the ADBC driver 1.12.0 (`Query_Timeout`: "invalid name" on set, "unknown session option" on get — consistent now). - **[P2] Classify malformed catalog names as INVALID_VALUE** — fixed. `Util.checkCatalogAllRules` (the check `SWITCH` makes first) runs before the statement; it is syntactic, so nothing about existence leaks and the privilege-before-existence order is unchanged. Covered for root and for a user without privileges, and in the regression suite. - **[P2] Preserve non-value SET failures as ERROR** — not changed, comments corrected. `StmtExecutor.execute` rewrites every command failure to `ERR_UNKNOWN_ERROR`, so the state after the statement cannot tell a refused value from an overdue instance or a blocked statement kind; telling them apart would mean duplicating `SetVarOp` / `VariableMgr` validation ahead of the statement, or changing the executor's rewriting for every client (a separate change). The drivers make nothing of the distinction: the ADBC driver maps `INVALID_NAME` / `INVALID_VALUE` / `ERROR` all to `INVALID_ARGUMENT`, differing in one word; the reason is in fe.log at WARN. The comment in `setVariable` that claimed the value was all that was left to refuse now says this. - **[P2] Keep the regression table available for debugging** — fixed: table name hardcoded, dropped before use only. - **[P2] Advertise the newly supported session actions** — fixed, one step further: `listActions` names exactly the actions this producer implements (`CreatePreparedStatement`, `ClosePreparedStatement`, `SetSessionOptions`, `GetSessionOptions`, `CloseSession`) rather than the inherited list plus three, since the inherited list also advertised transactions and savepoints Doris answers `UNIMPLEMENTED`. `DorisFlightSqlProducerTest.testListActionsNamesTheImplementedActions`; verified with `pyarrow.flight`'s `list_actions`. Locally: the six FE UT classes green (73 tests), checkstyle 0 violations, `arrow_flight_sql_p0` 10 suites green against an FE built from this commit. -- 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]
