divijvaidya commented on code in PR #13929:
URL: https://github.com/apache/kafka/pull/13929#discussion_r1251715961
##########
core/src/main/scala/kafka/server/ReplicaAlterLogDirsManager.scala:
##########
@@ -54,6 +54,7 @@ class ReplicaAlterLogDirsManager(brokerConfig: KafkaConfig,
def shutdown(): Unit = {
info("shutting down")
closeAllFetchers()
Review Comment:
this can throw exceptions such as Interrupted exception. I would recommend
to close the metrics in a finally block
##########
core/src/main/scala/kafka/server/AbstractFetcherManager.scala:
##########
@@ -31,6 +32,9 @@ abstract class AbstractFetcherManager[T <:
AbstractFetcherThread](val name: Stri
extends Logging {
private val metricsGroup = new KafkaMetricsGroup(this.getClass)
+ // Visible for test
+ private[server] var MetricNames: Map[String, java.util.Map[String, String]]
= Map.empty
Review Comment:
the name of the variable or the associated doc does not tell me what is
stored in the value portion of the map. Could we rename it to MetricNamesToTags
and also explain in the doc what it stores?
##########
core/src/main/scala/kafka/server/ReplicaFetcherManager.scala:
##########
@@ -53,6 +53,7 @@ class ReplicaFetcherManager(brokerConfig: KafkaConfig,
def shutdown(): Unit = {
info("shutting down")
closeAllFetchers()
+ removeMetrics()
Review Comment:
we need tests for both these implementation of FetcherManager to ensure that
they are correctly closing the metric. Similar to how you added for
ReplicaFetcherManager
##########
core/src/main/scala/kafka/server/ReplicaAlterLogDirsManager.scala:
##########
@@ -54,6 +54,7 @@ class ReplicaAlterLogDirsManager(brokerConfig: KafkaConfig,
def shutdown(): Unit = {
Review Comment:
we can completely remove this method and implement it in base class, since
both implementations of AbstractFetcherManager do the same thing. What do you
think?
--
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
