[GitHub] [kafka] divijvaidya commented on a diff in pull request #14133: KAFKA-15189: only init remote topic metrics when enabled

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

divijvaidya commented on code in PR #14133:
URL: https://github.com/apache/kafka/pull/14133#discussion_r1279101884


##########
core/src/main/scala/kafka/server/KafkaRequestHandler.scala:
##########
@@ -227,7 +227,7 @@ class KafkaRequestHandlerPool(val brokerId: Int,
   }
 }
 
-class BrokerTopicMetrics(name: Option[String]) {
+class BrokerTopicMetrics(name: Option[String], systemRemoteStorageEnabled: Boolean) {

Review Comment:
   In it's current form, this is not very extensible for future cases because if we want to optionally add metrics for a new feature, we will have to add new booleans as arguments to determine the usage of that feature. 
   
   Perhaps we can either pass the entire config object and let this `BrokerTopicMetrics` class make a decision based on config on what metrics it want to enable.



##########
core/src/test/scala/kafka/server/KafkaRequestHandlerTest.scala:
##########
@@ -77,4 +79,28 @@ class KafkaRequestHandlerTest {
     assertEquals(Some(startTime + 2000000), request.callbackRequestDequeueTimeNanos)
     assertEquals(Some(startTime + 3000000), request.callbackRequestCompleteTimeNanos)
   }
+
+
+  @ParameterizedTest
+  @ValueSource(booleans = Array(true, false))
+  def testTopicStats(systemRemoteStorageEnabled: Boolean): Unit = {

Review Comment:
   I would feel more confident if we have a test, that spins up the servers without Remote Storage enabled and verifies that no Remote Storage related metrics are registered. To do this, we will have to collect all Remote Storage metrics in a class called "RemoteStorageMetrics" similar to QuorumControllerMetrics and use that class as source of truth for all metrics names related to  RemoteStorage. In the test, we can validate that none of the metrics related to RemoteStorage is registered in the default registry.



##########
core/src/main/scala/kafka/server/KafkaRequestHandler.scala:
##########
@@ -439,12 +441,14 @@ class BrokerTopicStats extends Logging {
       topicMetrics.closeMetric(BrokerTopicStats.ProduceMessageConversionsPerSec)
       topicMetrics.closeMetric(BrokerTopicStats.ReplicationBytesOutPerSec)
       topicMetrics.closeMetric(BrokerTopicStats.ReassignmentBytesOutPerSec)
-      topicMetrics.closeMetric(BrokerTopicStats.RemoteBytesOutPerSec)
-      topicMetrics.closeMetric(BrokerTopicStats.RemoteBytesInPerSec)
-      topicMetrics.closeMetric(BrokerTopicStats.RemoteReadRequestsPerSec)
-      topicMetrics.closeMetric(BrokerTopicStats.RemoteWriteRequestsPerSec)
-      topicMetrics.closeMetric(BrokerTopicStats.FailedRemoteReadRequestsPerSec)
-      topicMetrics.closeMetric(BrokerTopicStats.FailedRemoteWriteRequestsPerSec)
+      if (systemRemoteStorageEnabled) {

Review Comment:
   closeMetric is a no-op if metric is missing, hence we don't need to do a check here.



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