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

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

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


##########
clients/src/main/java/org/apache/kafka/common/metrics/Metrics.java:
##########
@@ -563,10 +586,18 @@ public synchronized void removeReporter(MetricsReporter reporter) {
         }
     }
 
-    synchronized void registerMetric(KafkaMetric metric) {
+    /**
+     * Register a metric if not present or return an already existing metric otherwise.
+     * When a metric is newly registered, this method returns null
+     *
+     * @param metric The KafkaMetric to register
+     * @return KafkaMetric if the metric already exists, null otherwise
+     */
+    synchronized KafkaMetric registerMetric(KafkaMetric metric) {
         MetricName metricName = metric.metricName();
-        if (this.metrics.containsKey(metricName))
-            throw new IllegalArgumentException("A metric named '" + metricName + "' already exists, can't register another one.");
+        if (this.metrics.containsKey(metricName)) {

Review Comment:
   It's not efficient to do `contains` and then `get`. We should do the `get` and check if the result is `null`. That halves the cost of the operations.



-- 
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