see-quick commented on code in PR #20672:
URL: https://github.com/apache/kafka/pull/20672#discussion_r2459966062
##########
core/src/test/scala/unit/kafka/server/DynamicBrokerConfigTest.scala:
##########
@@ -1065,6 +1066,45 @@ class DynamicBrokerConfigTest {
)
assertFalse(ctx.currentDefaultLogConfig.get().originals().containsKey(SocketServerConfigs.ADVERTISED_LISTENERS_CONFIG))
}
+
+ @Test
+ def testClientTelemetryExporter(): Unit = {
+ val brokerId = 0
+ val origProps = TestUtils.createBrokerConfig(brokerId, port = 8181)
+ val config = KafkaConfig(origProps)
+ val metrics = mock(classOf[Metrics])
+ val telemetryPlugin = mock(classOf[ClientTelemetryExporterPlugin])
+
+ config.dynamicConfig.initialize(Some(telemetryPlugin))
+ val m = new DynamicMetricsReporters(brokerId, config, metrics, "clusterId")
+ config.dynamicConfig.addReconfigurable(m)
+
+ def updateReporter(reporterClass: Class[_]): Unit = {
+ val props = new Properties()
+ props.put(MetricConfigs.METRIC_REPORTER_CLASSES_CONFIG,
reporterClass.getName)
+ config.dynamicConfig.updateDefaultConfig(props)
+ }
+
+ // Reporter implementing only ClientTelemetryExporterProvider
+ updateReporter(classOf[TestExporterOnly])
Review Comment:
Hmmm, could you elaborate on this idea? Because
`ClientTelemetryExporterProvider` is an interface and not a specific class. I
don't think it would cover this part of the code:
```java
case telemetry: ClientTelemetry =>
telemetryExporterPlugin.add(telemetry.clientReceiver())
```
When we use
```java
// Reporter implementing only ClientTelemetryReceiver (deprecated)
updateReporter(classOf[ClientTelemetryReceiver])
verify(telemetryPlugin,
Mockito.atMostOnce()).add(ArgumentMatchers.any(classOf[ClientTelemetryReceiver]))
Mockito.reset(telemetryPlugin)
```
instead of `TestReceiverOnly`.
same for others.... for instance, this part
```java
// Reporter implementing both interfaces => only exporter should be used
updateReporter(classOf[TestReceiverAndExporter])
verify(telemetryPlugin,
Mockito.atMostOnce()).add(ArgumentMatchers.any(classOf[ClientTelemetryExporter]))
verify(telemetryPlugin,
Mockito.never()).add(ArgumentMatchers.any(classOf[ClientTelemetryReceiver]))
Mockito.reset(telemetryPlugin)
```
covers also the first case:
```java
// Reporter implementing only ClientTelemetryExporterProvider
updateReporter(classOf[TestExporterOnly])
verify(telemetryPlugin,
Mockito.atMostOnce()).add(ArgumentMatchers.any(classOf[ClientTelemetryExporter]))
Mockito.reset(telemetryPlugin)
```
So we might remove this first part (as it's currently checked/covered with
two paths).
--
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]
