divijvaidya commented on code in PR #13924: URL: https://github.com/apache/kafka/pull/13924#discussion_r1247921648
########## 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: ah ok. Can we please unit test this will multi dirs since the metric addition/removal here is dependent on number of directories and we want to ensure it works correctly for multiple dir case as well. -- 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