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/06/08 15:18:55 UTC
[GitHub] [kafka] tombentley commented on a diff in pull request #12046: KAFKA-10360: Allow disabling JMX Reporter (KIP-830)
tombentley commented on code in PR #12046:
URL: https://github.com/apache/kafka/pull/12046#discussion_r892518655
##########
clients/src/test/java/org/apache/kafka/clients/admin/KafkaAdminClientTest.java:
##########
@@ -329,6 +331,41 @@ public void testGenerateClientId() {
KafkaAdminClient.generateClientId(newConfMap(AdminClientConfig.CLIENT_ID_CONFIG, "myCustomId")));
}
+ @Test
+ public void testMetricsReporterAutoGeneratedClientId() {
+ Properties props = new Properties();
+ props.setProperty(AdminClientConfig.BOOTSTRAP_SERVERS_CONFIG, "localhost:9999");
+ props.setProperty(AdminClientConfig.METRIC_REPORTER_CLASSES_CONFIG, MockMetricsReporter.class.getName());
+ KafkaAdminClient admin = (KafkaAdminClient) AdminClient.create(props);
+
+ MockMetricsReporter mockMetricsReporter = (MockMetricsReporter) admin.metrics.reporters().get(0);
+
+ assertEquals(admin.getClientId(), mockMetricsReporter.clientId);
+ assertEquals(2, admin.metrics.reporters().size());
+ admin.close();
+ }
+
+ @Test
+ public void testDisableJmxReporter() {
+ Properties props = new Properties();
+ props.setProperty(AdminClientConfig.BOOTSTRAP_SERVERS_CONFIG, "localhost:9999");
+ props.setProperty(AdminClientConfig.AUTO_INCLUDE_JMX_REPORTER_CONFIG, "false");
+ KafkaAdminClient admin = (KafkaAdminClient) AdminClient.create(props);
+ assertTrue(admin.metrics.reporters().isEmpty());
+ admin.close();
+ }
+
+ @Test
+ public void testExplicitlyEnableJmxReporter() {
+ Properties props = new Properties();
+ props.setProperty(AdminClientConfig.BOOTSTRAP_SERVERS_CONFIG, "localhost:9999");
+ props.setProperty(AdminClientConfig.AUTO_INCLUDE_JMX_REPORTER_CONFIG, "false");
Review Comment:
Per previous comment, we should have a test for the case where this is true and the `JmxReporter` is explicitly included.
##########
clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java:
##########
@@ -367,9 +367,11 @@ private void warnIfPartitionerDeprecated() {
List<MetricsReporter> 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:
What about the case where `AUTO_INCLUDE_JMX_REPORTER_CONFIG` is true, but the JMX reporter is explicitly in the `METRIC_REPORTER_CLASSES_CONFIG`? I assume we don't want to include it twice.
--
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