----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/23655/#review48454 -----------------------------------------------------------
core/src/main/scala/kafka/consumer/ConsumerConfig.scala <https://reviews.apache.org/r/23655/#comment85261> Will do. core/src/main/scala/kafka/consumer/PartitionAllocator.scala <https://reviews.apache.org/r/23655/#comment85262> Will fix. core/src/main/scala/kafka/consumer/PartitionAllocator.scala <https://reviews.apache.org/r/23655/#comment85265> After thinking about it and discussing off-thread, we are thinking of: def allocate(partitions, consumerIdsPerTopic) At the same time, this code is not particularly complex so reusability is nice-have - worst case we can just adapt it to use in the new consumer. core/src/main/scala/kafka/consumer/PartitionAllocator.scala <https://reviews.apache.org/r/23655/#comment85268> Thanks for catching that - will fix. core/src/main/scala/kafka/consumer/PartitionAllocator.scala <https://reviews.apache.org/r/23655/#comment85269> After thinking about it I think we can actually merge the two and get rid of symmetric. So we will just have "range" and "roundrobin" (or maybe name it "uniform"). Will see how that turns out after I update the patch. core/src/main/scala/kafka/consumer/PartitionAllocator.scala <https://reviews.apache.org/r/23655/#comment85074> This uses the "<" operator defined in StringOps - so it probably translates to compareTo underneath the hood. That said, I can change it to compareTo just to be sure. core/src/main/scala/kafka/consumer/PartitionAllocator.scala <https://reviews.apache.org/r/23655/#comment85075> Yeah I didn't bother with describing the old allocator as it was copied/moved from the consumer connector - I will add brief comments. core/src/main/scala/kafka/consumer/ZookeeperConsumerConnector.scala <https://reviews.apache.org/r/23655/#comment85271> Will do. core/src/main/scala/kafka/consumer/ZookeeperConsumerConnector.scala <https://reviews.apache.org/r/23655/#comment85077> I think it is very useful. While the offset checker is useful, it can take a while to run if you want to (say) get a count of all partitions owned by the consumer. An mbean on the other hand helps continuously monitor how even your partitions are distributed across all your consumers. core/src/test/scala/unit/kafka/consumer/PartitionAllocatorTest.scala <https://reviews.apache.org/r/23655/#comment85272> Will add it. - Joel Koshy On July 18, 2014, 10:57 p.m., Joel Koshy wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/23655/ > ----------------------------------------------------------- > > (Updated July 18, 2014, 10:57 p.m.) > > > Review request for kafka. > > > Bugs: KAFKA-687 > https://issues.apache.org/jira/browse/KAFKA-687 > > > Repository: kafka > > > Description > ------- > > The updated diff contains the mbeans for ownership counts. > The comments in the code and the summary are pretty self-explanatory. > > Things to think about: > * Naming - do symmetric/range/roundrobin make sense? > * The comments briefly summarize why we needed a separate symmetric mode but > let me know if that is unclear. > * Rebalance time will be slightly higher - I have not measured (will do that) > > > Diffs > ----- > > core/src/main/scala/kafka/consumer/ConsumerConfig.scala > 1cf2f62ba02e4aa66bfa7575865e5d57baf82212 > core/src/main/scala/kafka/consumer/PartitionAllocator.scala PRE-CREATION > core/src/main/scala/kafka/consumer/TopicCount.scala > c79311097c5bd6718cb6a7fc403f804a1a939353 > core/src/main/scala/kafka/consumer/ZookeeperConsumerConnector.scala > 65f518d47c7555c42c4bff39c211814831f4b8b6 > core/src/main/scala/kafka/metrics/KafkaMetricsGroup.scala > a20ab90165cc7ebb1cf44078efe23a53938c8df6 > core/src/main/scala/kafka/utils/ZkUtils.scala > dcdc1ce2b02c996294e19cf480736106aaf29511 > core/src/test/scala/unit/kafka/consumer/PartitionAllocatorTest.scala > PRE-CREATION > > Diff: https://reviews.apache.org/r/23655/diff/ > > > Testing > ------- > > * I did the unit tests (including the new one) as well as mirror maker system > test suite with roundrobin. While this is being reviewed I will run the > system tests with symmetric > > > Thanks, > > Joel Koshy > >
