----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33378/#review91319 -----------------------------------------------------------
LGTM - just a few minor comments. clients/src/main/java/org/apache/kafka/common/requests/ProduceResponse.java (line 101) <https://reviews.apache.org/r/33378/#comment144632> Would prefer seeing this in this style: ``` func() { statement; } ``` clients/src/test/java/org/apache/kafka/clients/consumer/internals/FetcherTest.java (line 68) <https://reviews.apache.org/r/33378/#comment144638> EPSILON? core/src/main/scala/kafka/api/FetchResponse.scala (line 169) <https://reviews.apache.org/r/33378/#comment144643> A: B core/src/main/scala/kafka/api/FetchResponse.scala (line 170) <https://reviews.apache.org/r/33378/#comment144644> A: B core/src/main/scala/kafka/api/FetchResponse.scala (line 179) <https://reviews.apache.org/r/33378/#comment144646> Prefer placing throttleTimeSize on the next line. core/src/main/scala/kafka/api/FetchResponse.scala (line 229) <https://reviews.apache.org/r/33378/#comment144655> The throttleTimeSize... (However, see following comment) core/src/main/scala/kafka/api/FetchResponse.scala (line 230) <https://reviews.apache.org/r/33378/#comment144653> Just wondering if we should move all this to `FetchResponse` class (not object). i.e., add `headerSize` and `writeHeaderTo(buffer)` member methods since the throttle-time is really part of the header. Depending on the version, those two methods can then do the right thing. This code will then just become: ``` private val buffer = ByteBuffer.allocate(4 + fetchResponse.headerSize) fetchResponse.writeHeaderTo(buffer) ``` core/src/main/scala/kafka/server/DelayedFetch.scala (line 135) <https://reviews.apache.org/r/33378/#comment144657> For these, I'm wondering if we should put in the actual delay and in KAFKA-2136 just add a config to enable/disable quotas altogether. core/src/test/scala/unit/kafka/server/ReplicaManagerTest.scala (line 81) <https://reviews.apache.org/r/33378/#comment144658> delayTime: Int - Joel Koshy On July 1, 2015, 2:44 a.m., Aditya Auradkar wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/33378/ > ----------------------------------------------------------- > > (Updated July 1, 2015, 2:44 a.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. > - Addressed Joel's comments > > 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 > 56281ee15cc33dfc96ff64d5b1e596047c7132a4 > > clients/src/main/java/org/apache/kafka/clients/producer/internals/Sender.java > 07e65d4a54ba4eef5b787eba3e71cbe9f6a920bd > 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 > 5b38f8554898e54800abd65a7415dd0ac41fd958 > core/src/main/scala/kafka/api/FetchResponse.scala > 0b6b33ab6f7a732ff1322b6f48abd4c43e0d7215 > core/src/main/scala/kafka/api/ProducerRequest.scala > c866180d3680da03e48d374415f10220f6ca68c4 > core/src/main/scala/kafka/api/ProducerResponse.scala > 5d1fac4cb8943f5bfaa487f8e9d9d2856efbd330 > core/src/main/scala/kafka/consumer/SimpleConsumer.scala > c16f7edd322709060e54c77eb505c44cbd77a4ec > 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 > d63bc18d795a6f0e6994538ca55a9a46f7fb8ffd > core/src/main/scala/kafka/server/OffsetManager.scala > 5cca85cf727975f6d3acb2223fd186753ad761dc > 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/ReplicaManagerTest.scala > 00d59337a99ac135e8689bd1ecd928f7b1423d79 > > Diff: https://reviews.apache.org/r/33378/diff/ > > > Testing > ------- > > New tests added > > > Thanks, > > Aditya Auradkar > >
