[Mohith26]

The gRPC broker transport dials a new ClientConn for every query and closes it when the query finishes, so every query pays a TCP handshake (and a full TLS handshake when TLSConfig is enabled) before the request is sent. gRPC connections are meant to be long-lived and multiplex concurrent RPCs as HTTP/2 streams, so this change keeps one connection per broker address and reuses it.

What changed:

• grpcBrokerClientTransport caches a *grpc.ClientConn per broker address. Concurrent queries share it.
• If a query fails with codes.Unavailable (broker restarted, connection broken), that connection is evicted from the cache and closed once its in-flight queries finish. The next query to that broker dials fresh. Other errors (bad SQL, deadline exceeded, decode errors) keep the connection.
• Each query's context is now always canceled when execute returns, so a stream abandoned on an early return (for example a decode error) is torn down instead of lingering on the shared connection.
• Dial options are built once and cached only on success, so a bad TLS config still fails at query time like before.
• New Connection.Close() releases the cached connections. It is a no-op for the HTTP transport, which does not own its http.Client. Documented in docs/grpc.md and the README.

Benchmark (BenchmarkGrpcBrokerTransport, added in this PR): in-process broker on loopback, small result (2 rows), go test ./pinot -run '^$' -bench BenchmarkGrpcBrokerTransport -benchmem -count 5, run twice on master and twice on this branch, median of 10 runs each, Apple Silicon arm64:

case	master	this PR	speedup	B/op	allocs/op
plaintext, serial	3082 us	651 us	4.7x	131 KiB -> 19 KiB	979 -> 324
plaintext, parallel	559 us	96 us	5.8x	143 KiB -> 19 KiB	984 -> 313
TLS, serial	3854 us	529 us	7.3x	269 KiB -> 19 KiB	1947 -> 328
TLS, parallel	872 us	60 us	14.6x	293 KiB -> 19 KiB	1956 -> 314

The machine was shared with other jobs, so absolute latencies are noisy; the allocation numbers are stable across runs. Over a real network the savings per query grow with round-trip time, since the TCP and TLS handshakes are what gets skipped.

Tests (pinot/grpcBrokerConnReuse_test.go), using a real gRPC server on loopback with a listener that counts accepted connections:

• many queries, serial and concurrent, use one TCP connection
• separate brokers get separate connections
• a query failing with Unavailable causes a redial on the next query
• query errors that are not Unavailable keep the connection
• an evicted connection stays open until in-flight queries finish
• an abandoned stream is canceled on the server
• Close closes connections and later queries fail; Connection.Close works for both transports
• a dial-option (TLS) error is not cached

Ran go build ./..., go vet ./..., gofmt -l, golangci-lint v2.12.2 (0 issues) and go test -race on all non-example packages (pass; pinot coverage 99.7%). I did not run the integration tests against a live Pinot quickstart.

I used AI assistance while writing this; I reviewed and tested the changes.

[xiangfu0]

Independent maintenance review (not a GitHub-approving review).

Validation on head c00a81d against base master:

• go build ./... ✅, make lint (golangci-lint v2.12.2) → 0 issues ✅
• make test (race + atomic coverage) ✅ — pinot 99.6%, pinot/proto 100%
• go build ./examples/... ✅; gormpinot unaffected
• Hosted CI on this head: Unit Tests ✅, Integration Tests ✅, Linter ✅, Snyk (code/license/security) ✅

Correctness / concurrency / API review: No blocking issues.

• The per-broker connection cache (acquireConn/releaseConn/Close) is fully serialized under t.mu; refcount is balanced and cannot leak or go negative. Eviction-on-Unavailable defers conn.Close() until in-flight queries drain, and the t.conns[address] == cached guard avoids closing a replacement connection.
• Lazy dialOptions caching only persists on success, so a bad TLS config still fails per-query as before.
• Per-query context cancellation tears down abandoned streams without cancelling in-flight work (stream is fully drained before cancel() on the success path).
• Close() while queries are in flight is safe: grpc.ClientConn.Close() is concurrency-safe and the harmless double-close returns ErrClientConnClosing.
• Public API is purely additive (Connection.Close() via io.Closer assertion); HTTP transport Close() is a no-op. Existing callers who never call Close() are unaffected.

Non-blocking note — Coveralls: the Coveralls - unit check reports −0.07% (to 98.76%). The only newly-uncovered lines are the if closeErr := conn.Close(); closeErr != nil defensive error branches in releaseConn/Close, which are not reachable with a healthy loopback connection. This is a cosmetic drop in defensive error-handling paths, not a test-quality regression; optionally, a tiny unit test exercising a conn.Close() error could clear it.

Merge gate: This PR requires a GitHub approving review covering the current head (REVIEW_REQUIRED), which a maintainer still needs to provide. Leaving ready.

[Mohith26]

Thanks for the review. I added the small test in abd8286: TestGrpcTransportCloseOfAlreadyClosedConnection closes the cached connection out from under the transport, so both conn.Close() error branches (in releaseConn and Close) run. Locally releaseConn and Close are now at 100% and the pinot package at 99.8% under go test -race -covermode atomic; golangci-lint v2.12.2 reports 0 issues.

[xiangfu0]

Updated independent maintenance review for the new head abd8286 (not a GitHub-approving review).

Scope since my last review: the only change on top of c00a81d is the test-only commit abd8286, which adds TestGrpcTransportCloseOfAlreadyClosedConnection to exercise the conn.Close() error branches in releaseConn and Close — exactly the non-blocking Coveralls note I raised. No production code changed.

Validation on head abd8286 against base master (80ed43b):

• go build ./... ✅, go vet ./... ✅, gofmt -l clean ✅
• make lint (golangci-lint v2.12.2) → 0 issues ✅
• make test (race + atomic coverage) ✅ — pinot 99.8%, pinot/proto 100%
• No coverage regression vs pristine master: pinot 99.7% → 99.8%; repo total 98.9% → 99.1% (local go tool cover)
• Hosted CI on this exact head: Linter ✅, Unit Tests ✅, Integration Tests ✅ (live Pinot 1.4.0), Coveralls +0.1% → 98.966% ✅, Snyk code/license/security ✅

Coverage note from last review — resolved. The −0.07% Coveralls dip is gone; the new test covers both previously-uncovered conn.Close() defensive branches. Coveralls now reports an increase.

Independent correctness re-check (separate reviewer): No blocking issues. Confirmed again:

• Refcounting is balanced — acquireConn/releaseConn are paired exactly once per successful acquire; dial/TLS-option failures return before touching the map, so nothing leaks.
• Eviction-on-Unavailable + deferred close are guarded by !evicted and the t.conns[address] == cached identity check, so a replacement connection is never wrongly closed; an evicted conn closes only once inFlight == 0.
• All shared state is mutated under t.mu. Close() racing an in-flight query is safe (grpc ClientConn.Close() is idempotent and the error is ignored on the release path).
• Per-query context is always cancelled via defer cancel(), tearing down abandoned streams.
• Public API is purely additive (Connection.Close() via io.Closer assertion; HTTP transport no-op).

Merge gate: This PR still requires a GitHub approving review covering the current head (REVIEW_REQUIRED / BLOCKED); reviews: 0. A maintainer approval is needed — this maintenance review does not satisfy that gate, and I will not self-approve. Leaving ready.