You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by "brandboat (via GitHub)" <gi...@apache.org> on 2024/07/17 16:45:45 UTC
[PR] KAFKA-17153 KafkaMetricsGroup#newGauge should accept functional interface instead of `com.yammer.metrics.core.Gague` [kafka]
brandboat opened a new pull request, #16618:
URL: https://github.com/apache/kafka/pull/16618
related to https://issues.apache.org/jira/browse/KAFKA-17153,
Java disallow lambdas to implement abstract classes, and hence there are many ugly impl of Gague in code base. Those bad looking code will spring up as we migrate more and more scala code to java.
Hence, we should offer a variety to accept functional interface for java code. For example:
```java
public final <T> Gauge<T> newGauge(String name, Supplier<T> metric) {
return newGauge(name, new Gauge<T>() {
@Override
public T value() {
return metric.get();
}
}, Collections.emptyMap());
}
```
### 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
Re: [PR] KAFKA-17153 KafkaMetricsGroup#newGauge should accept functional interface instead of `com.yammer.metrics.core.Gague` [kafka]
Posted by "chia7712 (via GitHub)" <gi...@apache.org>.
chia7712 commented on code in PR #16618:
URL: https://github.com/apache/kafka/pull/16618#discussion_r1683727988
##########
server-common/src/main/java/org/apache/kafka/server/metrics/KafkaMetricsGroup.java:
##########
@@ -70,16 +71,26 @@ public static MetricName explicitMetricName(String group, String typeName,
return new MetricName(group, typeName, name, scope, nameBuilder.toString());
}
- public final <T> Gauge<T> newGauge(String name, Gauge<T> metric, Map<String, String> tags) {
- return KafkaYammerMetrics.defaultRegistry().newGauge(metricName(name, tags), metric);
+ public <T> Gauge<T> newGauge(String name, Supplier<T> metric, Map<String, String> tags) {
+ return KafkaYammerMetrics.defaultRegistry().newGauge(metricName(name, tags), new Gauge<T>() {
Review Comment:
how about `return newGauge(metricName(name, tags), metric);`?
##########
storage/src/main/java/org/apache/kafka/storage/internals/log/RemoteStorageThreadPool.java:
##########
@@ -51,18 +50,10 @@ public String logPrefix() {
}
}.logger(RemoteStorageThreadPool.class);
- metricsGroup.newGauge(REMOTE_LOG_READER_TASK_QUEUE_SIZE_METRIC.getName(), new Gauge<Integer>() {
- @Override
- public Integer value() {
- return RemoteStorageThreadPool.this.getQueue().size();
- }
- });
- metricsGroup.newGauge(REMOTE_LOG_READER_AVG_IDLE_PERCENT_METRIC.getName(), new Gauge<Double>() {
- @Override
- public Double value() {
- return 1 - (double) RemoteStorageThreadPool.this.getActiveCount() / (double) RemoteStorageThreadPool.this.getCorePoolSize();
- }
- });
+ metricsGroup.newGauge(REMOTE_LOG_READER_TASK_QUEUE_SIZE_METRIC.getName(),
+ () -> RemoteStorageThreadPool.this.getQueue().size());
Review Comment:
how about `getQueue().size()` ?
##########
storage/src/main/java/org/apache/kafka/storage/internals/log/RemoteStorageThreadPool.java:
##########
@@ -51,18 +50,10 @@ public String logPrefix() {
}
}.logger(RemoteStorageThreadPool.class);
- metricsGroup.newGauge(REMOTE_LOG_READER_TASK_QUEUE_SIZE_METRIC.getName(), new Gauge<Integer>() {
- @Override
- public Integer value() {
- return RemoteStorageThreadPool.this.getQueue().size();
- }
- });
- metricsGroup.newGauge(REMOTE_LOG_READER_AVG_IDLE_PERCENT_METRIC.getName(), new Gauge<Double>() {
- @Override
- public Double value() {
- return 1 - (double) RemoteStorageThreadPool.this.getActiveCount() / (double) RemoteStorageThreadPool.this.getCorePoolSize();
- }
- });
+ metricsGroup.newGauge(REMOTE_LOG_READER_TASK_QUEUE_SIZE_METRIC.getName(),
+ () -> RemoteStorageThreadPool.this.getQueue().size());
+ metricsGroup.newGauge(REMOTE_LOG_READER_AVG_IDLE_PERCENT_METRIC.getName(),
+ () -> 1 - (double) RemoteStorageThreadPool.this.getActiveCount() / (double) RemoteStorageThreadPool.this.getCorePoolSize());
Review Comment:
ditto
--
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
Re: [PR] [WIP] KAFKA-17153 KafkaMetricsGroup#newGauge should accept functional interface instead of `com.yammer.metrics.core.Gague` [kafka]
Posted by "chia7712 (via GitHub)" <gi...@apache.org>.
chia7712 commented on PR #16618:
URL: https://github.com/apache/kafka/pull/16618#issuecomment-2236055726
@brandboat those overloaded methods are final, and mockito may call the real method if mockito can't mock them. In order to make those tests pass, we can check the method which is mocked correctly. That is a known limit, so it is fine to change the test if we don't want to remove the "final" declaration.
--
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
Re: [PR] [WIP] KAFKA-17153 KafkaMetricsGroup#newGauge should accept functional interface instead of `com.yammer.metrics.core.Gague` [kafka]
Posted by "chia7712 (via GitHub)" <gi...@apache.org>.
chia7712 commented on PR #16618:
URL: https://github.com/apache/kafka/pull/16618#issuecomment-2236083113
> if we don't want to remove the "final" declaration.
or we can remove the `final`. not sure why this method needs those final methods when we try to mock it.
--
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
Re: [PR] [WIP] KAFKA-17153 KafkaMetricsGroup#newGauge should accept functional interface instead of `com.yammer.metrics.core.Gague` [kafka]
Posted by "brandboat (via GitHub)" <gi...@apache.org>.
brandboat commented on PR #16618:
URL: https://github.com/apache/kafka/pull/16618#issuecomment-2236856846
> or we can remove the final. not sure why this method needs those final methods when we try to mock it.
Thanks for the clear explanation, I will remove the `final` in newGauge methods.
--
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
Re: [PR] KAFKA-17153 KafkaMetricsGroup#newGauge should accept functional interface instead of `com.yammer.metrics.core.Gague` [kafka]
Posted by "chia7712 (via GitHub)" <gi...@apache.org>.
chia7712 merged PR #16618:
URL: https://github.com/apache/kafka/pull/16618
--
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
Re: [PR] KAFKA-17153 KafkaMetricsGroup#newGauge should accept functional interface instead of `com.yammer.metrics.core.Gague` [kafka]
Posted by "brandboat (via GitHub)" <gi...@apache.org>.
brandboat commented on code in PR #16618:
URL: https://github.com/apache/kafka/pull/16618#discussion_r1684578678
##########
server-common/src/main/java/org/apache/kafka/server/metrics/KafkaMetricsGroup.java:
##########
@@ -70,16 +71,26 @@ public static MetricName explicitMetricName(String group, String typeName,
return new MetricName(group, typeName, name, scope, nameBuilder.toString());
}
- public final <T> Gauge<T> newGauge(String name, Gauge<T> metric, Map<String, String> tags) {
- return KafkaYammerMetrics.defaultRegistry().newGauge(metricName(name, tags), metric);
+ public <T> Gauge<T> newGauge(String name, Supplier<T> metric, Map<String, String> tags) {
+ return KafkaYammerMetrics.defaultRegistry().newGauge(metricName(name, tags), new Gauge<T>() {
Review Comment:
Oops.
--
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