----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/23655/#review51182 -----------------------------------------------------------
core/src/main/scala/kafka/consumer/PartitionAssignor.scala <https://reviews.apache.org/r/23655/#comment89203> To avoid duplicating the ZK read, we can probably change all the vals in the constructor to def. Then, we can intialize some private vals during initialization. core/src/main/scala/kafka/consumer/PartitionAssignor.scala <https://reviews.apache.org/r/23655/#comment89207> It seems that (a) implies (b). core/src/main/scala/kafka/consumer/PartitionAssignor.scala <https://reviews.apache.org/r/23655/#comment89314> Are those fields needed in the contructor? The same set of fields are already passed into AssignmentContext. core/src/main/scala/kafka/consumer/PartitionAssignor.scala <https://reviews.apache.org/r/23655/#comment89315> Instead of passing in forConsumers, would it be simpler to just return the assignment for each consumer in a map? The unit test can also be made simpler since we just need to test the coverage and uniformity. core/src/main/scala/kafka/consumer/PartitionAssignor.scala <https://reviews.apache.org/r/23655/#comment89205> One of the implications of this is that the distribution is going to be sensitive to # topics. In the case when #topics == #consumerThreads, partitions from the same topic will be assigned to the same consumerThread. In general, if #topics is a multiple of #consumerThreads, a similar issue can happen. So, I am wondering if this is better than just sorting the consumer ids in some random order like hashcode. For debugging purpose, we can log the consumer ids in that order. core/src/main/scala/kafka/consumer/PartitionAssignor.scala <https://reviews.apache.org/r/23655/#comment89431> Since RoundRobin assigns partitions across topics, should we just log all partitions and all consumers once in the right order? core/src/main/scala/kafka/consumer/PartitionAssignor.scala <https://reviews.apache.org/r/23655/#comment89204> Could we do partitionsForTopic(topic) here too? core/src/test/scala/unit/kafka/consumer/PartitionAssignorTest.scala <https://reviews.apache.org/r/23655/#comment89302> Do we need a while loop here? Can this just be 2 + random.nextInt(consumerCount - 1)? core/src/test/scala/unit/kafka/consumer/PartitionAssignorTest.scala <https://reviews.apache.org/r/23655/#comment89432> This test is pretty complicated. I am wondering if we need to test the case that c1 and cx give consistent assignment. If the inputs to the assignor are the same and the assignor is deterministic, the assignment is always going to be consistent. We probably just need to test completeness and uniqueness. core/src/test/scala/unit/kafka/consumer/PartitionAssignorTest.scala <https://reviews.apache.org/r/23655/#comment89434> Shouldn't we assert count==1? Do we need to also make sure every original partition is in assignment? core/src/test/scala/unit/kafka/consumer/PartitionAssignorTest.scala <https://reviews.apache.org/r/23655/#comment89436> Should we rename this to partitionsPerStream? - Jun Rao On Aug. 21, 2014, 1:10 a.m., Joel Koshy wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/23655/ > ----------------------------------------------------------- > > (Updated Aug. 21, 2014, 1:10 a.m.) > > > Review request for kafka. > > > Bugs: KAFKA-687 > https://issues.apache.org/jira/browse/KAFKA-687 > > > Repository: kafka > > > Description > ------- > > v4 > > > Diffs > ----- > > core/src/main/scala/kafka/consumer/ConsumerConfig.scala > 1cf2f62ba02e4aa66bfa7575865e5d57baf82212 > core/src/main/scala/kafka/consumer/PartitionAssignor.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/PartitionAssignorTest.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 > >
