hachikuji commented on code in PR #12634:
URL: https://github.com/apache/kafka/pull/12634#discussion_r981556802
##########
core/src/main/scala/kafka/server/ClientRequestQuotaManager.scala:
##########
@@ -30,6 +30,9 @@ 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
Review Comment:
nit: perhaps overkill to have a constant. I'd suggest
```scala
// Since exemptSensor is for all clients and has a constant name, we do
not expire it
val exemptSensor: Sensor =
getOrCreateSensor(ClientRequestQuotaManager.ExemptSensorName,
Long.MaxValue, exemptMetricName)
```
##########
core/src/main/scala/kafka/server/ClientQuotaManager.scala:
##########
@@ -433,10 +433,10 @@ class ClientQuotaManager(private val config:
ClientQuotaManagerConfig,
.quota(new Quota(quotaLimit, true))
}
- protected def getOrCreateSensor(sensorName: String, metricName: MetricName):
Sensor = {
+ protected def getOrCreateSensor(sensorName: String, expirationTime: Long,
metricName: MetricName): Sensor = {
Review Comment:
nit: `expirationTimeMs`? It's conventional to include the unit type (even
though it's almost always milliseconds).
By the way, this method feels a bit clumsy. It assumes that the provided
metric name describes a "rate" even though there is nothing in the name of the
method that suggests it. It would be clearer to leave the metric initialization
to the caller. For example:
```scala
protected def getOrCreateSensor(
sensorName: String,
expirationTime: Long,
metricRegistrationCallback: Sensor => Unit
): Unit = {
sensorAccessor.getOrCreate(
sensorName,
expirationTime,
metricRegistrationCallback
)
}
```
Then the (only) caller can do the following:
```scala
val exemptSensor: Sensor = getOrCreateSensor(
sensorName = ClientRequestQuotaManager.ExemptSensorName,
expirationTimeMs = Long.MaxValue,
metricRegistrationCallback = sensor => sensor.add(exemptMetricName, new
Rate)
)
```
--
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-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
