morningman commented on PR #67504: URL: https://github.com/apache/doris/pull/67504#issuecomment-5550313751
Thanks for the precise write-up, the problem is real and #67503 nails the mechanism. I reviewed the branch and pushed a revision on top of your commit (the PR allows maintainer edits); the description is updated to match. What changed and why: 1. **Reap the deferred query instead of killing the session.** What leaks is the deferred coordinator, not the session. Killing the session has a client-visible cost: `unregisterConnection` leaves the bearer token in the cache marked as already used, so the client's next call fails with `UserSession expire after access` and has to re-handshake. With a 1h default, every pooled / BI-tool Flight connection that idles for an hour would fail once on its next use. The checker now finalizes the session's deferred executors when the bound passes and leaves the session under `wait_timeout`. The config is renamed to `arrow_flight_deferred_query_idle_timeout_second` accordingly. 2. **Narrow the deferral gate to the queries that need it.** #64799 deferred every BE-served Flight query, but only an external-table scan in batch mode needs the coordinator after GetFlightInfo (the BE fetches splits from its split source during DoGet). Finalizing the FE side does not cancel BE execution (`reportExecStatus` accepts reports for an unregistered query; `Coordinator.close()` only releases the queue token and the split sources), so every other query now closes its coordinator at the end of GetFlightInfo again, exactly as before #64799. This removes the leak for internal tables entirely, with no timeout involved; the idle bound only has to cover batch-mode external scans. New predicates: `ScanNode.hasBatchSplitSource()` / `Coordinator.hasBatchSplitSource()`. 3. **Capture the exec timeout when the executor is deferred.** While the session sleeps `executor` is null, so `getExecTimeoutS()` returned the *current* session `query_timeout`; `SET_VAR` hint values are reverted at the end of `execute()`. A query run with `/*+ SET_VAR(query_timeout=7200) */` would have been cut at 3600s, the case the floor is meant to protect. `StmtExecutor.deferForArrowFlight()` now freezes the value and the bound uses the max over the deferred executors. 4. **Tests.** `FlightSqlSessionIdleTimeoutTest` (formula only) is replaced by `FlightSqlDeferredQueryIdleTimeoutTest`, which drives `checkTimeout` and asserts both the release and that the session is not killed; `ArrowFlightDeferralGateTest` covers the predicates; `StmtExecutorTest` covers the frozen timeout. Two regression cases: `arrow_flight_sql_p0/test_arrow_flight_query_release` (internal table no longer occupies a `max_concurrency=1` group) and an extension of `test_iceberg_arrow_flight_split_source` (batch-mode scan stays registered, then is released by the reaper while the session keeps working). 5. Smaller things from the review: the `Config` description now states the floor and that the session is not killed; `masterOnly = false` dropped; `FeConstants.runningUnitTest` is restored after the test. Pick labels: #64799 is in 4.0.8 / 4.1.4, so this needs `dev/4.0.x` and `dev/4.1.x`. -- 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]
