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 2020/07/25 13:17:46 UTC
[GitHub] [kafka] rgroothuijsen opened a new pull request #9078: KAFKA-10132: Return correct value types for MBean attributes
rgroothuijsen opened a new pull request #9078:
URL: https://github.com/apache/kafka/pull/9078
Currently, JMX outputs all metrics as having type `double`, even if they are strings or other types of numbers. This PR gets the type from the metric's value if possible, with `null` as a fallback.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] rgroothuijsen removed a comment on pull request #9078: KAFKA-10132: Return correct value types for MBean attributes
Posted by GitBox <gi...@apache.org>.
rgroothuijsen removed a comment on pull request #9078:
URL: https://github.com/apache/kafka/pull/9078#issuecomment-663854901
I'm not entirely sure about returning raw nulls as a fallback, however, or how permissive it should be about null values in general. Thoughts?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] rgroothuijsen commented on pull request #9078: KAFKA-10132: Return correct value types for MBean attributes
Posted by GitBox <gi...@apache.org>.
rgroothuijsen commented on pull request #9078:
URL: https://github.com/apache/kafka/pull/9078#issuecomment-663854901
I'm not entirely sure about returning raw nulls as a fallback, however, or how permissive it should be about null values in general. Thoughts?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] rgroothuijsen commented on a change in pull request #9078: KAFKA-10132: Return correct value types for MBean attributes
Posted by GitBox <gi...@apache.org>.
rgroothuijsen commented on a change in pull request #9078:
URL: https://github.com/apache/kafka/pull/9078#discussion_r463845316
##########
File path: clients/src/main/java/org/apache/kafka/common/metrics/JmxReporter.java
##########
@@ -272,8 +272,16 @@ public MBeanInfo getMBeanInfo() {
for (Map.Entry<String, KafkaMetric> entry : this.metrics.entrySet()) {
String attribute = entry.getKey();
KafkaMetric metric = entry.getValue();
+ String metricType = double.class.getName();
+
+ try {
+ metricType = metric.metricValue().getClass().getName();
+ } catch (NullPointerException e) {
Review comment:
@abbccdda That's what I figured as well at first, and it passes in unit tests, but there's a strange side effect. Upon starting `connect-distributed`, a whole bunch of NPEs appear for various metrics. It looks like when I call `metricValue()`, it eventually arrives [here](https://github.com/apache/kafka/blob/trunk/connect/runtime/src/main/java/org/apache/kafka/connect/runtime/distributed/DistributedHerder.java#L1811), and throws an error because `member` is null. I could also do a null check there, though I'm not sure what value the method would return in that case.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] abbccdda commented on a change in pull request #9078: KAFKA-10132: Return correct value types for MBean attributes
Posted by GitBox <gi...@apache.org>.
abbccdda commented on a change in pull request #9078:
URL: https://github.com/apache/kafka/pull/9078#discussion_r463708722
##########
File path: clients/src/main/java/org/apache/kafka/common/metrics/JmxReporter.java
##########
@@ -272,8 +272,16 @@ public MBeanInfo getMBeanInfo() {
for (Map.Entry<String, KafkaMetric> entry : this.metrics.entrySet()) {
String attribute = entry.getKey();
KafkaMetric metric = entry.getValue();
+ String metricType = double.class.getName();
+
+ try {
+ metricType = metric.metricValue().getClass().getName();
+ } catch (NullPointerException e) {
Review comment:
It is weird to catch NPE, could we just check whether `metricValue` is defined?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] jeqo commented on a change in pull request #9078: KAFKA-10132: Return correct value types for MBean attributes
Posted by GitBox <gi...@apache.org>.
jeqo commented on a change in pull request #9078:
URL: https://github.com/apache/kafka/pull/9078#discussion_r519741816
##########
File path: clients/src/main/java/org/apache/kafka/common/metrics/JmxReporter.java
##########
@@ -272,8 +272,16 @@ public MBeanInfo getMBeanInfo() {
for (Map.Entry<String, KafkaMetric> entry : this.metrics.entrySet()) {
String attribute = entry.getKey();
KafkaMetric metric = entry.getValue();
+ String metricType = double.class.getName();
+
+ try {
+ metricType = metric.metricValue().getClass().getName();
+ } catch (NullPointerException e) {
Review comment:
@rgroothuijsen this could be a bug on the DistributedHerder side. Seems that `DistributedHerder#herderMetrics` is initialized [too early](https://github.com/apache/kafka/blob/8e211eb72f9a45897cc37fed394a38096aa47feb/connect/runtime/src/main/java/org/apache/kafka/connect/runtime/distributed/DistributedHerder.java#L216). Could you give a try moving it later in the constructor, [maybe after config](https://github.com/apache/kafka/blob/8e211eb72f9a45897cc37fed394a38096aa47feb/connect/runtime/src/main/java/org/apache/kafka/connect/runtime/distributed/DistributedHerder.java#L249), to check if the same exception happens again?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] rgroothuijsen commented on a change in pull request #9078: KAFKA-10132: Return correct value types for MBean attributes
Posted by GitBox <gi...@apache.org>.
rgroothuijsen commented on a change in pull request #9078:
URL: https://github.com/apache/kafka/pull/9078#discussion_r571647298
##########
File path: clients/src/main/java/org/apache/kafka/common/metrics/JmxReporter.java
##########
@@ -272,8 +272,16 @@ public MBeanInfo getMBeanInfo() {
for (Map.Entry<String, KafkaMetric> entry : this.metrics.entrySet()) {
String attribute = entry.getKey();
KafkaMetric metric = entry.getValue();
+ String metricType = double.class.getName();
+
+ try {
+ metricType = metric.metricValue().getClass().getName();
+ } catch (NullPointerException e) {
Review comment:
@jeqo Thanks, moving the initialization did make a difference, specifically after moving it past [this point](https://github.com/apache/kafka/blob/8e211eb72f9a45897cc37fed394a38096aa47feb/connect/runtime/src/main/java/org/apache/kafka/connect/runtime/distributed/DistributedHerder.java#L232).
This did lead to an opposite problem, though, as now other exceptions start appearing. From what I can tell, `WorkerGroupMember` and `HerderMetrics` are dependent on each other, so I'm getting errors in one or the other when I change the ordering around.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org