morningman commented on PR #68101:
URL: https://github.com/apache/doris/pull/68101#issuecomment-5747849113
<!-- doris-repo-review:v1:begin -->
### Local pipeline review — ✅ PASS
```yaml
schema: doris-repo-review/v1
status: PASS
pr: apache/doris#68101
commit: b446ac89ac8dea4ef942d27033293d2743493b45
base: 4e3b9673ef93514910d4e73837fd6ca4e3d7d2f3
reviewed_at: 2026-09-20T13:23+08:00
reviewer: morningman
model: claude-opus-5[1m]
effort: max
findings: {blocker: 0, major: 0, minor: 4, nit: 11}
rounds: 2
converged: true
```
**Notes for maintainers**
-
`fe/fe-core/src/main/java/org/apache/doris/arrowflight/sessions/FlightSessionsWithTokenManager.java:59-66`
— `sessionCreationLock` is FE-wide and its critical section covers token
validation, the whole `ConnectContext` build (a Java-serialization clone of
`SessionVariable`) and any Guava removal-listener drain those cache reads
piggy-back (teardown of *other* sessions, incl. `closeTxn`), so every new
Flight session on the FE waits behind the slowest creation. Minor: no deadlock
(always the outermost lock), nothing leaked; a per-token `Striped` lock or a
narrower critical section removes the coupling. The reply to the bot says
"serialized per token" — the comment/PR body should say FE-wide, or the lock
should become per token.
- `fe/fe-core/src/main/java/org/apache/doris/mysql/AcceptListener.java:118`
— pre-existing, on the seam this PR rewrites: a MySQL connection is registered
before its OK packet is sent and before the close listener is installed, so an
`IOException` from `sendResponsePacket` leaves the context in the pool with
`startTime` 0 (never reaped, `KILL` cannot unregister it) until FE restart.
Install the close listener first, or unregister in the catch; the new "every
teardown path meets in `unregisterConnection`" comment is not true for this
path.
- Two stated guarantees have no test: check-then-increment atomicity at a
boundary
(`fe/fe-core/src/test/java/org/apache/doris/qe/ConnectPoolMgrTest.java:266-271`
claims the test fails without the lock; it does not — the frozen attempt parks
before the lock) and the re-validate block
(`FlightSessionsWithTokenManager.java:115-120`; only `thenReturn` stubs exist).
- Rebase onto the new base checked: `#67820`'s `MetricRepo.init()` TSO
registration runs before `MetricsTest`'s scoped mock; `#68221` restores the
`RESOURCE_EXHAUSTED` passthrough `test_connection_quota` asserts (confirmed at
all eight producer entry points); no master commit in the window added a caller
of the deleted `FlightSqlConnectPoolMgr` API.
- Not run locally: no build, FE unit test or regression suite (read-only
review); the author-reported green runs are taken from the PR body.
<sub>Reviewed locally with the `doris-repo-review` pipeline. Repository
policy may accept this receipt for the matching commit; it is not a human
Apache approval.</sub>
<!-- doris-repo-review:v1:end -->
--
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]
