You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by "hudeqi (via GitHub)" <gi...@apache.org> on 2023/06/29 06:47:07 UTC

[GitHub] [kafka] hudeqi opened a new pull request, #13929: KAFKA-15129;[3/N] Remove metrics in AbstractFetcherManager when fetcher manager instance shutdown

hudeqi opened a new pull request, #13929:
URL: https://github.com/apache/kafka/pull/13929

   This pr is used to remove the metrics in `AbstractFetcherManager` when `ReplicaFetcherManager/ReplicaAlterLogDirsManager` instance shutdown.
   This pr has passed the corresponding unit test, and it is part of [KAFKA-15129](https://issues.apache.org/jira/browse/KAFKA-15129).


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


[GitHub] [kafka] hudeqi closed pull request #13929: KAFKA-15129;[3/N] Remove metrics in AbstractFetcherManager when fetcher manager instance shutdown

Posted by "hudeqi (via GitHub)" <gi...@apache.org>.
hudeqi closed pull request #13929: KAFKA-15129;[3/N] Remove metrics in AbstractFetcherManager when fetcher manager instance shutdown
URL: https://github.com/apache/kafka/pull/13929


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


[GitHub] [kafka] hudeqi commented on pull request #13929: KAFKA-15129;[3/N] Remove metrics in AbstractFetcherManager when fetcher manager instance shutdown

Posted by "hudeqi (via GitHub)" <gi...@apache.org>.
hudeqi commented on PR #13929:
URL: https://github.com/apache/kafka/pull/13929#issuecomment-1724720666

   @nizhikov Hi, can you help review the PR of this series? Looks like @divijvaidya  doesn't have time.


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


[GitHub] [kafka] hudeqi commented on pull request #13929: KAFKA-15129;[3/N] Remove metrics in AbstractFetcherManager when fetcher manager instance shutdown

Posted by "hudeqi (via GitHub)" <gi...@apache.org>.
hudeqi commented on PR #13929:
URL: https://github.com/apache/kafka/pull/13929#issuecomment-1614790587

   And please help to review this @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-unsubscribe@kafka.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [kafka] hudeqi commented on pull request #13929: KAFKA-15129;[3/N] Remove metrics in AbstractFetcherManager when fetcher manager instance shutdown

Posted by "hudeqi (via GitHub)" <gi...@apache.org>.
hudeqi commented on PR #13929:
URL: https://github.com/apache/kafka/pull/13929#issuecomment-1637703947

   > I will get to all your PRs on metrics close by end of this week @hudeqi. Right now I am focusing on releasing 3.5.1.
   
   @divijvaidya hi, reminding~


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


[GitHub] [kafka] divijvaidya commented on pull request #13929: KAFKA-15129;[3/N] Remove metrics in AbstractFetcherManager when fetcher manager instance shutdown

Posted by "divijvaidya (via GitHub)" <gi...@apache.org>.
divijvaidya commented on PR #13929:
URL: https://github.com/apache/kafka/pull/13929#issuecomment-1629316568

   I will get to all your PRs on metrics close by end of this week @hudeqi. Right now I am focusing on releasing 3.5.1.


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


[GitHub] [kafka] nizhikov commented on a diff in pull request #13929: KAFKA-15129;[3/N] Remove metrics in AbstractFetcherManager when fetcher manager instance shutdown

Posted by "nizhikov (via GitHub)" <gi...@apache.org>.
nizhikov commented on code in PR #13929:
URL: https://github.com/apache/kafka/pull/13929#discussion_r1335591076


##########
core/src/main/scala/kafka/server/AbstractFetcherManager.scala:
##########
@@ -39,27 +43,37 @@ abstract class AbstractFetcherManager[T <: AbstractFetcherThread](val name: Stri
   val failedPartitions = new FailedPartitions
   this.logIdent = "[" + name + "] "
 
-  private val tags = Map("clientId" -> clientId).asJava
+  newMetrics()
 
-  metricsGroup.newGauge("MaxLag", () => {
-    // current max lag across all fetchers/topics/partitions
-    fetcherThreadMap.values.foldLeft(0L) { (curMaxLagAll, fetcherThread) =>
-      val maxLagThread = fetcherThread.fetcherLagStats.stats.values.foldLeft(0L)((curMaxLagThread, lagMetrics) =>
-        math.max(curMaxLagThread, lagMetrics.lag))
-      math.max(curMaxLagAll, maxLagThread)
-    }
-  }, tags)
+  private[server] def newMetrics(): Unit = {
+    val tags = Map("clientId" -> clientId).asJava
+    metricsGroup.newGauge(MaxLagMetricName, () => {

Review Comment:
   It seems you can use `MetricName` to check metrics registered and removed during test.



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


[GitHub] [kafka] nizhikov commented on a diff in pull request #13929: KAFKA-15129;[3/N] Remove metrics in AbstractFetcherManager when fetcher manager instance shutdown

Posted by "nizhikov (via GitHub)" <gi...@apache.org>.
nizhikov commented on code in PR #13929:
URL: https://github.com/apache/kafka/pull/13929#discussion_r1331247420


##########
core/src/main/scala/kafka/server/AbstractFetcherManager.scala:
##########
@@ -39,27 +43,37 @@ abstract class AbstractFetcherManager[T <: AbstractFetcherThread](val name: Stri
   val failedPartitions = new FailedPartitions
   this.logIdent = "[" + name + "] "
 
-  private val tags = Map("clientId" -> clientId).asJava
+  newMetrics()
 
-  metricsGroup.newGauge("MaxLag", () => {
-    // current max lag across all fetchers/topics/partitions
-    fetcherThreadMap.values.foldLeft(0L) { (curMaxLagAll, fetcherThread) =>
-      val maxLagThread = fetcherThread.fetcherLagStats.stats.values.foldLeft(0L)((curMaxLagThread, lagMetrics) =>
-        math.max(curMaxLagThread, lagMetrics.lag))
-      math.max(curMaxLagAll, maxLagThread)
-    }
-  }, tags)
+  private[server] def newMetrics(): Unit = {
+    val tags = Map("clientId" -> clientId).asJava
+    metricsGroup.newGauge(MaxLagMetricName, () => {

Review Comment:
   We have `metrigGroup#meter` and other methods that takes MetricName as parameter.
   It's look convinient to create one for `Gauge`.



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


[GitHub] [kafka] divijvaidya commented on pull request #13929: KAFKA-15129;[3/N] Remove metrics in AbstractFetcherManager when fetcher manager instance shutdown

Posted by "divijvaidya (via GitHub)" <gi...@apache.org>.
divijvaidya commented on PR #13929:
URL: https://github.com/apache/kafka/pull/13929#issuecomment-1637720320

   I still need to get 3.5.1 out and focus on KIP-405 PRs (targeting 3.6 release) before I get some more time @hudeqi!


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


[GitHub] [kafka] nizhikov commented on pull request #13929: KAFKA-15129;[3/N] Remove metrics in AbstractFetcherManager when fetcher manager instance shutdown

Posted by "nizhikov (via GitHub)" <gi...@apache.org>.
nizhikov commented on PR #13929:
URL: https://github.com/apache/kafka/pull/13929#issuecomment-1726203159

   Hello. I will review this PR in the nearest time.


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


[GitHub] [kafka] hudeqi commented on pull request #13929: KAFKA-15129;[3/N] Remove metrics in AbstractFetcherManager when fetcher manager instance shutdown

Posted by "hudeqi (via GitHub)" <gi...@apache.org>.
hudeqi commented on PR #13929:
URL: https://github.com/apache/kafka/pull/13929#issuecomment-1627979051

   Are there any hidden problems in this PR? @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-unsubscribe@kafka.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [kafka] hudeqi commented on a diff in pull request #13929: KAFKA-15129;[3/N] Remove metrics in AbstractFetcherManager when fetcher manager instance shutdown

Posted by "hudeqi (via GitHub)" <gi...@apache.org>.
hudeqi commented on code in PR #13929:
URL: https://github.com/apache/kafka/pull/13929#discussion_r1252499247


##########
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:
   I have updated, thanks. @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-unsubscribe@kafka.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [kafka] divijvaidya commented on a diff in pull request #13929: KAFKA-15129;[3/N] Remove metrics in AbstractFetcherManager when fetcher manager instance shutdown

Posted by "divijvaidya (via GitHub)" <gi...@apache.org>.
divijvaidya commented on code in PR #13929:
URL: https://github.com/apache/kafka/pull/13929#discussion_r1254348749


##########
core/src/main/scala/kafka/server/AbstractFetcherManager.scala:
##########
@@ -226,6 +240,29 @@ abstract class AbstractFetcherManager[T <: AbstractFetcherThread](val name: Stri
       fetcherThreadMap.clear()
     }
   }
+
+  def shutdown(): Unit = {
+    info("shutting down")
+    try {
+      closeAllFetchers()
+    } finally {
+      removeMetrics()
+    }
+    info("shutdown completed")
+  }
+
+  private[server] def removeMetrics(): Unit = {
+    metricNamesToTags.foreach(metricTags => {

Review Comment:
   perhaps we also want to remove the metric from metricNamesToTags itself, other that map can be an always increasing map. Same comment for other PRs.



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


[GitHub] [kafka] nizhikov commented on a diff in pull request #13929: KAFKA-15129;[3/N] Remove metrics in AbstractFetcherManager when fetcher manager instance shutdown

Posted by "nizhikov (via GitHub)" <gi...@apache.org>.
nizhikov commented on code in PR #13929:
URL: https://github.com/apache/kafka/pull/13929#discussion_r1331235825


##########
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 metricNamesToTags: Map[String, java.util.Map[String, String]] = Map.empty

Review Comment:
   Can we use mutable map like it's done for `fetcherThreadMap`?
   Or even `Set<MetricName>`, see comment below



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


[GitHub] [kafka] hudeqi commented on a diff in pull request #13929: KAFKA-15129;[3/N] Remove metrics in AbstractFetcherManager when fetcher manager instance shutdown

Posted by "hudeqi (via GitHub)" <gi...@apache.org>.
hudeqi commented on code in PR #13929:
URL: https://github.com/apache/kafka/pull/13929#discussion_r1331294105


##########
core/src/main/scala/kafka/server/AbstractFetcherManager.scala:
##########
@@ -39,27 +43,37 @@ abstract class AbstractFetcherManager[T <: AbstractFetcherThread](val name: Stri
   val failedPartitions = new FailedPartitions
   this.logIdent = "[" + name + "] "
 
-  private val tags = Map("clientId" -> clientId).asJava
+  newMetrics()
 
-  metricsGroup.newGauge("MaxLag", () => {
-    // current max lag across all fetchers/topics/partitions
-    fetcherThreadMap.values.foldLeft(0L) { (curMaxLagAll, fetcherThread) =>
-      val maxLagThread = fetcherThread.fetcherLagStats.stats.values.foldLeft(0L)((curMaxLagThread, lagMetrics) =>
-        math.max(curMaxLagThread, lagMetrics.lag))
-      math.max(curMaxLagAll, maxLagThread)
-    }
-  }, tags)
+  private[server] def newMetrics(): Unit = {
+    val tags = Map("clientId" -> clientId).asJava
+    metricsGroup.newGauge(MaxLagMetricName, () => {

Review Comment:
   Hi, @nizhikov , thank you for taking the time to review. The significance of `metricNamesToTags` is that it can be used in the unit test to check whether all the metrics of the corresponding module are removed when closed, to prevent someone from adding new metrics but forgetting remove, with this collection, you only need to add the new metric to this collection.
   Removing the leaking metrics is a long and potentially fixed process, as can be seen from the original [reviews](https://github.com/apache/kafka/pull/13473#issuecomment-1542365763) reached with @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-unsubscribe@kafka.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


Re: [PR] KAFKA-15129;[3/N] Remove metrics in AbstractFetcherManager when fetcher manager instance shutdown [kafka]

Posted by "hudeqi (via GitHub)" <gi...@apache.org>.
hudeqi closed pull request #13929: KAFKA-15129;[3/N] Remove metrics in AbstractFetcherManager when fetcher manager instance shutdown
URL: https://github.com/apache/kafka/pull/13929


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


[GitHub] [kafka] hudeqi commented on a diff in pull request #13929: KAFKA-15129;[3/N] Remove metrics in AbstractFetcherManager when fetcher manager instance shutdown

Posted by "hudeqi (via GitHub)" <gi...@apache.org>.
hudeqi commented on code in PR #13929:
URL: https://github.com/apache/kafka/pull/13929#discussion_r1254581656


##########
core/src/main/scala/kafka/server/AbstractFetcherManager.scala:
##########
@@ -226,6 +240,29 @@ abstract class AbstractFetcherManager[T <: AbstractFetcherThread](val name: Stri
       fetcherThreadMap.clear()
     }
   }
+
+  def shutdown(): Unit = {
+    info("shutting down")
+    try {
+      closeAllFetchers()
+    } finally {
+      removeMetrics()
+    }
+    info("shutdown completed")
+  }
+
+  private[server] def removeMetrics(): Unit = {
+    metricNamesToTags.foreach(metricTags => {

Review Comment:
   updated.



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


[GitHub] [kafka] divijvaidya commented on a diff in pull request #13929: KAFKA-15129;[3/N] Remove metrics in AbstractFetcherManager when fetcher manager instance shutdown

Posted by "divijvaidya (via GitHub)" <gi...@apache.org>.
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-unsubscribe@kafka.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [kafka] hudeqi commented on pull request #13929: KAFKA-15129;[3/N] Remove metrics in AbstractFetcherManager when fetcher manager instance shutdown

Posted by "hudeqi (via GitHub)" <gi...@apache.org>.
hudeqi commented on PR #13929:
URL: https://github.com/apache/kafka/pull/13929#issuecomment-1665378793

   > I still need to get 3.5.1 out and focus on KIP-405 PRs (targeting 3.6 release) before I get some more time @hudeqi!
   
   Hi, do you have time to review this PR now? I want to combine all the currently opened PRs about metric leaks into one PR. I don’t know if this will help or burden your review? @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-unsubscribe@kafka.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [kafka] nizhikov commented on a diff in pull request #13929: KAFKA-15129;[3/N] Remove metrics in AbstractFetcherManager when fetcher manager instance shutdown

Posted by "nizhikov (via GitHub)" <gi...@apache.org>.
nizhikov commented on code in PR #13929:
URL: https://github.com/apache/kafka/pull/13929#discussion_r1331253141


##########
core/src/main/scala/kafka/server/AbstractFetcherManager.scala:
##########
@@ -39,27 +43,37 @@ abstract class AbstractFetcherManager[T <: AbstractFetcherThread](val name: Stri
   val failedPartitions = new FailedPartitions
   this.logIdent = "[" + name + "] "
 
-  private val tags = Map("clientId" -> clientId).asJava
+  newMetrics()
 
-  metricsGroup.newGauge("MaxLag", () => {
-    // current max lag across all fetchers/topics/partitions
-    fetcherThreadMap.values.foldLeft(0L) { (curMaxLagAll, fetcherThread) =>
-      val maxLagThread = fetcherThread.fetcherLagStats.stats.values.foldLeft(0L)((curMaxLagThread, lagMetrics) =>
-        math.max(curMaxLagThread, lagMetrics.lag))
-      math.max(curMaxLagAll, maxLagThread)
-    }
-  }, tags)
+  private[server] def newMetrics(): Unit = {
+    val tags = Map("clientId" -> clientId).asJava
+    metricsGroup.newGauge(MaxLagMetricName, () => {

Review Comment:
   I wonder, do we really need `metricNamesToTags` set. It's just 4 metric so may be it will be more clear to create explicit `MetricName` for each and use them while closing fetcher manager.



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


[GitHub] [kafka] nizhikov commented on pull request #13929: KAFKA-15129;[3/N] Remove metrics in AbstractFetcherManager when fetcher manager instance shutdown

Posted by "nizhikov (via GitHub)" <gi...@apache.org>.
nizhikov commented on PR #13929:
URL: https://github.com/apache/kafka/pull/13929#issuecomment-1727250598

   Hello. @hudeqi , thanks for the PR.
   Left couple of comments.


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


[GitHub] [kafka] nizhikov commented on a diff in pull request #13929: KAFKA-15129;[3/N] Remove metrics in AbstractFetcherManager when fetcher manager instance shutdown

Posted by "nizhikov (via GitHub)" <gi...@apache.org>.
nizhikov commented on code in PR #13929:
URL: https://github.com/apache/kafka/pull/13929#discussion_r1331245323


##########
core/src/main/scala/kafka/server/AbstractFetcherManager.scala:
##########
@@ -39,27 +43,37 @@ abstract class AbstractFetcherManager[T <: AbstractFetcherThread](val name: Stri
   val failedPartitions = new FailedPartitions
   this.logIdent = "[" + name + "] "
 
-  private val tags = Map("clientId" -> clientId).asJava
+  newMetrics()
 
-  metricsGroup.newGauge("MaxLag", () => {
-    // current max lag across all fetchers/topics/partitions
-    fetcherThreadMap.values.foldLeft(0L) { (curMaxLagAll, fetcherThread) =>
-      val maxLagThread = fetcherThread.fetcherLagStats.stats.values.foldLeft(0L)((curMaxLagThread, lagMetrics) =>
-        math.max(curMaxLagThread, lagMetrics.lag))
-      math.max(curMaxLagAll, maxLagThread)
-    }
-  }, tags)
+  private[server] def newMetrics(): Unit = {
+    val tags = Map("clientId" -> clientId).asJava
+    metricsGroup.newGauge(MaxLagMetricName, () => {

Review Comment:
   We can use 
   ```
   Set<MetricName> metrics = ...;
   
   MetricName name = metricGroup.metricName(MaxLagMetricName, tags);
   KafkaYammerMetrics.defaultRegistry().newGauge(name, () => { ... });
   metrics.add(name);
   ```
   
   And store explicit `MetricName` instead of implicit name and tags.
   
   ```
   metrics.forEach(KafkaYammerMetrics.defaultRegistry()::removeMetric);
   `` `



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