divijvaidya commented on code in PR #13700:
URL: https://github.com/apache/kafka/pull/13700#discussion_r1233939989
##########
core/src/test/scala/unit/kafka/server/ClientQuotaManagerTest.scala:
##########
@@ -418,6 +418,17 @@ class ClientQuotaManagerTest extends
BaseClientQuotaManagerTest {
}
}
+ @Test
+ def
testDelayedQueueSensorShouldShouldExistAfterInstantiationAndBeRemovedAfterShutdown():
Unit = {
Review Comment:
typo
\shouldshould\should
##########
core/src/test/scala/unit/kafka/server/ClientQuotaManagerTest.scala:
##########
@@ -418,6 +418,17 @@ class ClientQuotaManagerTest extends
BaseClientQuotaManagerTest {
}
}
+ @Test
+ def
testDelayedQueueSensorShouldShouldExistAfterInstantiationAndBeRemovedAfterShutdown():
Unit = {
+ val sensorName = Produce.toString + "-delayQueue"
+ val clientQuotaManager = new ClientQuotaManager(config, metrics, Produce,
time, "")
+ var delayedQueueSensor = metrics.getSensor(sensorName)
+ assertNotNull(delayedQueueSensor, "delayed queue sensor should exist")
+ clientQuotaManager.shutdown()
+ delayedQueueSensor = metrics.getSensor(sensorName)
Review Comment:
Another validation we can perform is to ensure that
`assertTrue(metrics.metrics().isEmpty)` at `@AfterEach` of
BaseClientQuotaManagerTest
##########
core/src/main/scala/kafka/server/ClientQuotaManager.scala:
##########
@@ -572,6 +572,7 @@ class ClientQuotaManager(private val config:
ClientQuotaManagerConfig,
}
def shutdown(): Unit = {
+ metrics.removeSensor(delayQueueSensor.name())
Review Comment:
we should clean up the metric at the very end of the shutdown call (inside a
finally). This is because the metric may still get values while the thread
hasn't completed shutdown.
##########
core/src/main/scala/kafka/server/ClientRequestQuotaManager.scala:
##########
@@ -28,13 +28,12 @@ import org.apache.kafka.server.quota.ClientQuotaCallback
import scala.jdk.CollectionConverters._
object ClientRequestQuotaManager {
- val QuotaRequestPercentDefault = Int.MaxValue.toDouble
val NanosToPercentagePerSecond = 100.0 / TimeUnit.SECONDS.toNanos(1)
// Since exemptSensor is for all clients and has a constant name, we do not
expire exemptSensor and only
// create once.
val DefaultInactiveExemptSensorExpirationTimeSeconds = Long.MaxValue
- private val ExemptSensorName = "exempt-" + QuotaType.Request
+ val ExemptSensorName = "exempt-" + QuotaType.Request
Review Comment:
this could be `private[server]`
##########
core/src/test/scala/unit/kafka/server/ClientQuotaManagerTest.scala:
##########
@@ -418,6 +418,17 @@ class ClientQuotaManagerTest extends
BaseClientQuotaManagerTest {
}
}
+ @Test
+ def
testDelayedQueueSensorShouldShouldExistAfterInstantiationAndBeRemovedAfterShutdown():
Unit = {
+ val sensorName = Produce.toString + "-delayQueue"
+ val clientQuotaManager = new ClientQuotaManager(config, metrics, Produce,
time, "")
+ var delayedQueueSensor = metrics.getSensor(sensorName)
+ assertNotNull(delayedQueueSensor, "delayed queue sensor should exist")
+ clientQuotaManager.shutdown()
Review Comment:
please ensure that this gets cleaned up at the end of the test if the above
assertion fails.
(same comment for other 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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
