gortiz opened a new pull request, #19411:
URL: https://github.com/apache/pinot/pull/19411
> **Draft until #19409 and #19410 merge.** This PR is stacked on both.
GitHub compares against
> `master`, so the diff shown here also contains their commits. **The change
under review here is the
> two commits "Select the stats store by name" and "Purge orphaned
statistics on request, not at
> startup" (+1,000).** I will rebase and mark this ready as the parents land.
Contributes to #18740. **Stacked on #19409 (contracts and stores) and #19410
(collection) — review those first.** Part of the split that replaces #18741.
This PR makes the feature configurable and operable: it selects a store by
name, wires collection
into broker startup behind `pinot.broker.stats.enabled`, and adds an
endpoint to reclaim statistics
for tables the broker no longer serves. Still no query-behavior change.
```text
┌──────────────────┐ ┌──────────────────┐ ┌──────────────────┐
┌──────────────┐ ┌──────────────┐
│ 1 — contracts │──▶│ 2 — collection │──▶│ 3 — this PR │──▶│ 4 —
planner │──▶│ 5 — join │
│ and the stores │ │ │ │ selection + │ │
wiring │ │ reordering │
│ │ │ │ │ purge endpoint │ │
│ │ │
└──────────────────┘ └──────────────────┘ └──────────────────┘
└──────────────┘ └──────────────┘
```
## Selecting a store
`pinot.broker.stats.store` resolves against `StatsStoreProvider`
implementations discovered through
`ServiceLoader`, each declaring its own name — the same pattern as
`TimeBoundaryStrategyService`.
The value is a **name, not a class name**. A class name in configuration is
a rename trap: moving or
renaming an implementation would silently invalidate an operator's config.
Duplicate names fail
loudly, and an unknown name fails startup listing the names that do exist,
rather than quietly
disabling statistics an operator explicitly asked for.
Discovery walks the plugin realms as well as the context classloader, since
a provider contributed
by a plugin is invisible to the context loader alone — which would make the
documented extension
seam unusable. The same provider class reached through two classloaders is
deduplicated by class
name, so a fat jar plus a plugin realm is not mistaken for a collision.
A provider's own failure is kept distinct from an operator typo: the typo
fails broker startup,
while a provider that cannot build a store from the given configuration
degrades to no-statistics.
Getting those two the wrong way round inverts the operator-visible outcome.
## Reclaiming orphaned statistics
A broker drops a table's statistics as soon as its routing entry goes away
(PR 2). That cannot cover
a table dropped **while the broker was down**: on restart no routing entry —
and so no listener — is
ever created for it, leaving rows nothing would revisit.
This is exposed as `DELETE /statistics/orphaned` rather than run
automatically, because a startup
sweep is wrong in the worst way. `BaseBrokerRoutingManager.init()` only
wires up ZooKeeper accessors;
routing entries are created later by Helix state transitions.
`routingExists()` would answer false
for every table and the sweep would delete the whole store on every boot,
logging it as tables the
broker no longer serves — making the store, its schema versioning and the
crc fast path pointless.
Moving it later does not fix it either. Routing is built asynchronously, so
before it settles "no
routing for T" also matches a table that has merely not loaded yet, and
purging then discards exactly
the column statistics that are expensive to re-fetch. There *is* a readiness
signal — service status
is GOOD only once CurrentState matches IdealState — but consuming it needs a
background waiter, and
an operator can lower `minResourcePercentForStart` and quietly break the
guarantee. An operator
calling this on a running broker removes the ambiguity entirely.
The liveness test and the purge are not atomic with respect to a routing
build, which publishes its
routing entry only after its listener has written that table's rows. A table
that becomes served in
that window has just had those rows deleted underneath a listener that
believes they are still
there; since a listener only removes segments its mirror knows about, the
loss would persist until
restart. The purge re-tests afterwards and hands such a listener a clean
slate instead.
## Reviewer notes
- **Off by default** (`pinot.broker.stats.enabled=false`) and broker-local:
no wire format, no
mixed-version concern, nothing to coordinate on a rolling upgrade.
- **New config keys**, all `[EXPERIMENTAL]`: `pinot.broker.stats.enabled`,
`pinot.broker.stats.store`, `pinot.broker.stats.dir` (the last applies
only to the `sqlite`
store). The default directory is
`<java.io.tmpdir>/<instanceId>/broker-stats` — per-instance,
because two brokers on one host sharing a single SQLite file would corrupt
it.
- **New REST endpoint** `DELETE /statistics/orphaned`, behind a new
`Actions.Cluster.DELETE_STATISTICS`
authorization action. Returns 404 when collection is disabled, so it is
inert on an unconfigured
broker.
- `BrokerAdminApiApplication` gains a nullable constructor parameter for the
stats manager; it is
bound only when statistics are enabled, and the resource injects it as
`@Optional`.
## Testing
- `StatsStoreFactoryTest` covers every configured value, the fail-fast path
for a typo, duplicate
names, and the deliberate split between a provider failure (degrade) and
an operator typo (fail
startup).
- `PinotBrokerStatisticsTest` covers the endpoint: the 404 when collection
is disabled, and that the
liveness predicate is actually wired to the routing table — an inverted
one would purge tables the
broker still serves.
- `BrokerStatsCollectionIntegrationTest` starts a **real broker** with
`pinot.broker.stats.enabled=true` against a real controller and ZooKeeper.
Everything else in this
stack is unit-tested against hand-built ZooKeeper records, which cannot
see the wiring: that the
listener provider is registered before the routing manager initialises,
that the
`pinot.broker.stats.*` subset reaches the provider, that the directory
default resolves, and that
sqlite-jdbc can load its native library in an assembled broker. Each of
those fails by silently
collecting nothing, so no other assertion would notice. It asserts that
segments announced through
Helix arrive in the store with the counts ZooKeeper carries, that the time
boundary of a
DAYS-typed column resolves to an instant rather than the day number, and
that
`DELETE /statistics/orphaned` is reachable and leaves a served table
alone. It justifies its own
cluster because the feature is off by default and cannot ride an existing
broker fixture.
--
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]
