[GitHub] [kafka] mimaison commented on a diff in pull request #12046: KAFKA-10360: Allow disabling JMX Reporter (KIP-830)

2022-08-11 Thread GitBox 


mimaison commented on code in PR #12046:
URL: https://github.com/apache/kafka/pull/12046#discussion_r943688586


##
server-common/src/main/java/org/apache/kafka/server/metrics/KafkaYammerMetrics.java:
##
@@ -53,16 +72,21 @@ public static MetricsRegistry defaultRegistry() {
 }
 
 private final MetricsRegistry metricsRegistry = new MetricsRegistry();
-private final FilteringJmxReporter jmxReporter = new 
FilteringJmxReporter(metricsRegistry,
-metricName -> true);
+private FilteringJmxReporter jmxReporter;
 
 private KafkaYammerMetrics() {
-jmxReporter.start();

Review Comment:
   Agreed, I've reverted the changes in this file.



-- 
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-unsubscr...@kafka.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [kafka] mimaison commented on a diff in pull request #12046: KAFKA-10360: Allow disabling JMX Reporter (KIP-830)

2022-08-10 Thread GitBox 


mimaison commented on code in PR #12046:
URL: https://github.com/apache/kafka/pull/12046#discussion_r942490893


##
server-common/src/main/java/org/apache/kafka/server/metrics/KafkaYammerMetrics.java:
##
@@ -53,16 +72,21 @@ public static MetricsRegistry defaultRegistry() {
 }
 
 private final MetricsRegistry metricsRegistry = new MetricsRegistry();
-private final FilteringJmxReporter jmxReporter = new 
FilteringJmxReporter(metricsRegistry,
-metricName -> true);
+private FilteringJmxReporter jmxReporter;
 
 private KafkaYammerMetrics() {
-jmxReporter.start();

Review Comment:
   Looking at this further I'm not sure this is the best way to make this work. 
I wonder if we should apply the same treatment to `kafka.metrics.reporters` as 
we're doing to `metric.reporters`. The issue is that `FilteringJmxReporter` 
does not implement `KafkaMetricsReporter` but instead has its own API (based on 
Yammer's `JmxReporter`) so this would require quite a bit of changes.
   
   The other alternative is to not handle these Yammer reporters as the KIP 
only covered `JmxReporter` and `metric.reporters`.



-- 
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-unsubscr...@kafka.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [kafka] mimaison commented on a diff in pull request #12046: KAFKA-10360: Allow disabling JMX Reporter (KIP-830)

2022-08-10 Thread GitBox 


mimaison commented on code in PR #12046:
URL: https://github.com/apache/kafka/pull/12046#discussion_r942316745


##
server-common/src/main/java/org/apache/kafka/server/metrics/KafkaYammerMetrics.java:
##
@@ -53,16 +72,21 @@ public static MetricsRegistry defaultRegistry() {
 }
 
 private final MetricsRegistry metricsRegistry = new MetricsRegistry();
-private final FilteringJmxReporter jmxReporter = new 
FilteringJmxReporter(metricsRegistry,
-metricName -> true);
+private FilteringJmxReporter jmxReporter;
 
 private KafkaYammerMetrics() {
-jmxReporter.start();
-Exit.addShutdownHook("kafka-jmx-shutdown-hook", jmxReporter::shutdown);
 }
 
 @Override
+@SuppressWarnings("deprecation")
 public void configure(Map configs) {
+AbstractConfig config = new AbstractConfig(CONFIG_DEF, configs);
+List reporters = 
config.getList(CommonClientConfigs.METRIC_REPORTER_CLASSES_CONFIG);
+if 
(config.getBoolean(CommonClientConfigs.AUTO_INCLUDE_JMX_REPORTER_CONFIG) || 
reporters.stream().anyMatch(r -> JmxReporter.class.getName().equals(r))) {

Review Comment:
   It's not trivial to consolidate all call sites as they are slightly 
different.
   
   This instance and the one in `DynamicBrokerConfig` use `Map` objects for 
configs. In `DynamicBrokerConfig`, reporters are in the Scala `Buffer` instead 
of Java `List`. And finally depending on the file, the reporters are either 
class names or class instances.



-- 
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-unsubscr...@kafka.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [kafka] mimaison commented on a diff in pull request #12046: KAFKA-10360: Allow disabling JMX Reporter (KIP-830)

2022-08-10 Thread GitBox 


mimaison commented on code in PR #12046:
URL: https://github.com/apache/kafka/pull/12046#discussion_r942300712


##
server-common/src/main/java/org/apache/kafka/server/metrics/KafkaYammerMetrics.java:
##
@@ -53,16 +72,21 @@ public static MetricsRegistry defaultRegistry() {
 }
 
 private final MetricsRegistry metricsRegistry = new MetricsRegistry();
-private final FilteringJmxReporter jmxReporter = new 
FilteringJmxReporter(metricsRegistry,
-metricName -> true);
+private FilteringJmxReporter jmxReporter;
 
 private KafkaYammerMetrics() {
-jmxReporter.start();

Review Comment:
   The `configure()` method is called from:
   - In KRaft: 
https://github.com/apache/kafka/blob/trunk/core/src/main/scala/kafka/server/KafkaRaftServer.scala#L60
   - In ZK: 
https://github.com/apache/kafka/blob/trunk/core/src/main/scala/kafka/server/KafkaServer.scala#L241



-- 
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-unsubscr...@kafka.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [kafka] mimaison commented on a diff in pull request #12046: KAFKA-10360: Allow disabling JMX Reporter (KIP-830)

2022-08-10 Thread GitBox 


mimaison commented on code in PR #12046:
URL: https://github.com/apache/kafka/pull/12046#discussion_r942291817


##
clients/src/main/java/org/apache/kafka/clients/CommonClientConfigs.java:
##
@@ -98,6 +102,10 @@ public class CommonClientConfigs {
 
 public static final String METRICS_CONTEXT_PREFIX = "metrics.context.";
 
+@Deprecated
+public static final String AUTO_INCLUDE_JMX_REPORTER_CONFIG = 
"auto.include.jmx.reporter";
+public static final String AUTO_INCLUDE_JMX_REPORTER_DOC = "Deprecated. 
Whether to automatically include JmxReporter even if it's not listed in 
metric.reporters. This configuration will be removed in Kafka 4.0, 
users should instead include 
org.apache.kafka.common.metrics.JmxReporter in 
metric.reporters in order to enable the JmxReporter.";

Review Comment:
   Good point, I opened https://issues.apache.org/jira/browse/KAFKA-14158



-- 
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-unsubscr...@kafka.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [kafka] mimaison commented on a diff in pull request #12046: KAFKA-10360: Allow disabling JMX Reporter (KIP-830)

2022-06-09 Thread GitBox 


mimaison commented on code in PR #12046:
URL: https://github.com/apache/kafka/pull/12046#discussion_r893743558


##
clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java:
##
@@ -367,9 +367,11 @@ private void warnIfPartitionerDeprecated() {
 List reporters = 
config.getConfiguredInstances(ProducerConfig.METRIC_REPORTER_CLASSES_CONFIG,
 MetricsReporter.class,
 Collections.singletonMap(ProducerConfig.CLIENT_ID_CONFIG, 
clientId));
-JmxReporter jmxReporter = new JmxReporter();
-
jmxReporter.configure(config.originals(Collections.singletonMap(ProducerConfig.CLIENT_ID_CONFIG,
 clientId)));
-reporters.add(jmxReporter);
+if 
(config.getBoolean(ProducerConfig.AUTO_INCLUDE_JMX_REPORTER_CONFIG)) {

Review Comment:
   Good point! I've added logic to handle this 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.

To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [kafka] mimaison commented on a diff in pull request #12046: KAFKA-10360: Allow disabling JMX Reporter (KIP-830)

2022-04-22 Thread GitBox 


mimaison commented on code in PR #12046:
URL: https://github.com/apache/kafka/pull/12046#discussion_r855922918


##
clients/src/main/java/org/apache/kafka/clients/admin/KafkaAdminClient.java:
##
@@ -331,7 +331,7 @@ public class KafkaAdminClient extends AdminClient {
 /**
  * The name of this AdminClient instance.
  */
-private final String clientId;
+final String clientId;

Review Comment:
   Hi @clolov Thanks for taking a look.
   No there is no good reason to do so. This PR is just a draft for now till 
the KIP gets approved.
   I think instead of making the field package private, we should probably add 
a new package private method `getClientId()` like we have in `KafkaProducer` 
and `KafkaConsumer`.



-- 
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-unsubscr...@kafka.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org