gortiz opened a new pull request, #19410:
URL: https://github.com/apache/pinot/pull/19410
> **Draft until #19409 merges.** This PR is stacked on it. GitHub compares
against `master`, so the
> diff shown here also contains #19409's two commits. **The change under
review here is the single
> commit "Collect broker statistics from the ZooKeeper metadata already
watched" (+2,238).** I will
> rebase and mark this ready as soon as the parent lands.
Contributes to #18740. **Stacked on #19409 (contracts and stores) — review
that one first.**
Part of the split that replaces #18741.
Nothing here changes a query plan. This PR fills the store; nothing reads it
until PR 4.
```text
┌──────────────────┐ ┌──────────────────┐ ┌──────────────────┐
┌──────────────┐ ┌──────────────┐
│ 1 — contracts │──▶│ 2 — this PR │──▶│ 3 — selection │──▶│ 4 —
planner │──▶│ 5 — join │
│ and the stores │ │ collection │ │ store by name + │ │
wiring │ │ reordering │
│ │ │ │ │ purge endpoint │ │
│ │ │
└──────────────────┘ └──────────────────┘ └──────────────────┘
└──────────────┘ └──────────────┘
```
## How collection works
The broker already subscribes to segment ZK metadata to build routing, so
collection rides that
existing stream rather than adding cluster traffic: row counts, sizes and
time boundaries are
effectively free. It attaches through the routing manager's generic
listener-provider seam, so the
routing manager itself needs no knowledge of statistics.
```text
┌─────────────────────────────┐
│ ZooKeeper segment metadata │
└──────────────┬──────────────┘
│ already watched to build routing
┌──────────────▼──────────────┐
│ SegmentZkMetadataFetcher │
└───────┬─────────────┬───────┘
existing consumer │ │ new, attached via the
│ │ listener-provider seam
┌─────────────────▼───┐ ┌─────▼───────────────────────┐
│ segment pruners │ │ TableStatsZkListener │
└─────────────────────┘ └─────────────┬───────────────┘
│ per-segment rows
│ (crc-reconciled)
┌─────────────▼───────────────┐
│ StatsStore (PR 1) │
└─────────────┬───────────────┘
│
┌─────────────▼───────────────┐
│ LogicalTableStatsResolver │
│ per-segment rows ──▶ the │
│ logical view of a table │
└─────────────┬───────────────┘
┊ PR 4
┌─────────────▼───────────────┐
│ planner │
└─────────────────────────────┘
```
Reconciliation is by crc, so a restart re-collects only what actually
changed, and segments that
leave the online set are dropped. Statistics for a table are released when
its routing entry goes
away, via a new `default onRoutingRemoved()` on the listener interface —
deliberately named for the
event it is, since routing is also removed when a broker stops serving a
table that still exists.
One listener failing there cannot abort the rest of the teardown.
## How a raw doc count becomes a trustworthy statistic
This is the part worth reviewing closely. Raw per-segment doc counts are
biased for several table
types, so rather than correcting the numbers we attach a confidence tier and
let consumers refuse
the ones that cannot be trusted.
```text
stored per-segment rows
│
├─ OFFLINE ──▶ sum committed segments ─────────────────────────▶
EXACT
│
├─ REALTIME ─▶ upsert or dedup?
│ │
│ ├─ yes ──▶ physical docs over-count ─────────▶
LOW
│ │ logical rows
│ │
│ └─ no ──▶ consuming segments?
│ │
│ ├─ yes ─▶ committed counts ─────▶
ESTIMATED
│ │ undercount fresh data
│ │
│ └─ no ─────────────────────────▶
EXACT
│
└─ HYBRID ───▶ split at the live time boundary ────────────────▶
capped at
(offline below, realtime above)
ESTIMATED,
never EXACT
│
▼
confidence == LOW ?
│ │
yes no
│ │
▼ ▼
treated as ABSENT used for estimates
(planner keeps
today's behavior)
```
When both realtime adjustments apply, the weaker wins.
### Unknown is not zero
Splitting a hybrid table at its boundary means one side can contribute rows
to a range while the
other structurally cannot. A side whose sub-range is empty contributes a
**certain zero** — no rows
fall in an empty interval, whatever the store knows. A side that must be
answered from statistics
and has none is **unknown**, and reporting that as zero would hand a planner
a confident "this table
is empty", which is a far worse input than no estimate at all.
### The time boundary is an instant, not a number
The boundary comes from `TimeBoundaryManager`, which already derives it in
epoch milliseconds and
owns the `DateTimeFormatSpec` needed to interpret `TimeBoundaryInfo`'s
formatted value. Re-deriving
it at the call site would be free to drift, and parsing the formatted value
directly is wrong for
any column stored in days, seconds or a numeric date pattern: it parses
cleanly as a number that is
not an instant.
## Known limitation, and the intended fix
Upsert and dedup tables are marked `LOW` and will therefore contribute
nothing to planning. That is
safe but blunt: those tables get no benefit at all. Keeping honest row
counts for them in the broker
would mean reproducing the upsert/dedup metadata (and its expensive data
structures) broker-side,
which is not worth it.
The intended fix, as follow-up work, is to stop deriving their statistics
from stored metadata and
ask the servers periodically instead — a scheduled
`SELECT $segmentName, MIN(col), MAX(col), COUNT(col) ... GROUP BY
$segmentName`. Query execution
already applies the upsert view, so replaced rows are filtered out by
valid-doc-ids and the result
counts only **live** rows. Those values slot into the same per-segment store
and would let these
tables carry an honest confidence instead of `LOW`. The price is a scheduled
scan, so it needs rate
limiting and off-peak scheduling, and consuming segments still need a
freshness policy.
## Reviewer notes
- **One interface change:** `SegmentZkMetadataFetchListener` gains a
`default onRoutingRemoved()`.
Source- and binary-compatible; existing implementers keep compiling.
- **Collection failures never fail a query.** Every store error degrades to
no-statistics.
- Statistics are still not reachable from a query:
`LogicalTableStatsResolver` has no consumer until
PR 4.
## Testing
- `BrokerTableStatsManagerTest` covers collection, crc reconciliation,
table-type confidence,
degradation when the store errors, the reconcile that repairs a mirror
which disagrees with the
store, and that a committed segment whose ZooKeeper metadata lacks a row
count contributes zero
rather than −1.
- `LogicalTableStatsResolverTest` covers the hybrid boundary split —
including that a range only one
side can serve reports *unknown* rather than zero when that side has no
statistics — and that a
failing provider or store costs precision rather than the query.
- `BrokerRoutingManagerTest` covers the removal dispatch and that one
failing listener cannot abort
teardown.
--
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]
