> On Aug. 20, 2014, 3:23 p.m., Jun Rao wrote: > > core/src/main/scala/kafka/consumer/PartitionAllocator.scala, line 27 > > <https://reviews.apache.org/r/23655/diff/3/?file=664708#file664708line27> > > > > Could we add a comment on what the return value is, especially the > > String part?
Sure - done > On Aug. 20, 2014, 3:23 p.m., Jun Rao wrote: > > core/src/main/scala/kafka/consumer/PartitionAllocator.scala, line 44 > > <https://reviews.apache.org/r/23655/diff/3/?file=664708#file664708line44> > > > > Could we use map { case(topic, replicaAssignment) => }? Will do. > On Aug. 20, 2014, 3:23 p.m., Jun Rao wrote: > > core/src/main/scala/kafka/consumer/PartitionAllocator.scala, line 57 > > <https://reviews.apache.org/r/23655/diff/3/?file=664708#file664708line57> > > > > owned => subscribed ? Yes - that's right. > On Aug. 20, 2014, 3:23 p.m., Jun Rao wrote: > > core/src/main/scala/kafka/consumer/PartitionAllocator.scala, line 77 > > <https://reviews.apache.org/r/23655/diff/3/?file=664708#file664708line77> > > > > Is the comment still valid? It doesn't seem we maintain isLocal any > > more. It is obsolete - will fix. > On Aug. 20, 2014, 3:23 p.m., Jun Rao wrote: > > core/src/main/scala/kafka/consumer/PartitionAllocator.scala, line 82 > > <https://reviews.apache.org/r/23655/diff/3/?file=664708#file664708line82> > > > > 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. Yes I did realize this while implementing it. i.e., we have to read ZK multiple times - once for each consumer. We could do it in the context, but I felt it was almost identical. That said, I think what you are saying is more for separation of functionality - i.e., the allocator should only allocate given some context. Let me see if I can make that work - the allocation context may be slightly different between the two allocators but I think that's okay. > On Aug. 20, 2014, 3:23 p.m., Jun Rao wrote: > > core/src/main/scala/kafka/consumer/PartitionAllocator.scala, line 85 > > <https://reviews.apache.org/r/23655/diff/3/?file=664708#file664708line85> > > > > Could we avoid using tuples and use named values through case instead? will do, although I won't be changing the RHS since that actually makes the code unnecessarily verbose. it will be clearer in the patch. > On Aug. 20, 2014, 3:23 p.m., Jun Rao wrote: > > core/src/main/scala/kafka/consumer/PartitionAllocator.scala, lines 104-105 > > <https://reviews.apache.org/r/23655/diff/3/?file=664708#file664708line104> > > > > Should this be info? Yes it should be info. > On Aug. 20, 2014, 3:23 p.m., Jun Rao wrote: > > core/src/test/scala/unit/kafka/consumer/PartitionAllocatorTest.scala, lines > > 50-51 > > <https://reviews.apache.org/r/23655/diff/3/?file=664713#file664713line50> > > > > If we move all ZK accesses outside of allocate(), the test can also be > > simplified. That is true - I actually tried it and it is simpler but not a whole lot. Furthermore changing the API as I mentioned earlier results in some changes in the range allocator which I don't want to make in this patch. In other words, I would rather remove the test than change the API in order to simplify it. > On Aug. 20, 2014, 3:23 p.m., Jun Rao wrote: > > core/src/main/scala/kafka/consumer/PartitionAllocator.scala, lines 154-155 > > <https://reviews.apache.org/r/23655/diff/3/?file=664708#file664708line154> > > > > Could we just use consumersPerTopicMap(topic)? Yes - although this is just moved from ZookeeperConsumerConnector. I did not want to touch the range allocation code. > On Aug. 20, 2014, 3:23 p.m., Jun Rao wrote: > > core/src/main/scala/kafka/utils/ZkUtils.scala, lines 661-665 > > <https://reviews.apache.org/r/23655/diff/3/?file=664712#file664712line661> > > > > Should this method be in TopicCount? Due to the allocation context change above I think we can get rid of this. Let me see. - Joel ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/23655/#review51104 ----------------------------------------------------------- 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 > >
