----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33049/#review88180 -----------------------------------------------------------
This is looking very good. clients/src/main/java/org/apache/kafka/common/metrics/QuotaViolationException.java (line 38) <https://reviews.apache.org/r/33049/#comment140622> We can remove the setter. clients/src/main/java/org/apache/kafka/common/metrics/Sensor.java (line 120) <https://reviews.apache.org/r/33049/#comment140623> How about making this a bit more concise and past tense (since you would `getMessage` on the exception after the fact): `%s violated quota. Actual: %f, Threshold: %f` clients/src/test/java/org/apache/kafka/common/metrics/MetricsTest.java (line 88) <https://reviews.apache.org/r/33049/#comment140648> Occurences -> Occurrences Also, `(0..%d) = %d` substituted with `count` and `count / elapsedSecs` - similar comment for the asserts below. clients/src/test/java/org/apache/kafka/common/metrics/MetricsTest.java (line 92) <https://reviews.apache.org/r/33049/#comment140649> `long sleepTimeMs = 2000` core/src/main/scala/kafka/server/ClientQuotaMetrics.scala (line 1) <https://reviews.apache.org/r/33049/#comment140650> Why was MockTime moved from test to main? core/src/main/scala/kafka/server/ClientQuotaMetrics.scala (line 140) <https://reviews.apache.org/r/33049/#comment140654> would prefer to see this on a single line (no braces) core/src/main/scala/kafka/server/KafkaConfig.scala (line 842) <https://reviews.apache.org/r/33049/#comment140656> Prefer a: b over a : b core/src/main/scala/kafka/server/ThrottledRequest.scala (line 42) <https://reviews.apache.org/r/33049/#comment140657> `if (` core/src/main/scala/kafka/server/ThrottledRequest.scala (line 43) <https://reviews.apache.org/r/33049/#comment140658> same core/src/test/scala/integration/kafka/api/QuotasTest.scala (line 142) <https://reviews.apache.org/r/33049/#comment140659> This is an important test, but this is a bit non-deterministic no? i.e., the replicas could have been throttled, but caught up soon after that. We would just need to assert (after) this test that the elapsed time is within the expected delay time for an otherwised throttled consumer. - Joel Koshy On June 12, 2015, 5:40 p.m., Aditya Auradkar wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/33049/ > ----------------------------------------------------------- > > (Updated June 12, 2015, 5:40 p.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/ClientQuotaMetrics.scala PRE-CREATION > core/src/main/scala/kafka/server/KafkaApis.scala > d63bc18d795a6f0e6994538ca55a9a46f7fb8ffd > core/src/main/scala/kafka/server/KafkaConfig.scala > 2d75186a110075e0c322db4b9f7a8c964a7a3e88 > core/src/main/scala/kafka/server/KafkaServer.scala > b320ce9f6a12c0ee392e91beb82e8804d167f9f4 > core/src/main/scala/kafka/server/ReplicaManager.scala > 59c9bc3ac3a8afc07a6f8c88c5871304db588d17 > core/src/main/scala/kafka/server/ThrottledRequest.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/ClientQuotaMetricsTest.scala > PRE-CREATION > core/src/test/scala/unit/kafka/server/KafkaConfigConfigDefTest.scala > ace6321b36d809946554d205bc926c9c76a43bd6 > core/src/test/scala/unit/kafka/server/ThrottledRequestExpirationTest.scala > PRE-CREATION > > Diff: https://reviews.apache.org/r/33049/diff/ > > > Testing > ------- > > > Thanks, > > Aditya Auradkar > >
