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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
