KAFKA-13846: Adding overloaded addMetricIfAbsent method by vamossagar12 · Pull Request #12121 · apache/kafka

vamossagar12 · 2022-05-04T17:43:50Z

More detailed description of your change,
if necessary. The PR title and PR message become
the squashed commit message, so use a separate
comment to ping reviewers.

Summary of testing strategy (including rationale)
for the feature or bug fix. Unit and/or integration
tests are expected for any behaviour change and
system tests should be considered for larger changes.

Committer Checklist (excluded from commit message)

Verify design and implementation
Verify test coverage and CI build status
Verify documentation (including upgrade notes)

clients/src/main/java/org/apache/kafka/common/metrics/Metrics.java

jolshan · 2022-05-06T21:28:26Z

clients/src/main/java/org/apache/kafka/common/metrics/Metrics.java

Would it be useful to know if the metric is created vs. already existed?

Yeah currently, metricOrElseCreate hides that fact. I can add a log line here is needed. Also, I guess the addMetric method(s) already can be used to answer the created v/s already existed question.

@jolshan I added a log line for the case where there's no exception to be thrown.

I am wonder if this change is really necessary. Is it?

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?

@guozhangwang i think yeah that could be done. I have made the changes.

registerMetric is also called from the Sensor class so we have to check if that works there as well.

There were a couple of test failures because of this usage. I updated the code to throw an IllegalArgumentException for backward compatibility reasons.

jolshan · 2022-05-06T21:33:17Z

I was also wondering -- is this file (clients/src/main/java/org/apache/kafka/common/metrics/Metrics.java) considered part of the public apis?

vamossagar12 · 2022-05-08T02:19:16Z

I was also wondering -- is this file (clients/src/main/java/org/apache/kafka/common/metrics/Metrics.java) considered part of the public apis?

I am not sure of that.. @guozhangwang can answer that probably..

vamossagar12 · 2022-05-14T12:00:46Z

I was also wondering -- is this file (clients/src/main/java/org/apache/kafka/common/metrics/Metrics.java) considered part of the public apis?

I am not sure of that.. @guozhangwang can answer that probably..

@jolshan my guess is it is. But i am not 100% sure.

vamossagar12 · 2022-05-17T17:10:19Z

@guozhangwang , could you plz review this whenever you get the chance?

dajac

Thanks for the PR. I left a few comments.

From a public API point of view, Metrics is in a gray area. It is not officially part of our public API however we have a few interfaces leaking it. That being said, we should be careful with the changes that we do here.

dajac · 2022-05-17T18:46:10Z

clients/src/main/java/org/apache/kafka/common/metrics/Metrics.java

I wonder if we really need all these overloads. Do we have use cases for all of them? I have the impression that the last one would be sufficient.

I removed the other 2. I had added them to be at par with the addMetric method and it's overloads.

clients/src/main/java/org/apache/kafka/common/metrics/Metrics.java

dajac · 2022-05-17T18:50:42Z

clients/src/main/java/org/apache/kafka/common/metrics/Metrics.java

I am wonder if this change is really necessary. Is it?

vamossagar12 · 2022-05-18T17:27:08Z

Thanks for the PR. I left a few comments.

From a public API point of view, Metrics is in a gray area. It is not officially part of our public API however we have a few interfaces leaking it. That being said, we should be careful with the changes that we do here.

Thank you for the review. Just wanted to know if it's a public API, does it mean we need a KIP for this? And I totally agree with being careful.

guozhangwang

@vamossagar12 Thanks for the PR! I left a couple comments, aiming to make the newly added function to be atomic with just a single operation on the underlying registry.

guozhangwang · 2022-05-18T18:46:53Z

clients/src/main/java/org/apache/kafka/common/metrics/Metrics.java

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

guozhangwang · 2022-05-18T19:00:04Z

From a public API point of view, Metrics is in a gray area. It is not officially part of our public API however we have a few interfaces leaking it. That being said, we should be careful with the changes that we do here.

I agree with @dajac as well, what I was thinking is that we can modify the private registerMetric, and I'm neutral about whether adding a public metricOrElseCreate should require a KIP. If people think this is necessary, we could create one.

vamossagar12 · 2022-05-19T18:24:07Z

From a public API point of view, Metrics is in a gray area. It is not officially part of our public API however we have a few interfaces leaking it. That being said, we should be careful with the changes that we do here.

I agree with @dajac as well, what I was thinking is that we can modify the private registerMetric, and I'm neutral about whether adding a public metricOrElseCreate should require a KIP. If people think this is necessary, we could create one.

Sure thing... Even I am neutral about KIP.

dajac · 2022-05-20T15:17:00Z

