----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/23655/#review51104 -----------------------------------------------------------
core/src/main/scala/kafka/consumer/PartitionAllocator.scala <https://reviews.apache.org/r/23655/#comment89062> Could we add a comment on what the return value is, especially the String part? core/src/main/scala/kafka/consumer/PartitionAllocator.scala <https://reviews.apache.org/r/23655/#comment89063> Could we use map { case(topic, replicaAssignment) => }? core/src/main/scala/kafka/consumer/PartitionAllocator.scala <https://reviews.apache.org/r/23655/#comment89064> owned => subscribed ? core/src/main/scala/kafka/consumer/PartitionAllocator.scala <https://reviews.apache.org/r/23655/#comment89068> Is the comment still valid? It doesn't seem we maintain isLocal any more. core/src/main/scala/kafka/consumer/PartitionAllocator.scala <https://reviews.apache.org/r/23655/#comment89066> It seems that we are reading the consumer info from ZK multiple times. Could we do all the necessary zk reads in AllocatorContext once? Then allocate() is just doing the allocation. core/src/main/scala/kafka/consumer/PartitionAllocator.scala <https://reviews.apache.org/r/23655/#comment89065> Could we avoid using tuples and use named values through case instead? core/src/main/scala/kafka/consumer/PartitionAllocator.scala <https://reviews.apache.org/r/23655/#comment89067> Should this be info? core/src/main/scala/kafka/consumer/PartitionAllocator.scala <https://reviews.apache.org/r/23655/#comment89061> Could we just use consumersPerTopicMap(topic)? core/src/main/scala/kafka/utils/ZkUtils.scala <https://reviews.apache.org/r/23655/#comment89060> Should this method be in TopicCount? core/src/test/scala/unit/kafka/consumer/PartitionAllocatorTest.scala <https://reviews.apache.org/r/23655/#comment89069> If we move all ZK accesses outside of allocate(), the test can also be simplified. - Jun Rao On Aug. 19, 2014, 7:08 p.m., Joel Koshy wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/23655/ > ----------------------------------------------------------- > > (Updated Aug. 19, 2014, 7:08 p.m.) > > > Review request for kafka. > > > Bugs: KAFKA-687 > https://issues.apache.org/jira/browse/KAFKA-687 > > > Repository: kafka > > > Description > ------- > > KAFKA-687; Add a roundrobin partition assignment scheme to the consumer. > > > 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 > 8b0ae5785e08272d0ea12483beae597f2eac4343 > core/src/main/scala/kafka/consumer/ZookeeperConsumerConnector.scala > acfd064bdba2b031f8869011da79649efd80946f > core/src/main/scala/kafka/metrics/KafkaMetricsGroup.scala > 00df4621fd724826a1e79d849c762ac1c4f49868 > 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 > >
