----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30482/#review70533 -----------------------------------------------------------
core/src/main/scala/kafka/coordinator/ConsumerCoordinator.scala <https://reviews.apache.org/r/30482/#comment115743> Can we refactor the consumerGroupRegistries into it's own class that does locking internally. I really feel these free standing locks inside big classes are an anti pattern. It is impossible to verify correctness without basically searching through all the code in the class to determine where it is used. core/src/main/scala/kafka/coordinator/ConsumerCoordinator.scala <https://reviews.apache.org/r/30482/#comment115741> It would be nice to remove the handle prefix from the method names unless it has special significance. I used in KafkaApis as a way to differentiate the actual API implementations from helper methods. Seems cleaner as joinGroup, heartbeat, etc. core/src/main/scala/kafka/coordinator/GroupRegistry.scala <https://reviews.apache.org/r/30482/#comment115740> If these will be the strings the client specifies, might be nice to have them be simpler. E.g. "range" and "round-robin" core/src/main/scala/kafka/server/KafkaApis.scala <https://reviews.apache.org/r/30482/#comment115738> "...for sending *a* produce response" core/src/main/scala/kafka/server/KafkaApis.scala <https://reviews.apache.org/r/30482/#comment115739> ditto Not sure this stuff is actually here for review...may still be a work in progress. Overall this structure of code makes a ton of sense to me. Left some minor comments. - Jay Kreps On Feb. 1, 2015, 2:45 a.m., Guozhang Wang wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/30482/ > ----------------------------------------------------------- > > (Updated Feb. 1, 2015, 2:45 a.m.) > > > Review request for kafka. > > > Bugs: KAFKA-1333 > https://issues.apache.org/jira/browse/KAFKA-1333 > > > Repository: kafka > > > Description > ------- > > 1. Add ConsumerCoordinator with GroupRegistry and ConsumerRegistry metadata, > and ZK listeners. > 2. Add a delayed heartbeat purgatory based on HeartbeatBucket to expire > heartbeat requests. > 3. Add a delayed rebalance purgatory for preparing rebalance. > 4. Add a join-group purgatory for sending back responses with assigned > partitions. > 5. Add TimeMsKey / ConsumerKey and ConsumerGroupKey for delayed heartbeat / > join-group / rebalance purgatories. > 6. Refactor KafkaApis for handling JoinGroup / Heartbeat requests with > coordinator, and sending reponses via callbacks. > > > Diffs > ----- > > core/src/main/scala/kafka/coordinator/ConsumerCoordinator.scala > PRE-CREATION > core/src/main/scala/kafka/coordinator/GroupRegistry.scala PRE-CREATION > core/src/main/scala/kafka/coordinator/HeartbeatBucket.scala PRE-CREATION > core/src/main/scala/kafka/server/DelayedOperationKey.scala > fb7e9ed5c16dd15b71e1b1ac12948641185871db > core/src/main/scala/kafka/server/KafkaApis.scala > f2b027bf944e735fd52cc282690ec1b8395f9290 > core/src/main/scala/kafka/server/KafkaServer.scala > 89200da30a04943f0b9befe84ab17e62b747c8c4 > > Diff: https://reviews.apache.org/r/30482/diff/ > > > Testing > ------- > > > Thanks, > > Guozhang Wang > >
