[GitHub] [kafka] dajac commented on a diff in pull request #13623: KAFKA-14926: Remove metrics on Log Cleaner shutdown

["dajac (via GitHub)" <gi...@apache.org>]

dajac commented on code in PR #13623:
URL: https://github.com/apache/kafka/pull/13623#discussion_r1182595446


##########
core/src/test/scala/unit/kafka/log/LogCleanerTest.scala:
##########
@@ -62,6 +65,33 @@ class LogCleanerTest {
     Utils.delete(tmpdir)
   }
 
+  @Test
+  def testRemoveMetricsOnClose(): Unit = {
+    val mockMetricsGroupCtor = mockConstruction(classOf[KafkaMetricsGroup])
+    try {
+      val logCleaner = new LogCleaner(new CleanerConfig(true),
+        logDirs = Array(TestUtils.tempDir()),
+        logs = new Pool[TopicPartition, UnifiedLog](),
+        logDirFailureChannel = new LogDirFailureChannel(1),
+        time = time)
+
+      // shutdown logCleaner so that metrics are removed
+      logCleaner.shutdown()
+
+      val mockMetricsGroup = mockMetricsGroupCtor.constructed.get(0)
+      val numMetricsRegistered = 5
+      verify(mockMetricsGroup, times(numMetricsRegistered)).newGauge(anyString(), any())
+      verify(mockMetricsGroup, times(numMetricsRegistered)).removeMetric(anyString())

Review Comment:
   nit: Should we actually verify that `removeMetric` was called for the metric name that we expected?



##########
core/src/main/scala/kafka/log/LogCleaner.scala:
##########
@@ -124,29 +124,37 @@ class LogCleaner(initialConfig: CleanerConfig,
   private def maxOverCleanerThreads(f: CleanerThread => Double): Int =
     cleaners.foldLeft(0.0d)((max: Double, thread: CleanerThread) => math.max(max, f(thread))).toInt
 
+  private val maxBufferUtilizationPercentMetricName =  "max-buffer-utilization-percent"
+  private val cleanerRecopyPercentMetricName =  "cleaner-recopy-percent"
+  private val maxCleanTimeMetricName =  "max-clean-time-secs"
+  private val maxCompactionDelayMetricsName = "max-compaction-delay-secs"
+  private val deadThreadCountMetricName = "DeadThreadCount"
+  private val metricNames = Set.apply(maxBufferUtilizationPercentMetricName,

Review Comment:
   Could we move those to the companion object? Moreover, as they are constants, they should start with a capital letter. There is also an extra space after `=` for some of them. You can also use `Set(..)` instead of `Set.apply()`.



-- 
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