You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by GitBox <gi...@apache.org> on 2022/05/04 17:43:52 UTC
[GitHub] [kafka] vamossagar12 opened a new pull request, #12121: Kafka 13846: Adding overloaded metricOrElseCreate method
vamossagar12 opened a new pull request, #12121:
URL: https://github.com/apache/kafka/pull/12121
*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)
--
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
[GitHub] [kafka] jolshan commented on pull request #12121: Kafka 13846: Adding overloaded metricOrElseCreate method
Posted by GitBox <gi...@apache.org>.
jolshan commented on PR #12121:
URL: https://github.com/apache/kafka/pull/12121#issuecomment-1120023305
I was also wondering -- is this file considered part of the public apis?
--
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
[GitHub] [kafka] guozhangwang commented on pull request #12121: KAFKA-13846: Adding overloaded metricOrElseCreate method
Posted by GitBox <gi...@apache.org>.
guozhangwang commented on PR #12121:
URL: https://github.com/apache/kafka/pull/12121#issuecomment-1130394986
> 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.
--
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
[GitHub] [kafka] guozhangwang commented on pull request #12121: KAFKA-13846: Adding overloaded metricOrElseCreate method
Posted by GitBox <gi...@apache.org>.
guozhangwang commented on PR #12121:
URL: https://github.com/apache/kafka/pull/12121#issuecomment-1150259953
@vamossagar12 could you close the VOTE thread before we can review and merge?
--
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
[GitHub] [kafka] vamossagar12 commented on a diff in pull request #12121: KAFKA-13846: Adding overloaded metricOrElseCreate method
Posted by GitBox <gi...@apache.org>.
vamossagar12 commented on code in PR #12121:
URL: https://github.com/apache/kafka/pull/12121#discussion_r876158002
##########
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) {
Review Comment:
I removed the other 2. I had added them to be at par with the addMetric method and it's overloads.
--
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
[GitHub] [kafka] guozhangwang commented on pull request #12121: KAFKA-13846: Adding overloaded addMetricIfAbsent method
Posted by GitBox <gi...@apache.org>.
guozhangwang commented on PR #12121:
URL: https://github.com/apache/kafka/pull/12121#issuecomment-1156954295
> @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.
--
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
[GitHub] [kafka] vamossagar12 commented on pull request #12121: KAFKA-13846: Adding overloaded metricOrElseCreate method
Posted by GitBox <gi...@apache.org>.
vamossagar12 commented on PR #12121:
URL: https://github.com/apache/kafka/pull/12121#issuecomment-1150188347
@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
--
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
[GitHub] [kafka] vamossagar12 commented on pull request #12121: KAFKA-13846: Adding overloaded metricOrElseCreate method
Posted by GitBox <gi...@apache.org>.
vamossagar12 commented on PR #12121:
URL: https://github.com/apache/kafka/pull/12121#issuecomment-1126701461
> > 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.
--
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
[GitHub] [kafka] jolshan commented on a diff in pull request #12121: Kafka 13846: Adding overloaded metricOrElseCreate method
Posted by GitBox <gi...@apache.org>.
jolshan commented on code in PR #12121:
URL: https://github.com/apache/kafka/pull/12121#discussion_r867226487
##########
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:
Would it be useful to know if the metric is created vs. already existed?
--
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
[GitHub] [kafka] vamossagar12 commented on pull request #12121: KAFKA-13846: Adding overloaded metricOrElseCreate method
Posted by GitBox <gi...@apache.org>.
vamossagar12 commented on PR #12121:
URL: https://github.com/apache/kafka/pull/12121#issuecomment-1120335768
> 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..
--
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
[GitHub] [kafka] ijuma commented on a diff in pull request #12121: KAFKA-13846: Adding overloaded metricOrElseCreate method
Posted by GitBox <gi...@apache.org>.
ijuma commented on code in PR #12121:
URL: https://github.com/apache/kafka/pull/12121#discussion_r896838472
##########
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:
Or even better, call `putIfAbsent`, if available.
--
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
[GitHub] [kafka] guozhangwang commented on a diff in pull request #12121: KAFKA-13846: Adding overloaded metricOrElseCreate method
Posted by GitBox <gi...@apache.org>.
guozhangwang commented on code in PR #12121:
URL: https://github.com/apache/kafka/pull/12121#discussion_r895970308
##########
clients/src/main/java/org/apache/kafka/common/metrics/Metrics.java:
##########
@@ -509,7 +509,10 @@ public void addMetric(MetricName metricName, MetricConfig config, MetricValuePro
Objects.requireNonNull(metricValueProvider),
config == null ? this.config : config,
time);
- registerMetric(m);
+ KafkaMetric existingMetric = registerMetric(m);
Review Comment:
Could we clarify in the javadoc above that if the metric already exists, an exception would be thrown?
##########
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
Review Comment:
nit: we can just say "the existing metric with the same name"
--
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
[GitHub] [kafka] jolshan commented on a diff in pull request #12121: Kafka 13846: Adding overloaded metricOrElseCreate method
Posted by GitBox <gi...@apache.org>.
jolshan commented on code in PR #12121:
URL: https://github.com/apache/kafka/pull/12121#discussion_r867226334
##########
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);
}
+ /**
Review Comment:
This makes sense, but I'm a bit curious about the motivation behind this change.
--
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
[GitHub] [kafka] vamossagar12 commented on pull request #12121: KAFKA-13846: Adding overloaded metricOrElseCreate method
Posted by GitBox <gi...@apache.org>.
vamossagar12 commented on PR #12121:
URL: https://github.com/apache/kafka/pull/12121#issuecomment-1132048815
> > 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.
--
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
[GitHub] [kafka] vamossagar12 commented on a diff in pull request #12121: KAFKA-13846: Adding overloaded metricOrElseCreate method
Posted by GitBox <gi...@apache.org>.
vamossagar12 commented on code in PR #12121:
URL: https://github.com/apache/kafka/pull/12121#discussion_r879402941
##########
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:
There were a couple of test failures because of this usage. I updated the code to throw an `IllegalArgumentException` for backward compatibility reasons.
--
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
[GitHub] [kafka] dajac commented on pull request #12121: KAFKA-13846: Adding overloaded metricOrElseCreate method
Posted by GitBox <gi...@apache.org>.
dajac commented on PR #12121:
URL: https://github.com/apache/kafka/pull/12121#issuecomment-1133027849
`Metrics` is actually published in our [javadoc](https://javadoc.io/doc/org.apache.kafka/kafka-clients/latest/org/apache/kafka/common/metrics/Metrics.html). This suggests that changing it would require a KIP, no?
--
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
[GitHub] [kafka] vamossagar12 commented on a diff in pull request #12121: KAFKA-13846: Adding overloaded metricOrElseCreate method
Posted by GitBox <gi...@apache.org>.
vamossagar12 commented on code in PR #12121:
URL: https://github.com/apache/kafka/pull/12121#discussion_r879408388
##########
clients/src/main/java/org/apache/kafka/common/metrics/Metrics.java:
##########
@@ -563,10 +586,11 @@ public synchronized void removeReporter(MetricsReporter reporter) {
}
}
- synchronized void registerMetric(KafkaMetric metric) {
+ synchronized KafkaMetric registerMetric(KafkaMetric metric) {
Review Comment:
done
--
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
[GitHub] [kafka] vamossagar12 commented on a diff in pull request #12121: KAFKA-13846: Adding overloaded metricOrElseCreate method
Posted by GitBox <gi...@apache.org>.
vamossagar12 commented on code in PR #12121:
URL: https://github.com/apache/kafka/pull/12121#discussion_r872971829
##########
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:
@jolshan I added a log line for the case where there's no exception to be thrown.
--
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
[GitHub] [kafka] vamossagar12 commented on pull request #12121: KAFKA-13846: Adding overloaded metricOrElseCreate method
Posted by GitBox <gi...@apache.org>.
vamossagar12 commented on PR #12121:
URL: https://github.com/apache/kafka/pull/12121#issuecomment-1130289233
> 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.
--
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
[GitHub] [kafka] ijuma commented on pull request #12121: KAFKA-13846: Adding overloaded metricOrElseCreate method
Posted by GitBox <gi...@apache.org>.
ijuma commented on PR #12121:
URL: https://github.com/apache/kafka/pull/12121#issuecomment-1155202092
The PR title and commit title seem wrong since the method we introduced is not `metricOrElseCreate`...
--
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
[GitHub] [kafka] vamossagar12 commented on pull request #12121: KAFKA-13846: Adding overloaded metricOrElseCreate method
Posted by GitBox <gi...@apache.org>.
vamossagar12 commented on PR #12121:
URL: https://github.com/apache/kafka/pull/12121#issuecomment-1134624584
> > 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](https://javadoc.io/doc/org.apache.kafka/kafka-clients/latest/org/apache/kafka/common/metrics/Metrics.html). 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
--
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
[GitHub] [kafka] vamossagar12 commented on a diff in pull request #12121: KAFKA-13846: Adding overloaded metricOrElseCreate method
Posted by GitBox <gi...@apache.org>.
vamossagar12 commented on code in PR #12121:
URL: https://github.com/apache/kafka/pull/12121#discussion_r867424519
##########
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:
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.
--
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
[GitHub] [kafka] vamossagar12 commented on pull request #12121: KAFKA-13846: Adding overloaded metricOrElseCreate method
Posted by GitBox <gi...@apache.org>.
vamossagar12 commented on PR #12121:
URL: https://github.com/apache/kafka/pull/12121#issuecomment-1129115177
@guozhangwang , could you plz review this whenever you get the chance?
--
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
[GitHub] [kafka] guozhangwang merged pull request #12121: KAFKA-13846: Adding overloaded metricOrElseCreate method
Posted by GitBox <gi...@apache.org>.
guozhangwang merged PR #12121:
URL: https://github.com/apache/kafka/pull/12121
--
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
[GitHub] [kafka] dajac commented on a diff in pull request #12121: KAFKA-13846: Adding overloaded metricOrElseCreate method
Posted by GitBox <gi...@apache.org>.
dajac commented on code in PR #12121:
URL: https://github.com/apache/kafka/pull/12121#discussion_r878262992
##########
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:
`registerMetric` is also called from the `Sensor` class so we have to check if that works there as well.
--
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
[GitHub] [kafka] vamossagar12 commented on a diff in pull request #12121: KAFKA-13846: Adding overloaded metricOrElseCreate method
Posted by GitBox <gi...@apache.org>.
vamossagar12 commented on code in PR #12121:
URL: https://github.com/apache/kafka/pull/12121#discussion_r867424245
##########
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);
}
+ /**
Review Comment:
@jolshan , the motivation is listed here =>
https://issues.apache.org/jira/projects/KAFKA/issues/KAFKA-13846?filter=allopenissues&orderby=created+DESC%2C+priority+DESC%2C+updated+DESC
As mentioned in the JIRA, currently the metric create operation relies on an IllegalArguementException in case of multi-threaded metric creation for the same metric. The other way is to syncrhonize the outer create call. This PR aims to make that process cleaner.
--
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
[GitHub] [kafka] dajac commented on pull request #12121: KAFKA-13846: Adding overloaded metricOrElseCreate method
Posted by GitBox <gi...@apache.org>.
dajac commented on PR #12121:
URL: https://github.com/apache/kafka/pull/12121#issuecomment-1133028737
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.
--
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
[GitHub] [kafka] dajac commented on a diff in pull request #12121: KAFKA-13846: Adding overloaded metricOrElseCreate method
Posted by GitBox <gi...@apache.org>.
dajac commented on code in PR #12121:
URL: https://github.com/apache/kafka/pull/12121#discussion_r875156714
##########
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) {
Review Comment:
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.
##########
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:
I am wonder if this change is really necessary. Is it?
##########
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:
Instead of updating ‘registerMetric’, did we consider using synchronized?
--
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
[GitHub] [kafka] guozhangwang commented on a diff in pull request #12121: KAFKA-13846: Adding overloaded metricOrElseCreate method
Posted by 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
[GitHub] [kafka] guozhangwang commented on pull request #12121: KAFKA-13846: Adding overloaded metricOrElseCreate method
Posted by GitBox <gi...@apache.org>.
guozhangwang commented on PR #12121:
URL: https://github.com/apache/kafka/pull/12121#issuecomment-1144026934
> @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 :)
--
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
[GitHub] [kafka] vamossagar12 commented on pull request #12121: KAFKA-13846: Adding overloaded metricOrElseCreate method
Posted by GitBox <gi...@apache.org>.
vamossagar12 commented on PR #12121:
URL: https://github.com/apache/kafka/pull/12121#issuecomment-1144537837
> > @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. :)
--
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
[GitHub] [kafka] vamossagar12 commented on a diff in pull request #12121: KAFKA-13846: Adding overloaded metricOrElseCreate method
Posted by GitBox <gi...@apache.org>.
vamossagar12 commented on code in PR #12121:
URL: https://github.com/apache/kafka/pull/12121#discussion_r876159510
##########
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:
Yeah i did consider it. What I felt was the logic remains exactly the same it's just that earlier `registerMetric` used to throw an IllegalArgumentException while with the new changes we just return. So, instead of duplicating I thought it's better to update the already existing method. Plz let me know if that makes sense.
--
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
[GitHub] [kafka] vamossagar12 commented on a diff in pull request #12121: KAFKA-13846: Adding overloaded metricOrElseCreate method
Posted by GitBox <gi...@apache.org>.
vamossagar12 commented on code in PR #12121:
URL: https://github.com/apache/kafka/pull/12121#discussion_r877394625
##########
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:
@guozhangwang i think yeah that could be done. I have made the changes.
--
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
[GitHub] [kafka] guozhangwang commented on a diff in pull request #12121: KAFKA-13846: Adding overloaded metricOrElseCreate method
Posted by GitBox <gi...@apache.org>.
guozhangwang commented on code in PR #12121:
URL: https://github.com/apache/kafka/pull/12121#discussion_r878742212
##########
clients/src/main/java/org/apache/kafka/common/metrics/Metrics.java:
##########
@@ -524,6 +527,26 @@ public void addMetric(MetricName metricName, MetricValueProvider<?> metricValueP
addMetric(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) {
+ KafkaMetric metric = new KafkaMetric(new Object(),
+ Objects.requireNonNull(metricName),
+ Objects.requireNonNull(metricValueProvider),
+ config == null ? this.config : config,
+ time);
+
+ KafkaMetric maybeMetric = registerMetric(metric);
Review Comment:
nit: `maybeMetric` => existingMetric?
##########
clients/src/main/java/org/apache/kafka/common/metrics/Metrics.java:
##########
@@ -563,10 +586,11 @@ public synchronized void removeReporter(MetricsReporter reporter) {
}
}
- synchronized void registerMetric(KafkaMetric metric) {
+ synchronized KafkaMetric registerMetric(KafkaMetric metric) {
Review Comment:
Could we also add a javadoc for this function, on its return value semantics?
--
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
[GitHub] [kafka] vamossagar12 commented on pull request #12121: KAFKA-13846: Adding overloaded metricOrElseCreate method
Posted by GitBox <gi...@apache.org>.
vamossagar12 commented on PR #12121:
URL: https://github.com/apache/kafka/pull/12121#issuecomment-1140297430
@dajac , @guozhangwang The KIP has been approved. Also, as per one of the suggestions on the KIP, metricOrElseCreate has been renamed to getMetricOrElseCreate.
--
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
[GitHub] [kafka] vamossagar12 commented on pull request #12121: KAFKA-13846: Adding overloaded addMetricIfAbsent method
Posted by GitBox <gi...@apache.org>.
vamossagar12 commented on PR #12121:
URL: https://github.com/apache/kafka/pull/12121#issuecomment-1156764298
@ijuma ., i updated the PR name. Also, created a follow up PR to address some of the comments: https://github.com/apache/kafka/pull/12297
@guozhangwang , which file does this correspond to? ` that update the web docs on 3.3 release new API changes`
--
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
[GitHub] [kafka] ijuma commented on a diff in pull request #12121: KAFKA-13846: Adding overloaded metricOrElseCreate method
Posted by 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
[GitHub] [kafka] vamossagar12 commented on pull request #12121: KAFKA-13846: Adding overloaded metricOrElseCreate method
Posted by GitBox <gi...@apache.org>.
vamossagar12 commented on PR #12121:
URL: https://github.com/apache/kafka/pull/12121#issuecomment-1150576919
@guozhangwang done.
--
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