Add AbstractMetrics#removeTableMetrics to drop every metric series for a table - #19503
Add AbstractMetrics#removeTableMetrics to drop every metric series for a table#19503jineshparakh wants to merge 1 commit into
AbstractMetrics#removeTableMetrics to drop every metric series for a table#19503Conversation
Signed-off-by: Jinesh Parakh <jineshparakh@hotmail.com>
jineshparakh
added
enhancement
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #19503 +/- ##
============================================
+ Coverage 67.59% 67.72% +0.13%
- Complexity 1430 1431 +1
============================================
Files 3487 3490 +3
Lines 224320 224666 +346
Branches 35417 35476 +59
============================================
+ Hits 151623 152166 +543
+ Misses 60679 60464 -215
- Partials 12018 12036 +18
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
xiangfu0
left a comment
xiangfu0
left a comment
There was a problem hiding this comment.
Reviewed this head across the eight Pinot review domains. Three substantive issues are detailed inline and were reproduced locally. The 60 shared AbstractMetrics tests passed across fake, Yammer, and Dropwizard registries; compound metrics compiled successfully. The issues affect the new cleanup API when invoked; this PR adds no production callers.
| int start = firstDot > 0 && gaugeNames.contains(name.substring(0, firstDot)) ? firstDot + 1 : 0; | ||
| for (String tableName : tableNames) { | ||
| int end = start + tableName.length(); | ||
| if (name.startsWith(tableName, start) && (end == name.length() || name.charAt(end) == '.')) { |
There was a problem hiding this comment.
[P2 / MAJOR] Preserve exact table identity when matching registered names
The delimiter and gauge-name heuristics can delete unrelated metrics as well as miss the requested table. I reproduced these cases against the compiled PR implementation:
- Register a meter and gauge for
db.foo_OFFLINE, then callremoveTableMetrics("db"): both are removed, although they belong to a different, database-qualified table. - Register
ControllerGauge.REALTIME_TABLE_COUNT, then sweeprealtimeTableCount: the global gauge is removed. - Register a meter for
numberOfReplicas.foo_OFFLINE: sweeping that table removes nothing, while sweeping unrelatedfoo_OFFLINEremoves the meter because the database name is interpreted as a gauge name.
The API explicitly accepts raw table names, so these inputs are within its contract. Please preserve table identity and metric scope at registration, or otherwise make matching unambiguous, and add regression coverage for these collisions. A dot boundary alone cannot distinguish database qualification from metric suffixes.
| } | ||
| // Re-deriving the key under this class is the ownership test: an identically named series registered by a | ||
| // sibling AbstractMetrics is a different key, so it compares unequal and is left for that instance to sweep. | ||
| if (registeredName.equals(PinotMetricUtils.makePinotMetricName(_clazz, name))) { |
There was a problem hiding this comment.
[P2 / MAJOR] Preserve ValidationMetrics ownership on Dropwizard
DropwizardMetricName discards the owning class and compares only the name string, so re-deriving the key with _clazz does not establish ownership on that backend. With a shared Dropwizard registry, I reproduced:
ValidationMetrics.updateMissingSegmentCountGauge("foo_OFFLINE", 1)registers one gauge.ControllerMetrics.removeTableMetrics("foo_OFFLINE")removes it.- Updating the validation gauge again leaves the registry empty:
ValidationMetricsstill has its private_gaugeValuesentry and skips re-registration.
This contradicts the documented exclusion of ValidationMetrics and can leave its series absent across subsequent updates. Please preserve ownership independently of backend key equality, or explicitly exclude these foreign registrations, with a Dropwizard regression that verifies the validation gauge survives and remains updateable.
| // gauge from ever coming back. Swept from this instance's own map rather than from what matched above, so it | ||
| // stays correct even where the registry cannot tell two instances' series apart. | ||
| synchronized (_gaugeValues) { | ||
| _gaugeValues.keySet().removeIf(gaugeName -> matchesAnyTable(gaugeName, targets, gaugeNames)); |
There was a problem hiding this comment.
[P2 / MAJOR] Keep registry removal and gauge-value cleanup atomic
A gauge can be registered after the registry snapshot above but before this removeIf. It is then absent from the removal snapshot, so its registry entry survives, while this code deletes its backing _gaugeValues entry. The supplier installed by setValueOfGauge calls _gaugeValues.get(gaugeName).get() and throws NullPointerException on scrape until another update.
I reproduced this deterministically with latches around snapshot/registration: the sweep returned 0, the registry retained one gauge, its backing value was null, and reading the gauge threw at AbstractMetrics.java:436.
Please coordinate the snapshot, registry removal, and backing-value cleanup with the same monitor used by gauge registration, as the existing targeted removeGauge does for its two removals. Add a barrier-controlled regression for registration during the sweep.
3 participants
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:
its final measurement forever.
percentOfReplicas,segmentsInErrorStateorpercentSegmentsAvailablekeep their last value. If a table was degraded at the moment it wasdropped — which is common, since deletion drives replicas offline — an alert can fire after the
table is gone and never clear.
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 fullyderivable 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:
tableRebalanceExecutionTimeMsis registered as<table>.<jobStatus>.<timer>, becauseTableRebalancerpasses a synthetic table name. The sweep composes<table>.<timer>. It never matches,for any of the seven
RebalanceResult.Statusvalues.cronSchedulerJobExecutionTimeMshas the sameshape with a task type in place of the status.
OPEN_STRUCT_LAST_SEGMENT_KEY_DOC_COUNTis registered as<gauge>.<table>.<column>$<key>. Theserver'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.
PinotMetricNameexposes onlyObject getMetricName(), andtoString()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 adefaultmethod it issource- 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.
Two ordering facts the follow-ups have to respect, recorded here because they are not obvious and each
cost a debugging cycle to find:
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.
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, sinceonly it knows it stopped reporting.
Approaches considered and rejected
Fix the two known offenders. Add a keyed timer remover and loop over
RebalanceResult.Status, thendo 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()toAbstractMetrics, addkeyed
removeTableTimer/removeTableMeteroverloads, and have each component iterate every key itknows 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. Rejectedbecause 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 forthe in-tree implementations. Rejected as an undocumented contract that would break silently rather than
at compile time.
Have
AbstractMetricsindex the names it registers and sweep that index, avoiding the SPI changeentirely. 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 amap write on every metric emission, and
addMeteredTableValueruns several times per query, to save onedefaultmethod. It also introduces a second source of truth that can drift from the registry. Theregistry scan pays its cost only on deletion, which is rare and operator-driven.
Backward compatibility
PinotMetricNameis confined to the metrics packages, is notSerializable, andappears in no message class, ZooKeeper znode or request/response type. Metric names leave a process
only as JMX MBeans.
it matched before.
defaultmethod, so third-party metrics plugins remain source- and binary-compatible.pinot-common; there is no shared metric state and nothing to negotiate. In a partially upgradedcluster each component simply behaves per its own version.
Testing
AbstractMetricsTestis abstract with three concrete subclasses, so the new cases run against thefake, yammer and dropwizard registries. They cover the names reconstruction cannot reach
(including a keyed timer registered exactly as
TableRebalanceremits it), that a swept gauge can bere-registered afterwards, whole-segment matching so neighbouring and database-qualified tables are not
caught, that global series and other tables survive, that the shared
allTablesaggregate cannot bedeleted, and that a second
AbstractMetricssharing the same registry and prefix keeps its gauges.