[GitHub] [kafka] guozhangwang commented on a diff in pull request #12121: KAFKA-13846: Adding overloaded metricOrElseCreate method

[GitBox <gi...@apache.org>]

guozhangwang commented on code in PR #12121:
URL: https://github.com/apache/kafka/pull/12121#discussion_r876234679


##########
clients/src/main/java/org/apache/kafka/common/metrics/Metrics.java:
##########
@@ -563,10 +615,15 @@ public synchronized void removeReporter(MetricsReporter reporter) {
         }
     }
 
-    synchronized void registerMetric(KafkaMetric metric) {
+    synchronized void registerMetric(KafkaMetric metric, boolean raiseIfMetricExists) {

Review Comment:
   Hey @vamossagar12 @jolshan @dajac , since `registerMetric` is a package private function I'm thinking about modifying its signature to return the already existed metric (null if it does not exist, not null means no changes to the underlying registry since there's already a metric), hence indicating if the register-metric succeeded or not. And let the caller (`addMetric` or `metricOrElseCreate`) to react accordingly: e.g. the former would still throw exception if it gets a non-null return value, while the latter just returns the metric. WDYT?



##########
clients/src/main/java/org/apache/kafka/common/metrics/Metrics.java:
##########
@@ -524,6 +524,58 @@ public void addMetric(MetricName metricName, MetricValueProvider<?> metricValueP
         addMetric(metricName, null, metricValueProvider);
     }
 
+    /**
+     * Create or get an existing metric to monitor an object that implements Measurable.
+     * This metric won't be associated with any sensor. This is a way to expose existing values as metrics.
+     *
+     * This method is kept for binary compatibility purposes, it has the same behaviour as
+     * {@link #metricOrElseCreate(MetricName, MetricConfig, MetricValueProvider)}.
+     *
+     * @param metricName The name of the metric
+     * @param config The configuration to use when measuring this measurable
+     * @param measurable The measurable that will be measured by this metric
+     * @return Existing KafkaMetric if already registered or else a newly created one
+     */
+    public KafkaMetric metricOrElseCreate(MetricName metricName, MetricConfig config, Measurable measurable) {
+        return metricOrElseCreate(metricName, config, (MetricValueProvider<?>) measurable);
+    }
+
+    /**
+     * Create or get an existing metric to monitor an object that implements MetricValueProvider.
+     * This metric won't be associated with any sensor. This is a way to expose existing values as metrics.
+     * This method takes care of synchronisation while updating/accessing metrics by concurrent threads.
+     *
+     * @param metricName The name of the metric
+     * @param metricValueProvider The metric value provider associated with this metric
+     * @return Existing KafkaMetric if already registered or else a newly created one
+     */
+    public KafkaMetric metricOrElseCreate(MetricName metricName, MetricValueProvider<?> metricValueProvider) {
+        return metricOrElseCreate(metricName, null, metricValueProvider);
+    }
+
+    /**
+     * Create or get an existing metric to monitor an object that implements MetricValueProvider.
+     * This metric won't be associated with any sensor. This is a way to expose existing values as metrics.
+     * This method takes care of synchronisation while updating/accessing metrics by concurrent threads.
+     *
+     * @param metricName The name of the metric
+     * @param metricValueProvider The metric value provider associated with this metric
+     * @return Existing KafkaMetric if already registered or else a newly created one
+     */
+    public KafkaMetric metricOrElseCreate(MetricName metricName, MetricConfig config, MetricValueProvider<?> metricValueProvider) {

Review Comment:
   If we agree on the other comment I had, i.e. letting the `registerMetric` to return the existing metric, then this function can be a single op: we just blindly create the metric, and tries to call `registerMetric`, if it returns non-null value, then just return that one; otherwise return the newly created metric.
   
   In this case we do not need the `first check, then create` two operations and worries about its atomicity.



-- 
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: jira-unsubscribe@kafka.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org