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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
