You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "gortiz (via GitHub)" <gi...@apache.org> on 2026/08/31 13:55:38 UTC
[PR] Select the stats store by name and purge orphaned statistics on request [pinot]
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Select the stats store by name and purge orphaned statistics on request [pinot]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #19411:
URL: https://github.com/apache/pinot/pull/19411#issuecomment-5480100911
## [Codecov](https://app.codecov.io/gh/apache/pinot/pull/19411?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:x: Patch coverage is `0%` with `209 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 57.59%. Comparing base ([`80dbb7d`](https://app.codecov.io/gh/apache/pinot/commit/80dbb7d02dfd92ccc20ae2606ac712ea4f73d3cf?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`07bc0d8`](https://app.codecov.io/gh/apache/pinot/commit/07bc0d8eb7c33fee5ca337f94105d4a8323167e3?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
| [Files with missing lines](https://app.codecov.io/gh/apache/pinot/pull/19411?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [...not/query/planner/spi/stats/StatsAggregations.java](https://app.codecov.io/gh/apache/pinot/pull/19411?src=pr&el=tree&filepath=pinot-query-planner-spi%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fquery%2Fplanner%2Fspi%2Fstats%2FStatsAggregations.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtcXVlcnktcGxhbm5lci1zcGkvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3Bpbm90L3F1ZXJ5L3BsYW5uZXIvc3BpL3N0YXRzL1N0YXRzQWdncmVnYXRpb25zLmphdmE=) | 0.00% | [61 Missing :warning: ](https://app.codecov.io/gh/apache/pinot/pull/19411?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...inot/query/planner/spi/stats/ColumnStatistics.java](https://app.codecov.io/gh/apache/pinot/pull/19411?src=pr&el=tree&filepath=pinot-query-planner-spi%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fquery%2Fplanner%2Fspi%2Fstats%2FColumnStatistics.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtcXVlcnktcGxhbm5lci1zcGkvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3Bpbm90L3F1ZXJ5L3BsYW5uZXIvc3BpL3N0YXRzL0NvbHVtblN0YXRpc3RpY3MuamF2YQ==) | 0.00% | [43 Missing :warning: ](https://app.codecov.io/gh/apache/pinot/pull/19411?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...pinot/query/planner/spi/stats/TableStatistics.java](https://app.codecov.io/gh/apache/pinot/pull/19411?src=pr&el=tree&filepath=pinot-query-planner-spi%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fquery%2Fplanner%2Fspi%2Fstats%2FTableStatistics.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtcXVlcnktcGxhbm5lci1zcGkvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3Bpbm90L3F1ZXJ5L3BsYW5uZXIvc3BpL3N0YXRzL1RhYmxlU3RhdGlzdGljcy5qYXZh) | 0.00% | [29 Missing :warning: ](https://app.codecov.io/gh/apache/pinot/pull/19411?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...pinot/query/planner/spi/stats/ColumnValueType.java](https://app.codecov.io/gh/apache/pinot/pull/19411?src=pr&el=tree&filepath=pinot-query-planner-spi%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fquery%2Fplanner%2Fspi%2Fstats%2FColumnValueType.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtcXVlcnktcGxhbm5lci1zcGkvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3Bpbm90L3F1ZXJ5L3BsYW5uZXIvc3BpL3N0YXRzL0NvbHVtblZhbHVlVHlwZS5qYXZh) | 0.00% | [27 Missing :warning: ](https://app.codecov.io/gh/apache/pinot/pull/19411?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...query/planner/spi/stats/SegmentColumnStatsRow.java](https://app.codecov.io/gh/apache/pinot/pull/19411?src=pr&el=tree&filepath=pinot-query-planner-spi%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fquery%2Fplanner%2Fspi%2Fstats%2FSegmentColumnStatsRow.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtcXVlcnktcGxhbm5lci1zcGkvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3Bpbm90L3F1ZXJ5L3BsYW5uZXIvc3BpL3N0YXRzL1NlZ21lbnRDb2x1bW5TdGF0c1Jvdy5qYXZh) | 0.00% | [20 Missing :warning: ](https://app.codecov.io/gh/apache/pinot/pull/19411?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...pinot/query/planner/spi/stats/SegmentStatsRow.java](https://app.codecov.io/gh/apache/pinot/pull/19411?src=pr&el=tree&filepath=pinot-query-planner-spi%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fquery%2Fplanner%2Fspi%2Fstats%2FSegmentStatsRow.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtcXVlcnktcGxhbm5lci1zcGkvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3Bpbm90L3F1ZXJ5L3BsYW5uZXIvc3BpL3N0YXRzL1NlZ21lbnRTdGF0c1Jvdy5qYXZh) | 0.00% | [16 Missing :warning: ](https://app.codecov.io/gh/apache/pinot/pull/19411?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [.../pinot/query/planner/spi/stats/StatConfidence.java](https://app.codecov.io/gh/apache/pinot/pull/19411?src=pr&el=tree&filepath=pinot-query-planner-spi%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fquery%2Fplanner%2Fspi%2Fstats%2FStatConfidence.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtcXVlcnktcGxhbm5lci1zcGkvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3Bpbm90L3F1ZXJ5L3BsYW5uZXIvc3BpL3N0YXRzL1N0YXRDb25maWRlbmNlLmphdmE=) | 0.00% | [5 Missing :warning: ](https://app.codecov.io/gh/apache/pinot/pull/19411?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...y/planner/spi/stats/ColumnStatsFetchException.java](https://app.codecov.io/gh/apache/pinot/pull/19411?src=pr&el=tree&filepath=pinot-query-planner-spi%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fquery%2Fplanner%2Fspi%2Fstats%2FColumnStatsFetchException.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtcXVlcnktcGxhbm5lci1zcGkvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3Bpbm90L3F1ZXJ5L3BsYW5uZXIvc3BpL3N0YXRzL0NvbHVtblN0YXRzRmV0Y2hFeGNlcHRpb24uamF2YQ==) | 0.00% | [4 Missing :warning: ](https://app.codecov.io/gh/apache/pinot/pull/19411?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...t/query/planner/spi/stats/StatsStoreException.java](https://app.codecov.io/gh/apache/pinot/pull/19411?src=pr&el=tree&filepath=pinot-query-planner-spi%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fquery%2Fplanner%2Fspi%2Fstats%2FStatsStoreException.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtcXVlcnktcGxhbm5lci1zcGkvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3Bpbm90L3F1ZXJ5L3BsYW5uZXIvc3BpL3N0YXRzL1N0YXRzU3RvcmVFeGNlcHRpb24uamF2YQ==) | 0.00% | [4 Missing :warning: ](https://app.codecov.io/gh/apache/pinot/pull/19411?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
> :exclamation: There is a different number of reports uploaded between BASE (80dbb7d) and HEAD (07bc0d8). Click for more details.
>
> <details><summary>HEAD has 24 uploads less than BASE</summary>
>
>| Flag | BASE (80dbb7d) | HEAD (07bc0d8) |
>|------|------|------|
>|unittests|2|1|
>|java-25|6|1|
>|temurin|6|1|
>|lane-a|2|0|
>|integration|4|0|
>|integration1|2|0|
>|lane-b|2|0|
>|integration2|2|0|
>|unittests2|1|0|
></details>
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #19411 +/- ##
============================================
- Coverage 67.56% 57.59% -9.98%
============================================
Files 3486 2692 -794
Lines 224173 164194 -59979
Branches 35381 26651 -8730
============================================
- Hits 151462 94567 -56895
- Misses 60684 61620 +936
+ Partials 12027 8007 -4020
```
| [Flag](https://app.codecov.io/gh/apache/pinot/pull/19411/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [integration](https://app.codecov.io/gh/apache/pinot/pull/19411/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [integration1](https://app.codecov.io/gh/apache/pinot/pull/19411/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [integration2](https://app.codecov.io/gh/apache/pinot/pull/19411/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [java-25](https://app.codecov.io/gh/apache/pinot/pull/19411/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `57.59% <0.00%> (-9.98%)` | :arrow_down: |
| [lane-a](https://app.codecov.io/gh/apache/pinot/pull/19411/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [lane-b](https://app.codecov.io/gh/apache/pinot/pull/19411/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [temurin](https://app.codecov.io/gh/apache/pinot/pull/19411/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `57.59% <0.00%> (-9.98%)` | :arrow_down: |
| [unittests](https://app.codecov.io/gh/apache/pinot/pull/19411/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `57.59% <0.00%> (-9.97%)` | :arrow_down: |
| [unittests1](https://app.codecov.io/gh/apache/pinot/pull/19411/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `57.59% <0.00%> (-0.08%)` | :arrow_down: |
| [unittests2](https://app.codecov.io/gh/apache/pinot/pull/19411/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/pinot/pull/19411?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org