Copilot commented on code in PR #18046:
URL: https://github.com/apache/pinot/pull/18046#discussion_r3107597375
##########
pinot-common/src/main/java/org/apache/pinot/common/metrics/AbstractMetrics.java:
##########
@@ -502,14 +503,27 @@ public void addValueToGlobalGauge(final G gauge, final
long unitCount) {
}
/**
- * Get the gauge metric value for the provided gauge name
- * @param gaugeName gauge name
- * @return gauge value. If gauge is not present return null.
+ * Get the gauge metric value for the provided gauge name.
+ * Checks the internal value map first (covers legacy setValueOf* APIs),
then falls back to looking up
+ * the metric in the registry (covers supplier-based gauges created via
setOrUpdateGauge).
+ * @param gaugeName gauge name (without the metric prefix)
+ * @return gauge value, or null if the gauge is not present
*/
@Nullable
public Long getGaugeValue(final String gaugeName) {
AtomicLong value = _gaugeValues.get(gaugeName);
- return value != null ? value.get() : null;
+ if (value != null) {
+ return value.get();
+ }
+ PinotMetric metric =
+
_metricsRegistry.allMetrics().get(PinotMetricUtils.makePinotMetricName(_clazz,
_metricPrefix + gaugeName));
+ if (metric instanceof PinotGauge) {
+ Object gaugeValue = ((PinotGauge<?>) metric).value();
+ if (gaugeValue instanceof Number) {
+ return ((Number) gaugeValue).longValue();
+ }
+ }
+ return null;
Review Comment:
`getGaugeValue()` claims to fall back to the registry for supplier-based
gauges, but this implementation will always return null with the current
metrics registries. Both YammerMetricsRegistry and DropwizardMetricsRegistry
wrap gauges in `YammerMetric`/`DropwizardMetric` (implementing only
`PinotMetric`), so `metric instanceof PinotGauge` is never true and the gauge
value is never read.
Consider either (a) enhancing `PinotMetricsRegistry` to expose typed metrics
(e.g., return `PinotGauge` for gauges), or (b) making this fallback
implementation-agnostic by unwrapping `PinotMetric#getMetric()` and reading the
gauge value (e.g., via reflection on `value()`/`getValue()`) when `metric` is
not a `PinotGauge`.
##########
pom.xml:
##########
@@ -2391,6 +2400,10 @@
<phase>validate</phase>
<configuration>
<rules>
+ <requireJavaVersion>
+ <version>[21,)</version>
+ <message>Apache Pinot requires JDK 21 or newer to
build.</message>
+ </requireJavaVersion>
Review Comment:
The new `<requireJavaVersion>` rule is added under `<pluginManagement>`, but
`maven-enforcer-plugin` is not declared in the root `<build><plugins>` section.
As a result, the rule won’t run for most modules unless they explicitly declare
the enforcer plugin (currently only a small subset do).
If the intent is to enforce JDK 21+ for the whole build, add
`maven-enforcer-plugin` to the root `<build><plugins>` (or ensure every module
declares it) so the execution actually binds to the `validate` phase.
--
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]
