----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33049/#review93991 -----------------------------------------------------------
clients/src/main/java/org/apache/kafka/common/metrics/Quota.java (line 65) <https://reviews.apache.org/r/33049/#comment148477> It's considered a best practice in Java to use ``instanceof`` instead of ``getClass()`` as explained here http://stackoverflow.com/a/596507/2698109 So, you could rewrite lines 65 to 74 as: `` if (!(obj instanceof Quota)) return false; Quota that = (Quota) obj; return (that.bound == this.bound) && (this.upper == this.upper); `` If you decide to keep the ``getClass()`` if-condition as is today, lines 70 to 74 can be simplified as above. clients/src/main/java/org/apache/kafka/common/metrics/QuotaViolationException.java (line 27) <https://reviews.apache.org/r/33049/#comment148478> It's a very good pratice to make any field ``final`` unless necessary otherwise. Make this field final. clients/src/main/java/org/apache/kafka/common/metrics/Sensor.java (line 138) <https://reviews.apache.org/r/33049/#comment148488> If time is, for example, 3.8 this will return 3. Wouldn't be better to round it to 4 using something akin ``(int) Math.round(time)``? core/src/main/scala/kafka/server/ClientQuotaMetrics.scala (line 99) <https://reviews.apache.org/r/33049/#comment148484> separate ``if`` and ``(`` with a space. core/src/main/scala/kafka/server/ClientQuotaMetrics.scala (line 174) <https://reviews.apache.org/r/33049/#comment148482> space between ``if`` and ``(`` core/src/main/scala/kafka/server/ClientQuotaMetrics.scala (line 184) <https://reviews.apache.org/r/33049/#comment148483> space between ``if`` and ``(`` core/src/main/scala/kafka/server/KafkaServer.scala (line 362) <https://reviews.apache.org/r/33049/#comment148489> insert a space between ``if`` and ``(``. core/src/test/scala/unit/kafka/server/ClientQuotaMetricsTest.scala (line 68) <https://reviews.apache.org/r/33049/#comment148487> separate ``for`` and ``(`` with a space. core/src/test/scala/unit/kafka/server/ThrottledRequestExpirationTest.scala (line 58) <https://reviews.apache.org/r/33049/#comment148486> separate ``for`` and ``(`` with a space. core/src/test/scala/unit/kafka/server/ThrottledRequestExpirationTest.scala (line 83) <https://reviews.apache.org/r/33049/#comment148485> separate ``for`` and ``(`` with a space. - Edward Ribeiro On June 30, 2015, 12:54 a.m., Aditya Auradkar wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/33049/ > ----------------------------------------------------------- > > (Updated June 30, 2015, 12:54 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/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 > >
