jineshparakh opened a new pull request, #19503:
URL: https://github.com/apache/pinot/pull/19503
## Intent
Delete a table and its metric series keep being exported by the brokers,
servers and controllers that
were serving it — frozen at whatever value they last held — until each of
those processes is restarted.
That is not merely untidy:
* **Dashboards lie.** A panel filtered by table shows a row for a table that
no longer exists, holding
its final measurement forever.
* **Alerts fire on ghosts.** Gauges such as `percentOfReplicas`,
`segmentsInErrorState` or
`percentSegmentsAvailable` keep their last value. If a table was degraded
at the moment it was
dropped — which is common, since deletion drives replicas offline — an
alert can fire *after* the
table is gone and never clear.
* **Cardinality only grows.** In a cluster where tables are created and
dropped routinely, the metric
registry, the JMX MBean set and everything downstream of it accumulate
monotonically for the life of
the process.
The goal is that deleting a table stops its metrics, everywhere, without
waiting for a restart.
This PR does not do that on its own. It adds the one capability all three
components need, and the one
SPI accessor that capability requires. It is deliberately
**behaviour-neutral** — nothing calls it yet —
so that the API can be discussed separately from the three behaviour changes
that will build on it.
## Why the cleanup we have today doesn't achieve it
Removal is retrofitted per component, and every implementation reconstructs
metric names by replaying
the composition rules in `AbstractMetrics`. Reconstruction can only reach a
name that is *fully
derivable from the table name*. Several names are not, so we have three
hand-rolled sweeps, each
incomplete in a different way: the controller and server iterate their
metric enums and so miss
anything carrying an extra segment between the table and the metric name,
and the broker removes three
gauges and nothing else — no query counters, no phase timings, no time
boundary, no quota gauges.
Two current examples of the blind spot:
**`tableRebalanceExecutionTimeMs`** is registered as
`<table>.<jobStatus>.<timer>`, because
`TableRebalancer` passes a synthetic table name. The sweep composes
`<table>.<timer>`. It never matches,
for any of the seven `RebalanceResult.Status` values.
`cronSchedulerJobExecutionTimeMs` has the same
shape with a task type in place of the status.
**`OPEN_STRUCT_LAST_SEGMENT_KEY_DOC_COUNT`** is registered as
`<gauge>.<table>.<column>$<key>`. The
server's deletion handler already documents this exact failure, works around
it by recovering keys from
the table config, and concedes in its own javadoc that the workaround is
partial: *"Gauges for
discovered keys survive until the server restarts."*
Both are the same defect. **Emission is modelled; removal is not.** Every
new keyed metric silently
re-opens the hole, and nothing tells the author who added it.
## What this PR adds
A way to remove a table's series based on **what is actually registered**,
rather than on names we can
reconstruct — so keyed, composite and future name shapes are all reachable
without anyone having to
remember them.
Doing that needs one thing from the SPI: the ability to read a registered
metric's name back as a
string. `PinotMetricName` exposes only `Object getMetricName()`, and
`toString()` is not portable —
yammer renders a JMX object name there, dropwizard renders the bare name. So
the PR adds a single
`default String getName()` and overrides it in the in-tree implementations.
As a `default` method it is
source- and binary-compatible: a third-party metrics plugin keeps working,
and at worst the sweep
matches nothing for it, which is exactly today's behaviour.
Arguably this accessor is one the interface should have had regardless; the
sweep is just its first
consumer.
## The flow this unlocks
The follow-up PRs attach the sweep to each component's **existing**
table-deletion signal — no new
messages, no new protocol, no new ZooKeeper writes.
```
DELETE /tables/{table}
│
PinotHelixResourceManager.deleteTable
│
┌────────────────────────────────────┼────────────────────────────────────┐
│ ① removed from brokerResource │ ② TableDeletionMessage
│ ③ table-config znode
│ (happens FIRST) │ → every server
│ removed (LAST)
▼ ▼
▼
BROKER SERVER
CONTROLLER (every one)
Helix ONLINE→OFFLINE/DROPPED Helix USER_DEFINE_MSG ZK
child watch /CONFIGS/TABLE
│ │
│
▼ ▼
▼
BaseBrokerRoutingManager SegmentMessageHandlerFactory
(new) TableMetricsCleaner
.removeRoutingInternal .TableDeletionMessageHandler
│ │
│
└────────────────┬───────────────────┴────────────────────────────────────┘
▼
AbstractMetrics.removeTableMetrics(...) ← this PR
```
Two ordering facts the follow-ups have to respect, recorded here because
they are not obvious and each
cost a debugging cycle to find:
* **The table config is removed last.** A "does the table still exist in
ZooKeeper?" guard is therefore
useless at broker and server time — the config is still present when the
deletion becomes observable
there. Only the controller's watch can use the ZK snapshot that reported
the deletion, which *is*
authoritative at that instant.
* **Some series key off the raw table name**, not the name with type — the
deep store timers and byte
counters, and the broker's whole query path. Those are shared by the
OFFLINE and REALTIME halves of a
hybrid table, so they may only be swept once neither half remains.
Scope note: this is deletion-driven. A table that still exists but has
stopped being reported on —
disabled, ideal state unreadable, or leadership moved — remains
`SegmentStatusChecker`'s business, since
only it knows it stopped reporting.
## Approaches considered and rejected
**Fix the two known offenders.** Add a keyed timer remover and loop over
`RebalanceResult.Status`, then
do the same for the cron timer. Rejected as symptom-level: it leaves the
defect class intact, so the
next keyed metric leaks again and the person adding it gets no signal.
**Extend the enum-driven sweep properly** — add an abstract `getTimers()` to
`AbstractMetrics`, add
keyed `removeTableTimer` / `removeTableMeter` overloads, and have each
component iterate every key it
knows about. This was the first design. Rejected because it inherits the
very defect it is fixing: it
can still only remove names it can reconstruct, so it is complete only for
as long as someone keeps it
in sync with every emission site. It also makes `getTimers()` a compile
break for every subclass.
**Clean up inside `PinotHelixResourceManager#deleteTable`.** Superficially
the obvious place. Rejected
because deletion happens in *one* process while the series live in nearly
all of them: that controller
holds one metric registry out of N and cannot reach any broker's, any
server's, or any other
controller's. It is also edge-triggered in the worst way — a missed call
leaks forever, and it races a
delete-then-recreate.
**Parse `toString()` instead of adding an SPI method.** Rejected: the format
is implementation-specific
(a JMX object name on yammer, the bare name on dropwizard), so this would
mean implementation-specific
parsing inside a generic base class, silently wrong for any plugin we do not
know about.
**Reach the name reflectively through the existing `Object
getMetricName()`.** Technically workable for
the in-tree implementations. Rejected as an undocumented contract that would
break silently rather than
at compile time.
**Have `AbstractMetrics` index the names it registers** and sweep that
index, avoiding the SPI change
entirely. Genuinely attractive — ownership becomes exact by construction and
sweeps get cheaper, since
`allMetrics()` materialises a fresh map on some implementations. Rejected on
hot-path cost: it puts a
map write on every metric emission, and `addMeteredTableValue` runs several
times per query, to save one
`default` method. It also introduces a second source of truth that can drift
from the registry. The
registry scan pays its cost only on deletion, which is rare and
operator-driven.
## Backward compatibility
* **No wire surface.** `PinotMetricName` is confined to the metrics
packages, is not `Serializable`, and
appears in no message class, ZooKeeper znode or request/response type.
Metric names leave a process
only as JMX MBeans.
* **No emitted name changes**, so every exporter rule, dashboard and alert
keeps matching exactly what
it matched before.
* **`default` method**, so third-party metrics plugins remain source- and
binary-compatible.
* **Mixed versions are independent.** Each component has its own JVM,
registry and copy of
`pinot-common`; there is no shared metric state and nothing to negotiate.
In a partially upgraded
cluster each component simply behaves per its own version.
* **No behaviour change at all in this PR** — nothing calls the new method
yet.
## Testing
`AbstractMetricsTest` is abstract with three concrete subclasses, so the new
cases run against the
**fake**, **yammer** and **dropwizard** registries. They cover the names
reconstruction cannot reach
(including a keyed timer registered exactly as `TableRebalancer` emits it),
that a swept gauge can be
re-registered afterwards, whole-segment matching so neighbouring and
database-qualified tables are not
caught, that global series and other tables survive, that the shared
`allTables` aggregate cannot be
deleted, and that a second `AbstractMetrics` sharing the same registry and
prefix keeps its gauges.
```
Pinot SPI ................ SUCCESS
Pinot Common ............. SUCCESS 46 tests
Pinot Yammer Metrics ..... SUCCESS 20 tests
Pinot Dropwizard Metrics . SUCCESS 20 tests
Pinot Compound Metrics ... SUCCESS
checkstyle:check + license:check ... clean
```
--
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]
