----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31650/#review75355 -----------------------------------------------------------
clients/src/main/java/org/apache/kafka/clients/CommonClientConfigs.java <https://reviews.apache.org/r/31650/#comment122365> Do key/value serializer/deserializer configs actually belong here? CommonClientConfigs is where shared configs for producers and consumers live. A producer is only interested in key/value serializer config and consumer is only interested in key/value deserializer config. If you are trying to move the addSerializerToConfig and addDeserializerToConfig out of the KafkaProducer and KafkaConsumer, maybe the ProducerConfig and ConsumerConfig would be a suitable place? clients/src/main/java/org/apache/kafka/clients/consumer/internals/Coordinator.java <https://reviews.apache.org/r/31650/#comment122377> This class name was very misleading to me. It should be renamed such that it's clear that it's used by clients to interact with coordinators, and is not the actual coordinator. Maybe CoordinatorClient, CoordinatorManager, or something else that makes this clear. clients/src/main/java/org/apache/kafka/clients/consumer/internals/Coordinator.java <https://reviews.apache.org/r/31650/#comment122397> This is really minor, but are longs necessary for these time parameters? Integer.MAX_VALUE translates to a little over 24 days. clients/src/main/java/org/apache/kafka/clients/consumer/internals/Coordinator.java <https://reviews.apache.org/r/31650/#comment122396> This is marking the receivedResponse as the time the request was sent rather than the time we received the response. clients/src/main/java/org/apache/kafka/clients/consumer/internals/Coordinator.java <https://reviews.apache.org/r/31650/#comment122421> I think this is simpler as: ```java boolean done = false; while (!done) { } ``` clients/src/main/java/org/apache/kafka/clients/consumer/internals/Coordinator.java <https://reviews.apache.org/r/31650/#comment122384> unless you plan on adding a condition to the loop, this can just be a while(true) loop. clients/src/main/java/org/apache/kafka/clients/consumer/internals/Coordinator.java <https://reviews.apache.org/r/31650/#comment122422> `if (responses.isEmpty())` clients/src/main/java/org/apache/kafka/common/protocol/Errors.java <https://reviews.apache.org/r/31650/#comment122360> Using the term "consumer" implies that generation ids are associated with a consumer, while they're really associated with a group. Maybe just call this ILLEGAL_GENERATION as stated in the wiki? https://cwiki.apache.org/confluence/display/KAFKA/Kafka+0.9+Consumer+Rewrite+Design#Kafka0.9ConsumerRewriteDesign-Groupmanagementprotocol core/src/main/scala/kafka/coordinator/GroupRegistry.scala <https://reviews.apache.org/r/31650/#comment122338> var to val for both of these. core/src/main/scala/kafka/coordinator/GroupRegistry.scala <https://reviews.apache.org/r/31650/#comment122339> the toString is not needed. core/src/test/scala/integration/kafka/api/ConsumerTest.scala <https://reviews.apache.org/r/31650/#comment122359> It would be nice to see unit tests specifically covering FetchManager. - Onur Karaman On March 3, 2015, 12:46 a.m., Guozhang Wang wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/31650/ > ----------------------------------------------------------- > > (Updated March 3, 2015, 12:46 a.m.) > > > Review request for kafka. > > > Bugs: KAFKA-1910 > https://issues.apache.org/jira/browse/KAFKA-1910 > > > Repository: kafka > > > Description > ------- > > See comments in KAFKA-1910 > > > Diffs > ----- > > clients/src/main/java/org/apache/kafka/clients/CommonClientConfigs.java > 06fcfe62cc1fe76f58540221698ef076fe150e96 > clients/src/main/java/org/apache/kafka/clients/KafkaClient.java > 8a3e55aaff7d8c26e56a8407166a4176c1da2644 > clients/src/main/java/org/apache/kafka/clients/NetworkClient.java > a7fa4a9dfbcfbc4d9e9259630253cbcded158064 > clients/src/main/java/org/apache/kafka/clients/consumer/ConsumerConfig.java > 5fb21001abd77cac839bd724afa04e377a3e82aa > clients/src/main/java/org/apache/kafka/clients/consumer/KafkaConsumer.java > 67ceb754a52c07143c69b053fe128b9e24060b99 > > clients/src/main/java/org/apache/kafka/clients/consumer/internals/Coordinator.java > PRE-CREATION > > clients/src/main/java/org/apache/kafka/clients/consumer/internals/FetchManager.java > PRE-CREATION > > clients/src/main/java/org/apache/kafka/clients/consumer/internals/Heartbeat.java > ee0751e4949120d114202c2299d49612a89b9d97 > > clients/src/main/java/org/apache/kafka/clients/consumer/internals/SubscriptionState.java > d41d3068c11d4b5c640467dc0ae1b7c20a8d128c > clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java > 7397e565fd865214529ffccadd4222d835ac8110 > clients/src/main/java/org/apache/kafka/clients/producer/ProducerConfig.java > 122375c473bf73caf05299b9f5174c6b226ca863 > clients/src/main/java/org/apache/kafka/common/network/Selector.java > 6baad9366a1975dbaba1786da91efeaa38533319 > clients/src/main/java/org/apache/kafka/common/protocol/Errors.java > ad2171f5417c93194f5f234bdc7fdd0b8d59a8a8 > clients/src/main/java/org/apache/kafka/common/requests/FetchResponse.java > e67c4c8332cb1dd3d9cde5de687df7760045dfe6 > > clients/src/main/java/org/apache/kafka/common/requests/HeartbeatResponse.java > 0057496228feeeccbc0c009a42f5268fa2cb8611 > > clients/src/main/java/org/apache/kafka/common/requests/JoinGroupRequest.java > 8c50e9be534c61ecf56106bf2b68cf678ea50d66 > > clients/src/main/java/org/apache/kafka/common/requests/JoinGroupResponse.java > 52b1803d8b558c1eeb978ba8821496c7d3c20a6b > > clients/src/main/java/org/apache/kafka/common/requests/ListOffsetResponse.java > cfac47a4a05dc8a535595542d93e55237b7d1e93 > > clients/src/main/java/org/apache/kafka/common/requests/MetadataResponse.java > 90f31413d7d80a06c0af359009cc271aa0c67be3 > > clients/src/main/java/org/apache/kafka/common/requests/OffsetCommitResponse.java > 4d3b9ececee4b4c0b50ba99da2ddbbb15f9cc08d > > clients/src/main/java/org/apache/kafka/common/requests/OffsetFetchResponse.java > edbed5880dc44fc178737a5e298c106a00f38443 > clients/src/main/java/org/apache/kafka/common/requests/ProduceResponse.java > a00dcdf15d1c7bac7228be140647bd7d849deb9b > clients/src/test/java/org/apache/kafka/clients/MockClient.java > 8f1a7a625e4eeafa44bbf9e5cff987de86c949be > core/src/main/scala/kafka/common/ErrorMapping.scala > eedc2f5f21dd8755fba891998456351622e17047 > core/src/main/scala/kafka/common/NoOffsetsCommittedException.scala > PRE-CREATION > core/src/main/scala/kafka/common/OffsetMetadataAndError.scala > 4cabffeacea09a49913505db19a96a55d58c0909 > core/src/main/scala/kafka/coordinator/ConsumerCoordinator.scala > 21790a5059ee00d6610be6f0389445327b88db1d > core/src/main/scala/kafka/coordinator/ConsumerRegistry.scala > b65c04d0a5d53bf92299d5f67f112be3da3bf77d > core/src/main/scala/kafka/coordinator/DelayedHeartbeat.scala > b1248e95d8a648b461f604c154879cc95dc7b1cb > core/src/main/scala/kafka/coordinator/GroupRegistry.scala > 7d17e102235134b6312271c4061abd27d7177f7e > core/src/main/scala/kafka/server/KafkaServer.scala > 426e522fc9819a0fc0f4e8269033552d716eb066 > core/src/test/scala/integration/kafka/api/ConsumerTest.scala PRE-CREATION > core/src/test/scala/integration/kafka/api/IntegrationTestHarness.scala > 5650b4a7b950b48af3e272947bfb5e271c4238c9 > core/src/test/scala/integration/kafka/api/ProducerFailureHandlingTest.scala > ba48a636dd0b0ed06646d56bb36aa3d43228604f > core/src/test/scala/unit/kafka/integration/KafkaServerTestHarness.scala > dc0512b526e914df7e7581b27df18f498da428e2 > core/src/test/scala/unit/kafka/server/OffsetCommitTest.scala > a2bb8855c3c0586b6b45b53ce534edee31b3bd12 > core/src/test/scala/unit/kafka/utils/TestUtils.scala > 6ce18076f6b5deb05b51c25be5bed9957e6b4339 > > Diff: https://reviews.apache.org/r/31650/diff/ > > > Testing > ------- > > > Thanks, > > Guozhang Wang > >
