morningman commented on PR #67966: URL: https://github.com/apache/doris/pull/67966#issuecomment-5675258353
Addressed in ec060968d2a + ad7c70ba0c5 (review target was 27615029d1d). Per finding: - **[P1] Reap each executor against its own frozen deadline** — fixed. `FlightProtocolAdapter.takeExpiredDeferredExecutors(now)` judges and removes each deferred executor in one synchronized pass, by its own deadline (`start + max(config, execTimeout)`); `ConnectContext.reapIdleFlightSqlDeferredExecutors` finalizes the taken ones outside the lock. The two aggregate getters (earliest start / longest timeout) are gone, so there is no `-1` sentinel to misread and no whole-list close: an executor deferred after the checker decided is not one it decided about, and whoever takes an executor out of the list (checker, `beginRequest`, teardown) is the only one that finalizes it. Tests: `FlightSqlDeferredQueryIdleTimeoutTest.testEachDeferredQueryIsBoundByItsOwnExecTimeout`, `testAnOverdueDeferredQueryDoesNotTakeAYoungerOneWithIt`, `testTheCheckerAndTheNextRequestFinalizeAnExecutorExactlyOnce` (both interleavings), `testAFailingFinalizationDoesNotStopTheOthers`. - **[P2] Freeze the deferred query's profiling state too** — fixed, one level up. `StmtExecutor` decides once, at the first profile update, whether the statement's profile is reported (`reportsProfile()`), and the final update follows that decision; this also covers a case older than this PR, a Flight query run with a `SET_VAR(enable_profile=true)` hint, whose hint is reverted before `FlightSqlConnectProcessor.close()` finalizes it. The final summary of a deferred query now adds only what ends it (end time, total time, task state, counted from its own start); everything recorded when it ran stays, as `SummaryProfile` merges by key, so the context's later catalog/database/state/variables do not reach the record. Test: `StmtExecutorTest.testADeferredQueryIsFinalizedFromItsOwnRecordNotTheSessions` (RUNNING published; then `enable_profile` off, the database left, a new query id and start time on the session; finalized from a thread without the session's context — finished, record untouched). - **[P2] Let clients clear the selected schema** — fixed. The empty value and the empty string both put the session back into no database (`ConnectContext.clearDatabase()`, which `SWITCH` now shares); `catalog` keeps refusing the empty value, a session always being in some catalog. Tested in `FlightSessionOptionsTest.testTheEmptyValueOrStringLeavesTheDatabase` (erase, `""` round trip, a user without privileges) and in the regression suite (a statement needs a database again after the clear). The contract these implement is now written down in `FlightProtocolAdapter`'s class comment: a query does not end where the frontend's command does, so the two things of a request that outlive the command — the frontend-side result cache and the deferred executors — are owned by the session's command stream and nothing else; a deferred executor is a closed object from the moment it is deferred (carries what finalizing it needs, reads nothing of the session later) and is finalized exactly once by whoever takes it out of the list. The frontend still has no signal for the moment a query is done on the backends; the next request stands in for it as it has since #62259. Giving the frontend that signal from the BE is the right follow-up and is out of this PR's scope. Locally: the six FE UT classes green (78 tests), fe-core/fe-common checkstyle 0 violations, `arrow_flight_sql_p0` 10 suites green against an FE built from ad7c70ba0c5. -- 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]
