hudeqi commented on code in PR #13924:
URL: https://github.com/apache/kafka/pull/13924#discussion_r1247914999
##########
core/src/test/scala/unit/kafka/log/LogCleanerTest.scala:
##########
@@ -83,11 +84,19 @@ class LogCleanerTest {
val numMetricsRegistered = LogCleaner.MetricNames.size
verify(mockMetricsGroup,
times(numMetricsRegistered)).newGauge(anyString(), any())
- // verify that each metric is removed
+ // verify that each metric in `LogCleaner` is removed
LogCleaner.MetricNames.foreach(verify(mockMetricsGroup).removeMetric(_))
+ // verify that each metric in `LogCleanerManager` is removed
+ val mockLogCleanerManagerMetricsGroup =
mockMetricsGroupCtor.constructed.get(1)
+ LogCleanerManager.GaugeMetricNameNoTag.foreach(metricName =>
verify(mockLogCleanerManagerMetricsGroup).newGauge(ArgumentMatchers.eq(metricName),
any()))
+
LogCleanerManager.GaugeMetricNameWithTag.keySet().asScala.foreach(metricName =>
verify(mockLogCleanerManagerMetricsGroup).newGauge(ArgumentMatchers.eq(metricName),
any(), any()))
Review Comment:
> GaugeMetricNameWithTag can contain multiple metrics with same name but
different tags. Let's say `UncleanablePartitionsCountMetricName ` is the name
and tag will be directory path. Over here, we are verifying that there is an
exactly 1 call to newGauge() with `UncleanablePartitionsCountMetricName` as the
metricname. But that is not correct because we are actually calling `newGauge`
for this key name numDirs times.
>
> Hence, this test should ideally fail. What am I missing here?
I think the reason is: "logDirs = Array(TestUtils.tempDir())", means that
the `numDirs` size is 1. @divijvaidya
--
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
