splett2 commented on a change in pull request #9386: URL: https://github.com/apache/kafka/pull/9386#discussion_r523682718
########## File path: core/src/test/scala/unit/kafka/network/ConnectionQuotasTest.scala ########## @@ -633,18 +837,27 @@ class ConnectionQuotasTest { listenerName: ListenerName, address: InetAddress, numConnections: Long, - timeIntervalMs: Long) : Unit = { + timeIntervalMs: Long, + expectIpThrottle: Boolean): Boolean = { var nextSendTime = System.currentTimeMillis + timeIntervalMs + var ipThrottled = false for (_ <- 0L until numConnections) { // this method may block if broker-wide or listener limit on the number of connections is reached - connectionQuotas.inc(listenerName, address, blockedPercentMeters(listenerName.value)) - + try { + connectionQuotas.inc(listenerName, address, blockedPercentMeters(listenerName.value)) + } catch { + case e: ConnectionThrottledException => + if (!expectIpThrottle) + throw e + ipThrottled = true Review comment: I took a closer look at some of the IP throttling tests, and I think they are a little too coarse-grained with respect to testing the quota rate being enforced. I updated the IP throttling rate above/below tests that don't expect to block to instead use `acceptConnectionsAndVerifyRate` with `MockTime`. It's a little messy, because a lot of the other tests for broker/listener quotas have to use `Time.SYSTEM` due to monitor waiting. I think it should also be possible to update the broker/listener quota tests in `ConnectionQuotasTest` and `DynamicConnectionQuotaTest` to use mock time by extending the `Time` interface, but I would prefer to do that in a follow-up PR. ---------------------------------------------------------------- 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org