Metrics is actually published in our javadoc. This suggests that changing it would require a KIP, no?

dajac · 2022-05-20T15:17:56Z

Do we also have concrete examples of usages of this new API? It would be helpful to update one or two metrics which would benefits from this change. This would be a good illustration.

guozhangwang

Do we also have concrete examples of usages of this new API? It would be helpful to update one or two metrics which would benefits from this change. This would be a good illustration.

Yes, there are quite many places where multiple threads tried to create the same metric and has to follow the pattern I described in the JIRA ticket. If we just search for the usage of the metric(metricName) function, we can found a bunch of them, e.g. in Fetcher, ConnectMetrics, TaskMetricsGroup, etc.

Metrics is actually published in our javadoc. This suggests that changing it would require a KIP, no?

Sounds good. Let's create a tiny KIP to add the function.

guozhangwang · 2022-05-21T17:39:30Z

clients/src/main/java/org/apache/kafka/common/metrics/Metrics.java

nit: maybeMetric => existingMetric?

clients/src/main/java/org/apache/kafka/common/metrics/Metrics.java

vamossagar12 · 2022-05-23T12:39:04Z

Do we also have concrete examples of usages of this new API? It would be helpful to update one or two metrics which would benefits from this change. This would be a good illustration.

Yes, there are quite many places where multiple threads tried to create the same metric and has to follow the pattern I described in the JIRA ticket. If we just search for the usage of the metric(metricName) function, we can found a bunch of them, e.g. in Fetcher, ConnectMetrics, TaskMetricsGroup, etc.

Metrics is actually published in our javadoc. This suggests that changing it would require a KIP, no?

Sounds good. Let's create a tiny KIP to add the function.

Sure. I made following updates in ConnectMetrics:

https://github.com/apache/kafka/blob/c1467aeeed99a11bae3e69c5d7edeee5bcc3e496/connect/runtime/src/main/java/org/apache/kafka/connect/runtime/ConnectMetrics.java#L320-L332

Note that, IlegalArguementExeception is still possible through metricName() method so I didn't remove that from the javadoc.

Also, I created a small KIP for the same: https://cwiki.apache.org/confluence/display/KAFKA/KIP-843%3A+Adding+metricOrElseCreate+method+to+Metrics

vamossagar12 · 2022-05-28T17:00:24Z

@dajac , @guozhangwang The KIP has been approved. Also, as per one of the suggestions on the KIP, metricOrElseCreate has been renamed to getMetricOrElseCreate.

guozhangwang · 2022-06-01T19:05:41Z

@dajac , @guozhangwang The KIP has been approved. Also, as per one of the suggestions on the KIP, metricOrElseCreate has been renamed to getMetricOrElseCreate.

Thanks! Let's wait and see what people like as the final name of the function :)

vamossagar12 · 2022-06-02T07:27:03Z

@dajac , @guozhangwang The KIP has been approved. Also, as per one of the suggestions on the KIP, metricOrElseCreate has been renamed to getMetricOrElseCreate.

Thanks! Let's wait and see what people like as the final name of the function :)

Makes sense. :)

…ception for backward compatibility. Also, replaced addMetric with metricOrElseCreate in ConnectMetrics for illustrative purposes

vamossagar12 · 2022-06-08T17:19:48Z

@guozhangwang/ @dajac I have updated the name of the function now. I sent out a note 2-3 days ago and didn't notice any resistance. Plz review

guozhangwang · 2022-06-08T18:36:47Z

@vamossagar12 could you close the VOTE thread before we can review and merge?

vamossagar12 · 2022-06-09T01:33:39Z

@guozhangwang done.

guozhangwang

LGTM! @vamossagar12 could you file a follow-up PR that update the web docs on 3.3 release new API changes, as well as a couple of nit comments above?

guozhangwang · 2022-06-13T17:32:23Z

clients/src/main/java/org/apache/kafka/common/metrics/Metrics.java

                                        config == null ? this.config : config,
                                        time);
-        registerMetric(m);
+        KafkaMetric existingMetric = registerMetric(m);


Could we clarify in the javadoc above that if the metric already exists, an exception would be thrown?

guozhangwang · 2022-06-13T17:34:19Z

clients/src/main/java/org/apache/kafka/common/metrics/Metrics.java

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


nit: we can just say "the existing metric with the same name"

ijuma · 2022-06-14T13:40:11Z

The PR title and commit title seem wrong since the method we introduced is not metricOrElseCreate...

ijuma · 2022-06-14T13:41:12Z

clients/src/main/java/org/apache/kafka/common/metrics/Metrics.java

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


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.

Or even better, call putIfAbsent, if available.

