LuciferYang commented on code in PR #13438:
URL: https://github.com/apache/gravitino/pull/13438#discussion_r4092964623
##########
core/src/main/java/org/apache/gravitino/metrics/MetricsSystem.java:
##########
@@ -114,6 +129,12 @@ public synchronized void unregister(MetricsSource
metricsSource) {
return;
}
this.metricSources.remove(metricsSource.getMetricsSourceName());
+ MetricRegistryListener listener =
sourceListeners.remove(metricsSource.getMetricsSourceName());
+ if (listener != null) {
+ // Sever the live link so a stale source cannot re-inject lazily created
+ // metrics into the shared registry after being unregistered.
+ metricsSource.getMetricRegistry().removeListener(listener);
+ }
metricRegistry.removeMatching(
MetricFilter.startsWith(metricsSource.getMetricsSourceName() + "."));
Review Comment:
`unregisterSource` is `synchronized` and, after removing the listener,
clears the already-forwarded metrics, so a stale source can no longer inject
through the manager. A metric the source adds at the exact instant
`unregisterSource` runs is an inherent edge that the old `register(name,
sourceRegistry)` path had too (its internal listener was async and, worse,
unremovable). So this is not a new or worsened race; the point of the fix is
that unregister can now actually detach the source, which the old wiring could
not do at all.
##########
core/src/main/java/org/apache/gravitino/metrics/MetricsSystem.java:
##########
@@ -223,4 +244,71 @@ private void registerMetricsToPrometheusRegistry() {
public MetricsServlet getPrometheusServlet() {
return new MetricsServlet(prometheusRegistry);
}
+
+ /**
+ * Forwards metrics added to or removed from a source's registry to the
shared registry under
+ * {@code "{metricsSourceName}.{metricName}"} while the source is
registered. Held by {@link
+ * MetricsSystem} so the link can be severed on unregister.
+ */
+ private class SourceMetricsListener extends MetricRegistryListener.Base {
+ private final String prefix;
+
+ SourceMetricsListener(String metricsSourceName) {
+ this.prefix = metricsSourceName + ".";
+ }
+
+ private String prefixed(String name) {
+ return prefix + name;
+ }
+
+ @Override
+ public void onGaugeAdded(String name, Gauge<?> gauge) {
+ metricRegistry.register(prefixed(name), gauge);
+ }
Review Comment:
Re-registering a source first unregisters any prior source under the same
name (covered by the test), so the manager itself does not collide. A collision
from an unrelated external registration of the same prefixed name into the
shared registry is outside this component's control and predates the change;
forwarding through our listener is no more prone to it than the old
`registerAll` was.
--
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]
