----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33049/#review94333 -----------------------------------------------------------
Thanks for the patch. A few comments below. clients/src/main/java/org/apache/kafka/common/metrics/Sensor.java (lines 135 - 139) <https://reviews.apache.org/r/33049/#comment148902> Is that calculation here right? Based on the calculation in Throttler, it should be sth like the following. metricValue/quota.bound() - windowSize Also, the window size is calculated as config.sampels() * config.timeWindows(), which is inaccurate. The last window is not complete. So we need to take the current time into consideration. Finally, it seems that delayTime only makes sense for rates and not for anything else. Perhaps we can at least add a comment. core/src/main/scala/kafka/server/ClientQuotaManager.scala (line 131) <https://reviews.apache.org/r/33049/#comment148907> Do we need to log the exception? We know exactly what it is. core/src/main/scala/kafka/server/ClientQuotaManager.scala (lines 208 - 211) <https://reviews.apache.org/r/33049/#comment148909> Instead of creating a new metric instance, we probably should just reuse the same metric instance created in KafkaServer. This will make reporting easier. core/src/main/scala/kafka/server/KafkaApis.scala (line 293) <https://reviews.apache.org/r/33049/#comment148906> We can just do quotaManagers(RequestKeys.ProduceKey) since we expect the entry to always exist in the map. core/src/main/scala/kafka/server/KafkaConfig.scala (line 418) <https://reviews.apache.org/r/33049/#comment148908> What's the use case of the delay factor? - Jun Rao On Aug. 5, 2015, 2:08 a.m., Aditya Auradkar wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/33049/ > ----------------------------------------------------------- > > (Updated Aug. 5, 2015, 2:08 a.m.) > > > Review request for kafka, Joel Koshy and Jun Rao. > > > Bugs: KAFKA-2084 > https://issues.apache.org/jira/browse/KAFKA-2084 > > > Repository: kafka > > > Description > ------- > > Updated patch for quotas. This patch does the following: > 1. Add per-client metrics for both producer and consumers > 2. Add configuration for quotas > 3. Compute delay times in the metrics package and return the delay times in > QuotaViolationException > 4. Add a DelayQueue in KafkaApi's that can be used to throttle any type of > request. Implemented request throttling for produce and fetch requests. > 5. Added unit and integration test cases for both producer and consumer > 6. This doesn't include a system test. There is a separate ticket for that > 7. Fixed KAFKA-2191 - (Included fix from : > https://reviews.apache.org/r/34418/ ) > > Addressing Joel's comments > > > Diffs > ----- > > clients/src/main/java/org/apache/kafka/common/metrics/Quota.java > d82bb0c055e631425bc1ebbc7d387baac76aeeaa > > clients/src/main/java/org/apache/kafka/common/metrics/QuotaViolationException.java > a451e5385c9eca76b38b425e8ac856b2715fcffe > clients/src/main/java/org/apache/kafka/common/metrics/Sensor.java > ca823fd4639523018311b814fde69b6177e73b97 > clients/src/main/java/org/apache/kafka/common/metrics/stats/Rate.java > 98429da34418f7f1deba1b5e44e2e6025212edb3 > clients/src/test/java/org/apache/kafka/common/metrics/MetricsTest.java > 544e120594de78c43581a980b1e4087b4fb98ccb > clients/src/test/java/org/apache/kafka/common/utils/MockTime.java > core/src/main/scala/kafka/server/ClientQuotaManager.scala PRE-CREATION > core/src/main/scala/kafka/server/KafkaApis.scala > 18f5b5b895af1469876b2223841fd90a2dd255e0 > core/src/main/scala/kafka/server/KafkaConfig.scala > dbe170f87331f43e2dc30165080d2cb7dfe5fdbf > core/src/main/scala/kafka/server/KafkaServer.scala > 84d4730ac634f9a5bf12a656e422fea03ad72da8 > core/src/main/scala/kafka/server/ReplicaManager.scala > 795220e7f63d163be90738b4c1a39687b44c1395 > core/src/main/scala/kafka/server/ThrottledResponse.scala PRE-CREATION > core/src/main/scala/kafka/utils/ShutdownableThread.scala > fc226c863095b7761290292cd8755cd7ad0f155c > core/src/test/scala/integration/kafka/api/QuotasTest.scala PRE-CREATION > core/src/test/scala/unit/kafka/server/ClientQuotaManagerTest.scala > PRE-CREATION > core/src/test/scala/unit/kafka/server/KafkaConfigConfigDefTest.scala > 04a02e08a54139ee1a298c5354731bae009efef3 > core/src/test/scala/unit/kafka/server/ThrottledResponseExpirationTest.scala > PRE-CREATION > > Diff: https://reviews.apache.org/r/33049/diff/ > > > Testing > ------- > > > Thanks, > > Aditya Auradkar > >
