Copilot commented on code in PR #18046:
URL: https://github.com/apache/pinot/pull/18046#discussion_r3104877144
##########
pinot-common/src/test/java/org/apache/pinot/common/metrics/MetricValueUtils.java:
##########
@@ -18,106 +18,70 @@
*/
package org.apache.pinot.common.metrics;
-import com.yammer.metrics.core.MetricName;
-import org.apache.pinot.plugin.metrics.yammer.YammerMetricName;
-import org.apache.pinot.plugin.metrics.yammer.YammerSettableGauge;
-import org.apache.pinot.spi.metrics.PinotMetric;
-
+/**
+ * Test utility for reading gauge values from {@link AbstractMetrics} without
depending on a specific metrics
+ * implementation.
+ */
public class MetricValueUtils {
private MetricValueUtils() {
}
public static boolean gaugeExists(AbstractMetrics metrics, String
metricName) {
- return extractMetric(metrics, metricName) != null;
+ return metrics.getGaugeValue(metricName) != null;
}
public static long getGaugeValue(AbstractMetrics metrics, String metricName)
{
- PinotMetric pinotMetric = extractMetric(metrics, metricName);
- if (pinotMetric == null) {
- return 0;
- }
- return ((YammerSettableGauge<Long>) pinotMetric.getMetric()).value();
+ Long value = metrics.getGaugeValue(metricName);
+ return value != null ? value : 0L;
Review Comment:
MetricValueUtils now relies solely on AbstractMetrics#getGaugeValue(), which
only reads the deprecated internal _gaugeValues map and returns null for
supplier-based gauges created via AbstractMetrics#setOrUpdateGauge(...,
Supplier). This breaks tests that assert on gauges registered via
setOrUpdateGauge (e.g., mergeRollupTaskNumBucketsToProcess metrics are created
with setOrUpdateGauge in MergeRollupTaskGenerator, but
MetricValueUtils.gaugeExists/getGaugeValue will now return false/0). Consider
restoring support for supplier gauges by falling back to looking up the metric
in metrics.getMetricsRegistry().allMetrics() (using
PinotMetricUtils.makePinotMetricName with the correct prefix) and reading the
gauge value from the underlying metric (e.g., via reflection on
value()/getValue()).
##########
docker/images/pinot/README.md:
##########
@@ -44,11 +44,7 @@ The docker image is tagged as `[Docker Tag]`.
`Kafka Version`: The Kafka Version to build pinot with. Default is `2.0`
-`Java Version`: The Java Build and Runtime image version. Default is `11`
-
-`JDK Version`: The JDK parameter to build pinot, set as part of maven build
option: `-Djdk.version=${JDK_VERSION}`. Default is `11`
-
-`OpenJDK Image`: Base image to use for Pinot build and runtime. Default is
`openjdk`.
+`Java Version`: The Java build and runtime image version. Default is `21`
Review Comment:
The documented CLI for docker-build.sh doesn't match the script
implementation: README lists 5 args including "Pinot Git URL" and "Git Branch",
but docker-build.sh only accepts (tag, git ref, kafka version, java version)
and always builds from https://github.com/apache/pinot.git (Dockerfile clones
it). Also, README says Kafka default is 2.0, while docker-build.sh defaults to
3.0. Please update the usage/argument descriptions to reflect the actual
parameters and defaults, or adjust the script to match the docs.
--
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]
