Skip to content

fix(trino): observe shared pools through their live instances - #1222

Merged
fuziontech merged 1 commit into
mainfrom
james/trino-pool-observer
Sep 23, 2026
Merged

fuziontech merged 1 commit into
mainfrom
james/trino-pool-observer

Conversation

@fuziontech

Copy link
Copy Markdown
Member

Problem

A shared-pool Trino cell has no fixed coordinator. Its console observer and usage collector were still built from the cell's CoordinatorURL, which is empty for a pool (#1216). Three effects:

  • The admin console reports every org on a pool as available: false with connection: null, so the tenant host is never advertised.
  • The leader's usage collector logs unsupported protocol scheme "" every 10 seconds, and Trino usage on the pool is never metered.
  • The query list, kill and node views show nothing for the pool.

This blocks moving orgs onto the prod pool, which is the next step for PostHog's own org.

Change

  • trino_pool_observer.go: a TrinoCoordinatorClient for pools.
    • Each call lists the pool's instances from the config store, so any control-plane replica can use it without the operator lease.
    • It then asks every instance that can hold queries (ADMITTED, SERVING, DRAINING, SEALED, SUSPECT) on the instance's own Service, the same address the pool operator probes.
    • Queries and nodes are merged across members. One unreachable member doesn't hide the rest; the call fails only if every member fails.
    • A kill goes only to the member that holds the query.
    • With no members, the error names that, rather than failing against an empty URL.
  • admin.NewTrinoPoolMemberClient: the observer client for one pool member.
    • It declares the Gateway's forwarded HTTPS hop (X-Forwarded-Proto: https, X-Forwarded-Port: 443), as the operator's probes do.
    • It maps the forwarded nextUri (https://<svc>:443/...) back onto the plain Service. Continuations to another host or a non-statement path are refused.
  • Wiring: a shared-pool cell always gets the pool observer, for both the console and usage metering. A store that can't list pool instances is now a startup error, not a silent fallback to the empty URL.

Tests

  • Unit tests cover:
    • merging across members, with only live phases dialled
    • partial and total member failure
    • a pool with no members
    • kills reaching only the owning member
    • dropping the cached client of a retired instance
    • wiring: a pool cell gets the pool observer, a fixed cell keeps its coordinator client
    • forwarded headers and continuation rewriting, and that the fixed-coordinator client is unchanged
  • e2e: the trino_shared_pool_active harness assertion gains an [observer] stage. The org must read available: true and advertise a connection host.
  • go test -tags kubernetes ./controlplane/ ./controlplane/admin/ and just test-trino-admin pass locally. golangci-lint isn't installed locally, so I'm relying on CI for it.

Docs: docs/trino-cells.md and CLAUDE.md (Trino cell views).

Closes #1216.

🤖 Generated with Claude Code

https://claude.ai/code/session_01Nic9bR3LPTRLk56ntJ4M5Z

A shared-pool cell has no fixed coordinator, but its console observer and
usage collector were built from the cell's CoordinatorURL, which is empty.
Every pooled org read as unavailable with no advertised connection, and
Trino usage on the pool was never collected (#1216).

The pool observer lists the pool's instances from the config store on each
call and asks every instance that can hold queries on its own Service,
declaring the Gateway's forwarded HTTPS hop and mapping the forwarded
nextUri back onto the plain Service. Queries and nodes are unioned across
members; one unreachable member does not hide the others.

Closes #1216.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Nic9bR3LPTRLk56ntJ4M5Z
@fuziontech
fuziontech requested review from a team and benben September 23, 2026 20:34
@github-actions

Copy link
Copy Markdown

Test Impact Plan

Deterministic summary of how this PR changes tests, CI runners, and coverage-risk signals.

Summary

Area Added Changed Deleted
Test files 2 2 0
E2E/journey files 0 0 0
Workflow files 0 0 0

Signals

  • Test cases: +9 / -0
  • Assertions: +36 / -0
  • Skips or known failures added: 0
  • Workflow continue-on-error added: 0
  • Workflow path filters added: 0
  • Test commands removed from justfile: 0
  • E2E/journey retry lines added: 0

Coverage risk: neutral or increased

No coverage-reduction warnings detected.

@bill-ph bill-ph left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approval: no P0 blocker found.

Non-blocking metering caveat: trinoPoolObserver.Queries returns partial results with a nil error whenever at least one member responds. The usage collector treats nil error as a complete snapshot, while Trino retains terminal query records only in coordinator memory. If a member remains unreachable beyond that retention window, its completed-query usage can be silently missed. Please consider surfacing partial collection (for example via a metric or warning) so this limitation is observable.

The pool-member client confines continuation requests to the same service, and the fan-out, wiring, tests, and e2e assertion are directly scoped to shared-pool observation. No overengineering or scope creep found. CI still had unit tests, build, and Go analysis pending at review time.

— Robo Bill

@fuziontech
fuziontech merged commit bedf4b5 into main Sep 23, 2026
29 checks passed
@fuziontech
fuziontech deleted the james/trino-pool-observer branch September 23, 2026 21:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Shared-pool Trino cell has no observer coordinator URL: usage collection skipped, org Trino status unavailable

2 participants