CONFLUENT: Sync from apache/kafka trunk to confluentinc/kafka master (13 Jun 2022) apache/trunk: (7 commits) KAFKA-13891: reset generation when syncgroup failed with REBALANCE_IN…(apache#12140) KAFKA-10000: Exactly-once source tasks (apache#11780) KAFKA-13436: Omitted BrokerTopicMetrics metrics in the documentation (apache#11473) MINOR: Use Exit.addShutdownHook instead of directly adding hooks to R…(apache#12283) KAFKA-13846: Adding overloaded metricOrElseCreate method (apache#12121) KAFKA-13935 Fix static usages of IBP in KRaft mode (apache#12250) HOTFIX: null check keys of ProducerRecord when computing sizeInBytes (apache#12288) Conflicts: None

vamossagar12 · 2022-06-15T17:51:31Z

@ijuma ., i updated the PR name. Also, created a follow up PR to address some of the comments: #12297
@guozhangwang , which file does this correspond to? that update the web docs on 3.3 release new API changes

guozhangwang · 2022-06-15T21:15:43Z

@guozhangwang , which file does this correspond to? that update the web docs on 3.3 release new API changes

Here: https://github.com/apache/kafka/blob/trunk/docs/upgrade.html. You can find earlier PRs how they update the upgrade guide / API changes in upcoming releases.

jolshan reviewed May 6, 2022

View reviewed changes

clients/src/main/java/org/apache/kafka/common/metrics/Metrics.java Outdated Show resolved Hide resolved

jolshan reviewed May 6, 2022

View reviewed changes

vamossagar12 changed the title ~~Kafka 13846: Adding overloaded metricOrElseCreate method~~ KAFKA-13846: Adding overloaded metricOrElseCreate method May 8, 2022

vamossagar12 force-pushed the KAFKA-13846 branch from 2e840f1 to 38ba167 Compare May 14, 2022 11:58

dajac reviewed May 17, 2022

View reviewed changes

guozhangwang reviewed May 18, 2022

View reviewed changes

guozhangwang reviewed May 21, 2022

View reviewed changes

vamossagar12 force-pushed the KAFKA-13846 branch from 5334ef8 to c1467ae Compare May 23, 2022 12:32

Sagar Rao added 8 commits June 8, 2022 22:36

KAFKA-13846: Adding overloaded metricOrElseCreate method

43b6a31

Adding tests

9a49629

Adding log line if metric already present for cases metricOrElseCreate

d3ea1fc

Removing unwanted overloads for metricOrElseCreate method

233a173

Changing registerMEtric to return null or actual metric

a653023

Updating handling code of registerMetrics to throw IllegalArguementEx…

24387ce

…ception for backward compatibility. Also, replaced addMetric with metricOrElseCreate in ConnectMetrics for illustrative purposes

Renaming metricOrElseCreate to getMetricOrElseCreate

4da9f48

Replacing getOrCreateMetrics function name with addMetricIfAbsent

a58f8dc

vamossagar12 force-pushed the KAFKA-13846 branch from 63bc93d to a58f8dc Compare June 8, 2022 17:15

guozhangwang approved these changes Jun 13, 2022

View reviewed changes

guozhangwang merged commit 5cab11c into apache:trunk Jun 13, 2022

ijuma reviewed Jun 14, 2022

View reviewed changes

vamossagar12 changed the title ~~KAFKA-13846: Adding overloaded metricOrElseCreate method~~ KAFKA-13846: Adding overloaded addMetricIfAbsent method Jun 15, 2022

Conversation

vamossagar12 commented May 4, 2022

Committer Checklist (excluded from commit message)

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jolshan commented May 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vamossagar12 commented May 8, 2022

Uh oh!

vamossagar12 commented May 14, 2022

Uh oh!

vamossagar12 commented May 17, 2022

Uh oh!

dajac left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vamossagar12 commented May 18, 2022

Uh oh!

guozhangwang left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

guozhangwang commented May 18, 2022

Uh oh!

vamossagar12 commented May 19, 2022

Uh oh!

dajac commented May 20, 2022

Uh oh!

dajac commented May 20, 2022

Uh oh!

guozhangwang left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

vamossagar12 commented May 23, 2022

Uh oh!

vamossagar12 commented May 28, 2022

Uh oh!

guozhangwang commented Jun 1, 2022

Uh oh!

vamossagar12 commented Jun 2, 2022

Uh oh!

vamossagar12 commented Jun 8, 2022

Uh oh!

guozhangwang commented Jun 8, 2022

Uh oh!

vamossagar12 commented Jun 9, 2022

Uh oh!

guozhangwang left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

jolshan commented May 6, 2022 •

edited

Loading