----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33378/#review86428 -----------------------------------------------------------
Overall, looks good. General comment on the naming: delay vs throttle. Personally, I prefer throttle - I think that is clearer from the client perspective. Should probably add a test to verify that the replica fetchers are never throttled. Or this may be more relevant for your other patch. clients/src/main/java/org/apache/kafka/clients/producer/internals/Sender.java <https://reviews.apache.org/r/33378/#comment138801> How about recordThrottleTime? and replacing quotaDelay with throttle in other places as well? clients/src/main/java/org/apache/kafka/common/protocol/Protocol.java <https://reviews.apache.org/r/33378/#comment138802> Can drop this comment clients/src/main/java/org/apache/kafka/common/protocol/Protocol.java <https://reviews.apache.org/r/33378/#comment138803> May be slightly clearer: "Duration in milliseconds for which the request was throttled due to quota violation. (Zero if the request did not violate any quota.)" clients/src/main/java/org/apache/kafka/common/protocol/Protocol.java <https://reviews.apache.org/r/33378/#comment138804> same clients/src/test/java/org/apache/kafka/clients/producer/internals/SenderTest.java <https://reviews.apache.org/r/33378/#comment138810> requests core/src/main/scala/kafka/api/FetchResponse.scala <https://reviews.apache.org/r/33378/#comment138811> follow-up core/src/main/scala/kafka/api/FetchResponse.scala <https://reviews.apache.org/r/33378/#comment138813> `if (` core/src/main/scala/kafka/api/FetchResponse.scala <https://reviews.apache.org/r/33378/#comment138812> ... from a client that send a v0 FetchRequest core/src/main/scala/kafka/api/FetchResponse.scala <https://reviews.apache.org/r/33378/#comment138814> `if (` core/src/main/scala/kafka/api/ProducerResponse.scala <https://reviews.apache.org/r/33378/#comment138815> We should do this based on the response version as well right? core/src/main/scala/kafka/api/ProducerResponse.scala <https://reviews.apache.org/r/33378/#comment138816> `if (` core/src/main/scala/kafka/server/DelayedFetch.scala <https://reviews.apache.org/r/33378/#comment138818> This is slightly unwieldy. Perhaps we can hold this patch especially since this will be impacted by the main patch (KAFKA-2084) core/src/main/scala/kafka/server/DelayedProduce.scala <https://reviews.apache.org/r/33378/#comment138819> similar comment here core/src/test/scala/unit/kafka/server/DelayedOperationTest.scala <https://reviews.apache.org/r/33378/#comment138823> Why do we need this as part of this patch? - Joel Koshy On May 12, 2015, 9:42 p.m., Aditya Auradkar wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/33378/ > ----------------------------------------------------------- > > (Updated May 12, 2015, 9:42 p.m.) > > > Review request for kafka, Joel Koshy and Jun Rao. > > > Bugs: KAFKA-2136 > https://issues.apache.org/jira/browse/KAFKA-2136 > > > Repository: kafka > > > Description > ------- > > Changes are > - protocol changes to the fetch request and response to return the > throttle_time_ms to clients > - New producer/consumer metrics to expose the avg and max delay time for a > client > - Test cases. > > For now the patch will publish a zero delay and return a response > > > Diffs > ----- > > > clients/src/main/java/org/apache/kafka/clients/consumer/internals/Fetcher.java > ef9dd5238fbc771496029866ece1d85db6d7b7a5 > > clients/src/main/java/org/apache/kafka/clients/producer/internals/Sender.java > b2db91ca14bbd17fef5ce85839679144fff3f689 > clients/src/main/java/org/apache/kafka/common/protocol/Protocol.java > 3dc8b015afd2347a41c9a9dbc02b8e367da5f75f > clients/src/main/java/org/apache/kafka/common/requests/FetchRequest.java > 8686d83aa52e435c6adafbe9ff4bd1602281072a > clients/src/main/java/org/apache/kafka/common/requests/FetchResponse.java > eb8951fba48c335095cc43fc3672de1c733e07ff > clients/src/main/java/org/apache/kafka/common/requests/ProduceRequest.java > fabeae3083a8ea55cdacbb9568f3847ccd85bab4 > clients/src/main/java/org/apache/kafka/common/requests/ProduceResponse.java > 37ec0b79beafcf5735c386b066eb319fb697eff5 > > clients/src/test/java/org/apache/kafka/clients/consumer/internals/FetcherTest.java > 419541011d652becf0cda7a5e62ce813cddb1732 > > clients/src/test/java/org/apache/kafka/clients/producer/internals/SenderTest.java > 8b1805d3d2bcb9fe2bacb37d870c3236aa9532c4 > > clients/src/test/java/org/apache/kafka/common/requests/RequestResponseTest.java > e3cc1967e407b64cc734548c19e30de700b64ba8 > core/src/main/scala/kafka/api/FetchRequest.scala > b038c15186c0cbcc65b59479324052498361b717 > core/src/main/scala/kafka/api/FetchResponse.scala > 75aaf57fb76ec01660d93701a57ae953d877d81c > core/src/main/scala/kafka/api/ProducerRequest.scala > 570b2da1d865086f9830aa919a49063abbbe574d > core/src/main/scala/kafka/api/ProducerResponse.scala > 5d1fac4cb8943f5bfaa487f8e9d9d2856efbd330 > core/src/main/scala/kafka/consumer/SimpleConsumer.scala > 31a2639477bf66f9a05d2b9b07794572d7ec393b > core/src/main/scala/kafka/server/AbstractFetcherThread.scala > 83fc47417dd7fe4edf030217fa7fd69d99b170b0 > core/src/main/scala/kafka/server/DelayedFetch.scala > de6cf5bdaa0e70394162febc63b50b55ca0a92db > core/src/main/scala/kafka/server/DelayedProduce.scala > 05078b24ef28f2f4e099afa943e43f1d00359fda > core/src/main/scala/kafka/server/KafkaApis.scala > 417960dd1ab407ebebad8fdb0e97415db3e91a2f > core/src/main/scala/kafka/server/OffsetManager.scala > 18680ce100f10035175cc0263ba7787ab0f6a17a > core/src/main/scala/kafka/server/ReplicaFetcherThread.scala > b31b432a226ba79546dd22ef1d2acbb439c2e9a3 > core/src/main/scala/kafka/server/ReplicaManager.scala > 59c9bc3ac3a8afc07a6f8c88c5871304db588d17 > core/src/test/scala/unit/kafka/api/RequestResponseSerializationTest.scala > 5717165f2344823fabe8f7cfafae4bb8af2d949a > core/src/test/scala/unit/kafka/server/DelayedOperationTest.scala > f3ab3f4ff8eb1aa6b2ab87ba75f72eceb6649620 > core/src/test/scala/unit/kafka/server/ReplicaManagerTest.scala > 00d59337a99ac135e8689bd1ecd928f7b1423d79 > > Diff: https://reviews.apache.org/r/33378/diff/ > > > Testing > ------- > > New tests added > > > Thanks, > > Aditya Auradkar > >
