----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33049/#review82382 -----------------------------------------------------------
Thanks for the patch. I agree that ClientQuotaMetrics2 is better. It's simpler if all quota-ed metrics are recorded using the same api. core/src/main/scala/kafka/server/ClientQuotaMetrics2.scala <https://reviews.apache.org/r/33049/#comment133125> Perhaps we should handle the delaying logic here too. I was thinking that we can pass in a callback in record(). If the quota is violated, we will register the callback in a delayed queue. core/src/main/scala/kafka/server/ClientQuotaMetrics2.scala <https://reviews.apache.org/r/33049/#comment133116> Instead of putting both the apiKey and clientId in a single string as the name, we probably should make use of tags in MetricName to be consistent with the rest of the usage. core/src/main/scala/kafka/server/ClientQuotaMetrics2.scala <https://reviews.apache.org/r/33049/#comment133118> For measuring the amount of throtting, would it be better to measure it as a percentage of the total time? We can just measure this as a rate and keep recording the amount of delayed time. The rate then tells us the fraction of the total time when throtting happens. This is probably simpler and more intuitive than measuring a separate max and avg. - Jun Rao On April 28, 2015, 12:38 a.m., Aditya Auradkar wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/33049/ > ----------------------------------------------------------- > > (Updated April 28, 2015, 12:38 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 > ------- > > WIP: First patch for quotas. > Changes are > 1. Adding per-client throttle time and quota metrics in > ClientQuotaMetrics.scala > 2. Making changes in QuotaViolationException and Sensor to return delay time > changes. > 3. Added configuration needed so far for quotas in KafkaConfig. > 4. Unit tests This is currently not being used anywhere in the code because I > haven't yet figured out how to enforce delays i.e. purgatory vs delay queue. > I'll have a better idea once I look at the new purgatory implementation. > > This is currently not being used anywhere in the code because I haven't yet > figured out how to enforce delays i.e. purgatory vs delay queue. Hopefully, > this smaller patch is easier to review. > > > Please read: This patch has 2 approaches for managing quotas in > ClientQuotaMetrics and CLientQuotaMetrics2 along with some example usage in > ReplicaManager. This code will have to be cleaned up significantly in order > to commit but I'm looking for feedback on which approach to use. > > Approach 1: ClientQuotaMetrics wraps everything into a single class. Adding > new metrics is much clumsier. > Approach 2: ClientQuotaMetrics2 only maintains per-client metrics for a > single entity (producer, consumer) etc.. This makes the code easier to use. > For throttling on a new dimention i.e. request per second, we only need to > create this object with a new quota and will just work. > > > Diffs > ----- > > clients/src/main/java/org/apache/kafka/common/metrics/MetricConfig.java > dfa1b0a11042ad9d127226f0e0cec8b1d42b8441 > clients/src/main/java/org/apache/kafka/common/metrics/Metrics.java > b3d3d7c56acb445be16a3fbe00f05eaba659be46 > 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 > core/src/main/scala/kafka/server/ClientQuotaMetrics.scala PRE-CREATION > core/src/main/scala/kafka/server/ClientQuotaMetrics2.scala PRE-CREATION > core/src/main/scala/kafka/server/KafkaConfig.scala > 69b772c1941865fbe15b34bb2784c511f8ce519a > core/src/main/scala/kafka/server/KafkaServer.scala > c63f4ba9d622817ea8636d4e6135fba917ce085a > core/src/main/scala/kafka/server/ReplicaManager.scala > 8ddd325015de4245fd2cf500d8b0e8c1fd2bc7e8 > core/src/test/scala/unit/kafka/server/ClientQuotaMetricsTest.scala > PRE-CREATION > core/src/test/scala/unit/kafka/server/ClientQuotaMetricsTest2.scala > PRE-CREATION > > Diff: https://reviews.apache.org/r/33049/diff/ > > > Testing > ------- > > > Thanks, > > Aditya Auradkar > >
