----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33049/#review80130 -----------------------------------------------------------
clients/src/main/java/org/apache/kafka/common/metrics/MetricConfig.java <https://reviews.apache.org/r/33049/#comment129902> quotaEnforcementBlackoutMs may be a clearer name. core/src/main/scala/kafka/server/ClientQuotaMetrics.scala <https://reviews.apache.org/r/33049/#comment129908> Is this necessary? core/src/main/scala/kafka/server/ClientQuotaMetrics.scala <https://reviews.apache.org/r/33049/#comment130453> It would help readability a bit if you can indent the RHS for each argument. core/src/main/scala/kafka/server/ClientQuotaMetrics.scala <https://reviews.apache.org/r/33049/#comment130576> Overall, I think this should work, but it seems slightly high touch no? i.e., do we really need to wrap everything? i.e., you definitely need a clientQuotaMetricsConfig, but it seems many of the wrapper routines here can be folded into the core metrics package. E.g., otherwise for every metric that we want to quota on, we are forced to add new record* methods; If I'm reading this right, a motivation for having the wrappers is to getOrCreate the sensors. Can we just pre-emptively (at the beginning) create the per-client sensors and then avoid the wrapper routines? This would also help avoid the need for the extra quota map and the synchronization logic in creating the sensors. core/src/main/scala/kafka/server/ClientQuotaMetrics.scala <https://reviews.apache.org/r/33049/#comment129915> It is somewhat weird for the record* methods to return the delay time - ideally we should just propagate the quota violation. core/src/main/scala/kafka/server/ClientQuotaMetrics.scala <https://reviews.apache.org/r/33049/#comment130584> Can we make this private to kafka.server? You should be able to access it from the unit tests if you need core/src/main/scala/kafka/server/ClientQuotaMetrics.scala <https://reviews.apache.org/r/33049/#comment130585> This too core/src/main/scala/kafka/server/ClientQuotaMetrics.scala <https://reviews.apache.org/r/33049/#comment130472> Why do we need to create an additional default metricconfig here? core/src/main/scala/kafka/server/ClientQuotaMetrics.scala <https://reviews.apache.org/r/33049/#comment130473> and here? core/src/main/scala/kafka/server/KafkaConfig.scala <https://reviews.apache.org/r/33049/#comment130589> Should we name this more specifically? E.g., ...QuotaBytesPerSecondOverrides ? i.e., in future if we want to quota on other metrics such as messages/sec or requests/sec core/src/main/scala/kafka/server/KafkaServer.scala <https://reviews.apache.org/r/33049/#comment130590> We should probably expose various metric configs - reporters, window size, etc. in the server config. - Joel Koshy On April 15, 2015, 5:36 p.m., Aditya Auradkar wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/33049/ > ----------------------------------------------------------- > > (Updated April 15, 2015, 5:36 p.m.) > > > Review request for kafka and Joel Koshy. > > > 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. Hopefully, this > smaller patch is easier to review. > > Added more testcases > > > Some locking changes for reading/creating the sensors > > > 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/KafkaConfig.scala > 69b772c1941865fbe15b34bb2784c511f8ce519a > core/src/main/scala/kafka/server/KafkaServer.scala > c63f4ba9d622817ea8636d4e6135fba917ce085a > core/src/test/scala/unit/kafka/server/ClientQuotaMetricsTest.scala > PRE-CREATION > > Diff: https://reviews.apache.org/r/33049/diff/ > > > Testing > ------- > > > Thanks, > > Aditya Auradkar > >
