Jackie-Jiang commented on code in PR #18259:
URL: https://github.com/apache/pinot/pull/18259#discussion_r3113103376
##########
pinot-common/src/main/java/org/apache/pinot/common/metrics/AbstractMetrics.java:
##########
@@ -601,17 +614,20 @@ public void addCallbackGaugeIfNeeded(final String
metricName, final Callable<Lon
*/
@Deprecated
public void addCallbackGauge(final String metricName, final Callable<Long>
valueCallback) {
- PinotMetricUtils
- .makeGauge(_metricsRegistry,
PinotMetricUtils.makePinotMetricName(_clazz, _metricPrefix + metricName),
- PinotMetricUtils.makePinotGauge(avoid -> {
- try {
- return valueCallback.call();
- } catch (Exception e) {
- LOGGER.error("Caught exception", e);
- Utils.rethrowException(e);
- throw new AssertionError("Should not reach this");
- }
- }));
+ synchronized (_gaugeValues) {
Review Comment:
Do we need to synchronize it since the map is already concurrent?
##########
pinot-common/src/main/java/org/apache/pinot/common/metrics/AbstractMetrics.java:
##########
@@ -63,6 +63,10 @@ public abstract class AbstractMetrics<QP extends
AbstractMetrics.QueryPhase, M e
@Deprecated
private final Map<String, AtomicLong> _gaugeValues = new
ConcurrentHashMap<String, AtomicLong>();
+ // Tracks PinotGauge instances registered via
setOrUpdateGauge/addCallbackGauge so getGaugeValue() can read
+ // supplier-based gauges that are not present in _gaugeValues. Keyed by
un-prefixed gauge name.
+ private final Map<String, PinotGauge<Long>> _gauges = new
ConcurrentHashMap<>();
Review Comment:
Why do we need this extra map?
--
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]
