> On May 4, 2015, 3:46 p.m., Jun Rao wrote: > > core/src/main/scala/kafka/server/ClientQuotaMetrics2.scala, lines 153-162 > > <https://reviews.apache.org/r/33049/diff/5/?file=938427#file938427line153> > > > > 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.
Good suggestion. I'll incorporate into the review > On May 4, 2015, 3:46 p.m., Jun Rao wrote: > > core/src/main/scala/kafka/server/ClientQuotaMetrics2.scala, line 115 > > <https://reviews.apache.org/r/33049/diff/5/?file=938427#file938427line115> > > > > 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. I'm using this as a sensor name which must be a unique string (doesn't have any tags). The MetricName does have tags and I'm them while creating the metrics within a sensor. > On May 4, 2015, 3:46 p.m., Jun Rao wrote: > > core/src/main/scala/kafka/server/ClientQuotaMetrics2.scala, lines 82-95 > > <https://reviews.apache.org/r/33049/diff/5/?file=938427#file938427line82> > > > > 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. Interesting thought. So basically, something like this? try { // record metric // trigger callback } catch { case QuotaViolationException => // insert in delayed queue } I'm currently managing the callbacks inside KafkaApis in my current patch. Take a look at that(once I publish it) and let me know which one you prefer. I open to doing it either way. - Aditya ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33049/#review82382 ----------------------------------------------------------- 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 > >
