You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by "jim0987795064 (via GitHub)" <gi...@apache.org> on 2025/08/23 20:57:46 UTC
[PR] KAFKA-19518: Remove the usage of KafkaMetricsGroup(Class klass) [kafka]
jim0987795064 opened a new pull request, #20399:
URL: https://github.com/apache/kafka/pull/20399
the constructor is error-prone when migrating code, since metrics could get unintentionally changed. We should remove the constructor and use constant strings instead to avoid issues like KAFKA-17876, KAFKA-19150, and others.
--
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-19518: Remove the usage of KafkaMetricsGroup(Class klass) [kafka]
Posted by "chia7712 (via GitHub)" <gi...@apache.org>.
chia7712 merged PR #20399:
URL: https://github.com/apache/kafka/pull/20399
--
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-19518: Remove the usage of KafkaMetricsGroup(Class klass) [kafka]
Posted by "jim0987795064 (via GitHub)" <gi...@apache.org>.
jim0987795064 commented on code in PR #20399:
URL: https://github.com/apache/kafka/pull/20399#discussion_r2349953748
##########
core/src/test/scala/unit/kafka/server/AbstractFetcherManagerTest.scala:
##########
@@ -355,4 +356,22 @@ class AbstractFetcherManagerTest {
override protected def endOffsetForEpoch(topicPartition: TopicPartition, epoch: Int): Optional[OffsetAndEpoch] = Optional.of(new OffsetAndEpoch(1, 0))
}
+ @Test
+ def testMetricsClassName(): Unit = {
+ val config = mock(classOf[KafkaConfig])
+ val replicaManager = mock(classOf[ReplicaManager])
+ val quotaManager = mock(classOf[ReplicationQuotaManager])
+ val brokerTopicStats = new BrokerTopicStats()
+ val directoryEventHandler = DirectoryEventHandler.NOOP
+ val metrics = new Metrics()
+ val time = new MockTime()
+ val metadataVersionSupplier = () => MetadataVersion.LATEST_PRODUCTION
+ val brokerEpochSupplier = () => 1L
+
+ val replicaAlterLogDirsManager = new ReplicaAlterLogDirsManager(config, replicaManager, quotaManager, brokerTopicStats, directoryEventHandler)
+ val replicaFetcherManager = new ReplicaFetcherManager(config, replicaManager, metrics, time, quotaManager, metadataVersionSupplier, brokerEpochSupplier)
+
+ assertEquals("ReplicaAlterLogDirsManager", replicaAlterLogDirsManager.getMetricsClassName)
+ assertEquals("ReplicaFetcherManager", replicaFetcherManager.getMetricsClassName)
Review Comment:
@kuoche1712003 Thanks for your suggestion. I’ve used this API to verify the correctness of MetricsClassName.
--
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-19518: Remove the usage of KafkaMetricsGroup(Class klass) [kafka]
Posted by "chia7712 (via GitHub)" <gi...@apache.org>.
chia7712 commented on code in PR #20399:
URL: https://github.com/apache/kafka/pull/20399#discussion_r2361479007
##########
core/src/main/scala/kafka/cluster/Partition.scala:
##########
@@ -116,7 +116,9 @@ class DelayedOperations(topicId: Option[Uuid],
}
object Partition {
- private val metricsGroup = new KafkaMetricsGroup(classOf[Partition])
+ private val metricsPackage = "kafka.cluster"
Review Comment:
Could you please add specific comments, for example: “Changing the package or class name will break compatibility.”
--
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-19518: Remove the usage of KafkaMetricsGroup(Class klass) [kafka]
Posted by "kuoche1712003 (via GitHub)" <gi...@apache.org>.
kuoche1712003 commented on code in PR #20399:
URL: https://github.com/apache/kafka/pull/20399#discussion_r2318536120
##########
core/src/main/scala/kafka/server/AbstractFetcherManager.scala:
##########
@@ -30,7 +30,9 @@ import scala.jdk.OptionConverters._
abstract class AbstractFetcherManager[T <: AbstractFetcherThread](val name: String, clientId: String, numFetchers: Int)
extends Logging {
- private val metricsGroup = new KafkaMetricsGroup(this.getClass)
+ private val metricsPackage = "kafka.server"
+ private val metricsClassName = "AbstractFetcherManager"
+ private val metricsGroup = new KafkaMetricsGroup(metricsPackage, metricsClassName)
Review Comment:
Hardcoding the metricsClassName for AbstractFetcherManager.
I want to make sure this approach won't introduce any unexpected side effects or break existing functionality.
ex. ReplicaAlterLogDirsManager and ReplicaFetcherManager
--
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-19518: Remove the usage of KafkaMetricsGroup(Class klass) [kafka]
Posted by "chia7712 (via GitHub)" <gi...@apache.org>.
chia7712 commented on code in PR #20399:
URL: https://github.com/apache/kafka/pull/20399#discussion_r2415693889
##########
core/src/main/scala/kafka/network/SocketServer.scala:
##########
@@ -834,7 +833,9 @@ private[kafka] class Processor(
threadName: String,
connectionDisconnectListeners: Seq[ConnectionDisconnectListener]
) extends Runnable with Logging {
- private val metricsGroup = new KafkaMetricsGroup(this.getClass)
+ private val metricsPackage = "kafka.server"
Review Comment:
#20666
--
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-19518: Remove the usage of KafkaMetricsGroup(Class klass) [kafka]
Posted by "Yunyung (via GitHub)" <gi...@apache.org>.
Yunyung commented on code in PR #20399:
URL: https://github.com/apache/kafka/pull/20399#discussion_r2318573502
##########
core/src/main/scala/kafka/server/AbstractFetcherManager.scala:
##########
@@ -30,7 +30,9 @@ import scala.jdk.OptionConverters._
abstract class AbstractFetcherManager[T <: AbstractFetcherThread](val name: String, clientId: String, numFetchers: Int)
extends Logging {
- private val metricsGroup = new KafkaMetricsGroup(this.getClass)
+ private val metricsPackage = "kafka.server"
+ private val metricsClassName = "AbstractFetcherManager"
+ private val metricsGroup = new KafkaMetricsGroup(metricsPackage, metricsClassName)
Review Comment:
That’s a good point. It would also be good to add some tests to ensure this.
--
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-19518: Remove the usage of KafkaMetricsGroup(Class klass) [kafka]
Posted by "omkreddy (via GitHub)" <gi...@apache.org>.
omkreddy commented on code in PR #20399:
URL: https://github.com/apache/kafka/pull/20399#discussion_r2415633676
##########
core/src/main/scala/kafka/network/SocketServer.scala:
##########
@@ -834,7 +833,9 @@ private[kafka] class Processor(
threadName: String,
connectionDisconnectListeners: Seq[ConnectionDisconnectListener]
) extends Runnable with Logging {
- private val metricsGroup = new KafkaMetricsGroup(this.getClass)
+ private val metricsPackage = "kafka.server"
Review Comment:
@jim0987795064 This should be "kafka.network" ?
--
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-19518: Remove the usage of KafkaMetricsGroup(Class klass) [kafka]
Posted by "jim0987795064 (via GitHub)" <gi...@apache.org>.
jim0987795064 commented on code in PR #20399:
URL: https://github.com/apache/kafka/pull/20399#discussion_r2379130319
##########
core/src/main/scala/kafka/cluster/Partition.scala:
##########
@@ -116,7 +116,9 @@ class DelayedOperations(topicId: Option[Uuid],
}
object Partition {
- private val metricsGroup = new KafkaMetricsGroup(classOf[Partition])
+ private val metricsPackage = "kafka.cluster"
Review Comment:
@chia7712 Thanks for this suggestion. I've already added a comment to describe this situation.
--
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-19518: Remove the usage of KafkaMetricsGroup(Class klass) [kafka]
Posted by "jim0987795064 (via GitHub)" <gi...@apache.org>.
jim0987795064 commented on code in PR #20399:
URL: https://github.com/apache/kafka/pull/20399#discussion_r2395099316
##########
core/src/main/scala/kafka/network/SocketServer.scala:
##########
@@ -486,7 +488,9 @@ private[kafka] abstract class Acceptor(val socketServer: SocketServer,
apiVersionManager: ApiVersionManager)
extends Runnable with Logging {
- private val metricsGroup = new KafkaMetricsGroup(this.getClass)
+ private val metricsPackage = "kafka.network"
+ private val metricsClassName = "Acceptor"
+ private val metricsGroup = new KafkaMetricsGroup(metricsPackage, metricsClassName)
Review Comment:
Sure. Update it.
--
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-19518: Remove the usage of KafkaMetricsGroup(Class klass) [kafka]
Posted by "kuoche1712003 (via GitHub)" <gi...@apache.org>.
kuoche1712003 commented on code in PR #20399:
URL: https://github.com/apache/kafka/pull/20399#discussion_r2347193890
##########
core/src/main/scala/kafka/server/AbstractFetcherManager.scala:
##########
@@ -226,6 +228,10 @@ abstract class AbstractFetcherManager[T <: AbstractFetcherThread](val name: Stri
fetcherThreadMap.clear()
}
}
+
+ def getMetricsClassName: String = {
Review Comment:
Do we really need to introduce this method just for testing purposes?
##########
core/src/test/scala/unit/kafka/server/AbstractFetcherManagerTest.scala:
##########
@@ -355,4 +356,22 @@ class AbstractFetcherManagerTest {
override protected def endOffsetForEpoch(topicPartition: TopicPartition, epoch: Int): Optional[OffsetAndEpoch] = Optional.of(new OffsetAndEpoch(1, 0))
}
+ @Test
+ def testMetricsClassName(): Unit = {
+ val config = mock(classOf[KafkaConfig])
+ val replicaManager = mock(classOf[ReplicaManager])
+ val quotaManager = mock(classOf[ReplicationQuotaManager])
+ val brokerTopicStats = new BrokerTopicStats()
+ val directoryEventHandler = DirectoryEventHandler.NOOP
+ val metrics = new Metrics()
+ val time = new MockTime()
+ val metadataVersionSupplier = () => MetadataVersion.LATEST_PRODUCTION
+ val brokerEpochSupplier = () => 1L
+
+ val replicaAlterLogDirsManager = new ReplicaAlterLogDirsManager(config, replicaManager, quotaManager, brokerTopicStats, directoryEventHandler)
+ val replicaFetcherManager = new ReplicaFetcherManager(config, replicaManager, metrics, time, quotaManager, metadataVersionSupplier, brokerEpochSupplier)
+
+ assertEquals("ReplicaAlterLogDirsManager", replicaAlterLogDirsManager.getMetricsClassName)
+ assertEquals("ReplicaFetcherManager", replicaFetcherManager.getMetricsClassName)
Review Comment:
Maybe you can use `KafkaYammerMetrics.defaultRegistry()` to get the target metrics.
--
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-19518: Remove the usage of KafkaMetricsGroup(Class klass) [kafka]
Posted by "chia7712 (via GitHub)" <gi...@apache.org>.
chia7712 commented on code in PR #20399:
URL: https://github.com/apache/kafka/pull/20399#discussion_r2386216470
##########
core/src/main/scala/kafka/network/SocketServer.scala:
##########
@@ -486,7 +488,9 @@ private[kafka] abstract class Acceptor(val socketServer: SocketServer,
apiVersionManager: ApiVersionManager)
extends Runnable with Logging {
- private val metricsGroup = new KafkaMetricsGroup(this.getClass)
+ private val metricsPackage = "kafka.network"
+ private val metricsClassName = "Acceptor"
+ private val metricsGroup = new KafkaMetricsGroup(metricsPackage, metricsClassName)
Review Comment:
It seems this `metricsGroup` is useless since there is already a `backwardCompatibilityMetricGroup`
--
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-19518: Remove the usage of KafkaMetricsGroup(Class klass) [kafka]
Posted by "chia7712 (via GitHub)" <gi...@apache.org>.
chia7712 commented on code in PR #20399:
URL: https://github.com/apache/kafka/pull/20399#discussion_r2415662765
##########
core/src/main/scala/kafka/network/SocketServer.scala:
##########
@@ -834,7 +833,9 @@ private[kafka] class Processor(
threadName: String,
connectionDisconnectListeners: Seq[ConnectionDisconnectListener]
) extends Runnable with Logging {
- private val metricsGroup = new KafkaMetricsGroup(this.getClass)
+ private val metricsPackage = "kafka.server"
Review Comment:
Thanks for the heads-up. will fix it.
--
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-19518: Remove the usage of KafkaMetricsGroup(Class klass) [kafka]
Posted by "chia7712 (via GitHub)" <gi...@apache.org>.
chia7712 commented on code in PR #20399:
URL: https://github.com/apache/kafka/pull/20399#discussion_r2415673042
##########
core/src/test/scala/unit/kafka/server/AbstractFetcherManagerTest.scala:
##########
@@ -1,7 +1,7 @@
/*
* Licensed to the Apache Software Foundation (ASF) under one or more
* contributor license agreements. See the NOTICE file distributed with
- * this work for additional information regarding copyright ownership.
+ * this work for additional information registryarding copyright ownership.
Review Comment:
this should be reverted too :smiling_face_with_tear:
--
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