> On March 5, 2015, 10:42 p.m., Onur Karaman wrote: > > clients/src/main/java/org/apache/kafka/clients/consumer/internals/Coordinator.java, > > lines 137-138 > > <https://reviews.apache.org/r/31650/diff/1/?file=882439#file882439line137> > > > > This is really minor, but are longs necessary for these time parameters? > > > > Integer.MAX_VALUE translates to a little over 24 days.
These two configs are defined in the common client configs that are used by producers also. I think it would be ok to be more conversative on these config values. > On March 5, 2015, 10:42 p.m., Onur Karaman wrote: > > clients/src/main/java/org/apache/kafka/clients/consumer/internals/Coordinator.java, > > line 183 > > <https://reviews.apache.org/r/31650/diff/1/?file=882439#file882439line183> > > > > This is marking the receivedResponse as the time the request was sent > > rather than the time we received the response. Actually we do not need a last heart beat response as consumer client does not check for time out expiration at all. > On March 5, 2015, 10:42 p.m., Onur Karaman wrote: > > clients/src/main/java/org/apache/kafka/clients/consumer/internals/Coordinator.java, > > lines 218-234 > > <https://reviews.apache.org/r/31650/diff/1/?file=882439#file882439line218> > > > > I think this is simpler as: > > ```java > > boolean done = false; > > while (!done) { > > } > > ``` Actually this is not simpler with this pattern as you need to initialize inside the loop as true determine whether to override it to false, not vice versa, right? > On March 5, 2015, 10:42 p.m., Onur Karaman wrote: > > clients/src/main/java/org/apache/kafka/common/protocol/Errors.java, lines > > 71-72 > > <https://reviews.apache.org/r/31650/diff/1/?file=882446#file882446line71> > > > > 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 Good point. - Guozhang ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31650/#review75355 ----------------------------------------------------------- On March 5, 2015, 10:57 p.m., Guozhang Wang wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/31650/ > ----------------------------------------------------------- > > (Updated March 5, 2015, 10:57 p.m.) > > > Review request for kafka. > > > Bugs: KAFKA-1910 > https://issues.apache.org/jira/browse/KAFKA-1910 > > > Repository: kafka > > > Description > ------- > > See comments in KAFKA-1910; > > Updated RB includes unit test for Coordinator / FetchManager / Heartbeat and > a couple changes on MemoryRecords and test utils. > > > 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/clients/producer/internals/Sender.java > ed9c63a6679e3aaf83d19fde19268553a4c107c2 > 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/record/MemoryRecords.java > 083e7a39249ab56a73a014b106876244d619f189 > 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 > > clients/src/test/java/org/apache/kafka/clients/consumer/internals/CoordinatorTest.java > PRE-CREATION > > clients/src/test/java/org/apache/kafka/clients/consumer/internals/FetchManagerTest.java > PRE-CREATION > > clients/src/test/java/org/apache/kafka/clients/consumer/internals/HeartbeatTest.java > PRE-CREATION > > clients/src/test/java/org/apache/kafka/clients/consumer/internals/SubscriptionStateTest.java > 090087a319e2697d3a0653ca947d2cfa6d53f6c2 > > clients/src/test/java/org/apache/kafka/clients/producer/internals/RecordAccumulatorTest.java > c1bc40648479d4c2ae4ac52f40dadc070a6bcf6f > > clients/src/test/java/org/apache/kafka/clients/producer/internals/SenderTest.java > ea56c997cb90d5bac8e3706dfc1eaae5b1555ccb > clients/src/test/java/org/apache/kafka/common/record/MemoryRecordsTest.java > e343327faf115a901657ec6da8e0c5b8bbf0b76a > 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 > 378a74d9e8e408e1e5d283badf3eded6333fadff > core/src/test/scala/integration/kafka/api/ConsumerTest.scala PRE-CREATION > core/src/test/scala/integration/kafka/api/IntegrationTestHarness.scala > 82fe4c9a138617f7af99a54cca7176d6c80747d0 > core/src/test/scala/integration/kafka/api/ProducerFailureHandlingTest.scala > 8246e1281097e33eb8fadb291dc5feefdb631515 > core/src/test/scala/unit/kafka/integration/KafkaServerTestHarness.scala > dc0512b526e914df7e7581b27df18f498da428e2 > core/src/test/scala/unit/kafka/server/OffsetCommitTest.scala > ea9b315099254eca82b8d5534bc02535b5996ee9 > core/src/test/scala/unit/kafka/utils/TestUtils.scala > 6ce18076f6b5deb05b51c25be5bed9957e6b4339 > > Diff: https://reviews.apache.org/r/31650/diff/ > > > Testing > ------- > > > Thanks, > > Guozhang Wang > >