xiangfu0 commented on code in PR #19503:
URL: https://github.com/apache/pinot/pull/19503#discussion_r3954577446
##########
pinot-common/src/main/java/org/apache/pinot/common/metrics/AbstractMetrics.java:
##########
@@ -711,6 +718,106 @@ public String composePluginGaugeName(String pluginName,
Gauge gauge) {
return gauge.getGaugeName() + "." + pluginName;
}
+ /// Removes every series this instance registered for the given table.
+ ///
+ /// Unlike the targeted `removeTable*` methods, this does not rebuild names
from the rules used to emit them -- it
+ /// scans what is actually registered. That is the whole point. A series
emitted with an extra key, or with a
+ /// composite table name, embeds a segment no caller can rediscover from the
table name alone, so a sweep built on
+ /// reconstruction strands exactly those series and keeps stranding each new
one that gets added.
+ ///
+ /// Matching is deliberately narrow:
+ ///
+ /// - Only names under this instance's metric prefix are considered, so a
table named after a component
+ /// (`broker`) cannot match the prefix itself.
+ /// - The table name must occupy whole `.`-delimited segments, never part
of one -- `foo` does not match
+ /// `foobar`, and a database-qualified `db.tbl_OFFLINE` matches only as
a unit.
+ /// - A sibling [AbstractMetrics] sharing this registry and prefix keeps
its **gauges**. The ownership check
+ /// below -- re-deriving the key under this instance's class -- is exact
only where the registry key carries
+ /// the owning class; yammer's does, dropwizard's discards it. What
protects the case that actually matters,
+ /// on every implementation, is the vocabulary check above: a sibling's
gauge name is absent from this
+ /// instance's [#getGauges()], so `<siblingGauge>.<table>` reads as
meter-shaped and the table is not at
+ /// offset 0, so it cannot match. Gauges are the only kind with a
re-registration gate ([#_gaugeValues]), so
+ /// dropping one from under its owner would silence it for the life of
the process. A sibling's meter or
+ /// timer may be dropped early where the key cannot distinguish owners;
that is harmless -- they carry no
+ /// gate and re-register on the next emission. Every instance should
still run its own sweep, since that is
+ /// what clears its own [#_gaugeValues].
+ ///
+ /// A table folded into the shared `allTables` aggregate is safe without a
special case: no registered name
+ /// contains its name, so nothing matches. Passing `allTables` itself is
rejected for the same reason it would be
+ /// a disaster -- it would delete the aggregate for every table at once.
+ ///
+ /// Two residual false positives are accepted: a workload or remote-cluster
name exactly equal to a table name
+ /// sits in the same slot and would be swept. Both re-register on next use,
so the cost is one counter reset.
+ ///
+ /// Two things this deliberately does **not** reach, both of which need
their owner to clean up:
+ ///
+ /// - Series a component registers outside any [AbstractMetrics] --
[ValidationMetrics] composes its own
+ /// `pinot.controller.<table>.<gauge>` names against its own class and
keeps its own value map, so the
+ /// ownership check above skips it. Dropping its registry entries from
here would strand that map and retire
+ /// those gauges for the life of the process.
+ /// - Names where the table is not followed by a path separator, such as
the consumer client id form
+ /// `<gauge>.<table>-<topic>-<partition>`. Matching those would mean
accepting any prefix match, which is
+ /// what makes `tbl` match `tbl_OFFLINE` and `db.tbl`.
+ ///
+ /// @param tableName the table to sweep, in whichever name form its emitters
used (raw or with type)
+ /// @return the number of series removed
+ public int removeTableMetrics(String tableName) {
+ return removeTableMetrics(List.of(tableName));
+ }
+
+ /// Like [#removeTableMetrics(String)], for several tables at once. Prefer
this when sweeping a batch: the
+ /// registry is scanned once per call, and yammer and dropwizard both
materialise a fresh map on every
+ /// `allMetrics()`.
+ public int removeTableMetrics(Collection<String> tableNames) {
+ Set<String> targets = tableNames.stream().filter(t ->
!ALL_TABLES.equals(t)).collect(Collectors.toSet());
+ if (targets.isEmpty()) {
+ return 0;
+ }
+ Set<String> gaugeNames =
+
Arrays.stream(getGauges()).map(Gauge::getGaugeName).collect(Collectors.toCollection(HashSet::new));
+ int removed = 0;
+ // Snapshot the keys before mutating: the compound registry hands back its
live map.
+ for (PinotMetricName registeredName : new
ArrayList<>(_metricsRegistry.allMetrics().keySet())) {
+ String name = registeredName.getName();
+ if (!name.startsWith(_metricPrefix)
+ || !matchesAnyTable(name.substring(_metricPrefix.length()), targets,
gaugeNames)) {
+ continue;
+ }
+ // 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))) {
+ PinotMetricUtils.removeMetric(_metricsRegistry, registeredName);
+ removed++;
+ }
+ }
+ // The deprecated gauge paths gate re-registration on _gaugeValues, so an
entry left here would stop a removed
+ // 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));
+ }
+ return removed;
+ }
+
+ /// Whether the prefix-stripped metric name names one of the given tables.
+ ///
+ /// The table sits at exactly one offset, decided by the shape: gauges
compose `<gauge>.<table>[.<key>]`, while
+ /// meters, timers and query phases compose `<table>.<rest>`. Which one
applies is settled by asking whether the
+ /// leading segment is a known gauge name -- and that question is what keeps
a bare `tbl_OFFLINE` from matching
+ /// `db.tbl_OFFLINE`, a genuinely different table whose series must survive.
A free search for the name anywhere
+ /// in the string cannot tell those two apart.
+ private static boolean matchesAnyTable(String name, Set<String> tableNames,
Set<String> gaugeNames) {
+ int firstDot = name.indexOf('.');
+ 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) == '.')) {
Review 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 call
`removeTableMetrics("db")`: both are removed, although they belong to a
different, database-qualified table.
- Register `ControllerGauge.REALTIME_TABLE_COUNT`, then sweep
`realtimeTableCount`: the global gauge is removed.
- Register a meter for `numberOfReplicas.foo_OFFLINE`: sweeping that table
removes nothing, while sweeping unrelated `foo_OFFLINE` removes 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.
##########
pinot-common/src/main/java/org/apache/pinot/common/metrics/AbstractMetrics.java:
##########
@@ -711,6 +718,106 @@ public String composePluginGaugeName(String pluginName,
Gauge gauge) {
return gauge.getGaugeName() + "." + pluginName;
}
+ /// Removes every series this instance registered for the given table.
+ ///
+ /// Unlike the targeted `removeTable*` methods, this does not rebuild names
from the rules used to emit them -- it
+ /// scans what is actually registered. That is the whole point. A series
emitted with an extra key, or with a
+ /// composite table name, embeds a segment no caller can rediscover from the
table name alone, so a sweep built on
+ /// reconstruction strands exactly those series and keeps stranding each new
one that gets added.
+ ///
+ /// Matching is deliberately narrow:
+ ///
+ /// - Only names under this instance's metric prefix are considered, so a
table named after a component
+ /// (`broker`) cannot match the prefix itself.
+ /// - The table name must occupy whole `.`-delimited segments, never part
of one -- `foo` does not match
+ /// `foobar`, and a database-qualified `db.tbl_OFFLINE` matches only as
a unit.
+ /// - A sibling [AbstractMetrics] sharing this registry and prefix keeps
its **gauges**. The ownership check
+ /// below -- re-deriving the key under this instance's class -- is exact
only where the registry key carries
+ /// the owning class; yammer's does, dropwizard's discards it. What
protects the case that actually matters,
+ /// on every implementation, is the vocabulary check above: a sibling's
gauge name is absent from this
+ /// instance's [#getGauges()], so `<siblingGauge>.<table>` reads as
meter-shaped and the table is not at
+ /// offset 0, so it cannot match. Gauges are the only kind with a
re-registration gate ([#_gaugeValues]), so
+ /// dropping one from under its owner would silence it for the life of
the process. A sibling's meter or
+ /// timer may be dropped early where the key cannot distinguish owners;
that is harmless -- they carry no
+ /// gate and re-register on the next emission. Every instance should
still run its own sweep, since that is
+ /// what clears its own [#_gaugeValues].
+ ///
+ /// A table folded into the shared `allTables` aggregate is safe without a
special case: no registered name
+ /// contains its name, so nothing matches. Passing `allTables` itself is
rejected for the same reason it would be
+ /// a disaster -- it would delete the aggregate for every table at once.
+ ///
+ /// Two residual false positives are accepted: a workload or remote-cluster
name exactly equal to a table name
+ /// sits in the same slot and would be swept. Both re-register on next use,
so the cost is one counter reset.
+ ///
+ /// Two things this deliberately does **not** reach, both of which need
their owner to clean up:
+ ///
+ /// - Series a component registers outside any [AbstractMetrics] --
[ValidationMetrics] composes its own
+ /// `pinot.controller.<table>.<gauge>` names against its own class and
keeps its own value map, so the
+ /// ownership check above skips it. Dropping its registry entries from
here would strand that map and retire
+ /// those gauges for the life of the process.
+ /// - Names where the table is not followed by a path separator, such as
the consumer client id form
+ /// `<gauge>.<table>-<topic>-<partition>`. Matching those would mean
accepting any prefix match, which is
+ /// what makes `tbl` match `tbl_OFFLINE` and `db.tbl`.
+ ///
+ /// @param tableName the table to sweep, in whichever name form its emitters
used (raw or with type)
+ /// @return the number of series removed
+ public int removeTableMetrics(String tableName) {
+ return removeTableMetrics(List.of(tableName));
+ }
+
+ /// Like [#removeTableMetrics(String)], for several tables at once. Prefer
this when sweeping a batch: the
+ /// registry is scanned once per call, and yammer and dropwizard both
materialise a fresh map on every
+ /// `allMetrics()`.
+ public int removeTableMetrics(Collection<String> tableNames) {
+ Set<String> targets = tableNames.stream().filter(t ->
!ALL_TABLES.equals(t)).collect(Collectors.toSet());
+ if (targets.isEmpty()) {
+ return 0;
+ }
+ Set<String> gaugeNames =
+
Arrays.stream(getGauges()).map(Gauge::getGaugeName).collect(Collectors.toCollection(HashSet::new));
+ int removed = 0;
+ // Snapshot the keys before mutating: the compound registry hands back its
live map.
+ for (PinotMetricName registeredName : new
ArrayList<>(_metricsRegistry.allMetrics().keySet())) {
+ String name = registeredName.getName();
+ if (!name.startsWith(_metricPrefix)
+ || !matchesAnyTable(name.substring(_metricPrefix.length()), targets,
gaugeNames)) {
+ continue;
+ }
+ // 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))) {
Review 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:
1. `ValidationMetrics.updateMissingSegmentCountGauge("foo_OFFLINE", 1)`
registers one gauge.
2. `ControllerMetrics.removeTableMetrics("foo_OFFLINE")` removes it.
3. Updating the validation gauge again leaves the registry empty:
`ValidationMetrics` still has its private `_gaugeValues` entry 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.
##########
pinot-common/src/main/java/org/apache/pinot/common/metrics/AbstractMetrics.java:
##########
@@ -711,6 +718,106 @@ public String composePluginGaugeName(String pluginName,
Gauge gauge) {
return gauge.getGaugeName() + "." + pluginName;
}
+ /// Removes every series this instance registered for the given table.
+ ///
+ /// Unlike the targeted `removeTable*` methods, this does not rebuild names
from the rules used to emit them -- it
+ /// scans what is actually registered. That is the whole point. A series
emitted with an extra key, or with a
+ /// composite table name, embeds a segment no caller can rediscover from the
table name alone, so a sweep built on
+ /// reconstruction strands exactly those series and keeps stranding each new
one that gets added.
+ ///
+ /// Matching is deliberately narrow:
+ ///
+ /// - Only names under this instance's metric prefix are considered, so a
table named after a component
+ /// (`broker`) cannot match the prefix itself.
+ /// - The table name must occupy whole `.`-delimited segments, never part
of one -- `foo` does not match
+ /// `foobar`, and a database-qualified `db.tbl_OFFLINE` matches only as
a unit.
+ /// - A sibling [AbstractMetrics] sharing this registry and prefix keeps
its **gauges**. The ownership check
+ /// below -- re-deriving the key under this instance's class -- is exact
only where the registry key carries
+ /// the owning class; yammer's does, dropwizard's discards it. What
protects the case that actually matters,
+ /// on every implementation, is the vocabulary check above: a sibling's
gauge name is absent from this
+ /// instance's [#getGauges()], so `<siblingGauge>.<table>` reads as
meter-shaped and the table is not at
+ /// offset 0, so it cannot match. Gauges are the only kind with a
re-registration gate ([#_gaugeValues]), so
+ /// dropping one from under its owner would silence it for the life of
the process. A sibling's meter or
+ /// timer may be dropped early where the key cannot distinguish owners;
that is harmless -- they carry no
+ /// gate and re-register on the next emission. Every instance should
still run its own sweep, since that is
+ /// what clears its own [#_gaugeValues].
+ ///
+ /// A table folded into the shared `allTables` aggregate is safe without a
special case: no registered name
+ /// contains its name, so nothing matches. Passing `allTables` itself is
rejected for the same reason it would be
+ /// a disaster -- it would delete the aggregate for every table at once.
+ ///
+ /// Two residual false positives are accepted: a workload or remote-cluster
name exactly equal to a table name
+ /// sits in the same slot and would be swept. Both re-register on next use,
so the cost is one counter reset.
+ ///
+ /// Two things this deliberately does **not** reach, both of which need
their owner to clean up:
+ ///
+ /// - Series a component registers outside any [AbstractMetrics] --
[ValidationMetrics] composes its own
+ /// `pinot.controller.<table>.<gauge>` names against its own class and
keeps its own value map, so the
+ /// ownership check above skips it. Dropping its registry entries from
here would strand that map and retire
+ /// those gauges for the life of the process.
+ /// - Names where the table is not followed by a path separator, such as
the consumer client id form
+ /// `<gauge>.<table>-<topic>-<partition>`. Matching those would mean
accepting any prefix match, which is
+ /// what makes `tbl` match `tbl_OFFLINE` and `db.tbl`.
+ ///
+ /// @param tableName the table to sweep, in whichever name form its emitters
used (raw or with type)
+ /// @return the number of series removed
+ public int removeTableMetrics(String tableName) {
+ return removeTableMetrics(List.of(tableName));
+ }
+
+ /// Like [#removeTableMetrics(String)], for several tables at once. Prefer
this when sweeping a batch: the
+ /// registry is scanned once per call, and yammer and dropwizard both
materialise a fresh map on every
+ /// `allMetrics()`.
+ public int removeTableMetrics(Collection<String> tableNames) {
+ Set<String> targets = tableNames.stream().filter(t ->
!ALL_TABLES.equals(t)).collect(Collectors.toSet());
+ if (targets.isEmpty()) {
+ return 0;
+ }
+ Set<String> gaugeNames =
+
Arrays.stream(getGauges()).map(Gauge::getGaugeName).collect(Collectors.toCollection(HashSet::new));
+ int removed = 0;
+ // Snapshot the keys before mutating: the compound registry hands back its
live map.
+ for (PinotMetricName registeredName : new
ArrayList<>(_metricsRegistry.allMetrics().keySet())) {
+ String name = registeredName.getName();
+ if (!name.startsWith(_metricPrefix)
+ || !matchesAnyTable(name.substring(_metricPrefix.length()), targets,
gaugeNames)) {
+ continue;
+ }
+ // 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))) {
+ PinotMetricUtils.removeMetric(_metricsRegistry, registeredName);
+ removed++;
+ }
+ }
+ // The deprecated gauge paths gate re-registration on _gaugeValues, so an
entry left here would stop a removed
+ // 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));
Review 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.
--
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]
