kafka-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Joel Koshy" <jjko...@gmail.com>
Subject Re: Review Request 33049: Patch for KAFKA-2084
Date Tue, 04 Aug 2015 00:36:09 GMT

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33049/#review93965
-----------------------------------------------------------


Looks good overall. I have a few more minor comments/suggestions. Apart from that the patch
needs a rebase.


core/src/main/scala/kafka/server/ClientQuotaMetrics.scala (line 74)
<https://reviews.apache.org/r/33049/#comment148446>

    Would ClientQuotaManager be a better name?



core/src/main/scala/kafka/server/ClientQuotaMetrics.scala (line 98)
<https://reviews.apache.org/r/33049/#comment148447>

    A number of places in this patch use a : b instead of a: b. Highly stylistic, I'm beginning
to not care about it, but thought we generally prefer a: b



core/src/main/scala/kafka/server/KafkaApis.scala (line 643)
<https://reviews.apache.org/r/33049/#comment148417>

    Rather than string -> quota manager should we just do short (for requestkey) ->
quota manager and avoid additional `nameForKey` lookups further above?



core/src/main/scala/kafka/server/KafkaApis.scala (line 652)
<https://reviews.apache.org/r/33049/#comment148418>

    ```
    quotaManagers.foreach { case(apiKey, quotaManager) =>
      quotaManager.shutdown()
    }
    ```



core/src/main/scala/kafka/server/ThrottledRequest.scala (line 31)
<https://reviews.apache.org/r/33049/#comment148422>

    Would ThrottledResponse be a more accurate name for this?



core/src/test/scala/integration/kafka/api/QuotasTest.scala (line 133)
<https://reviews.apache.org/r/33049/#comment148423>

    Can we look up the sensor directly?
    
    E.g., `...metrics().getSensor(RequestKeys.nameForKey(RequestKeys.FetchKey) + "ThrottleTime-"
+ consumerId1)`



core/src/test/scala/integration/kafka/api/QuotasTest.scala (line 140)
<https://reviews.apache.org/r/33049/#comment148425>

    and with the above this would just be `assertNotNull`



core/src/test/scala/integration/kafka/api/QuotasTest.scala (line 148)
<https://reviews.apache.org/r/33049/#comment148424>

    Uncomment?



core/src/test/scala/integration/kafka/api/QuotasTest.scala (line 166)
<https://reviews.apache.org/r/33049/#comment148426>

    Similar comments as above.


- Joel Koshy


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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message