----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/23655/#review51153 -----------------------------------------------------------
core/src/main/scala/kafka/consumer/PartitionAssignor.scala <https://reviews.apache.org/r/23655/#comment89191> AssignmentContext now contains all the zk reads, so the assignor only does assignment. This potentially makes it easier to unit test since you don't need to mock. HOWEVER, I actually think it is easier to use the zkclient mock - because the code to build the threadId map, consumers for topic map, etc. is fairly complex and is already in ZkUtils/elsewhere. So that's why I did not change the unit tests. With that said, let me know if anyone feels it is better to revert to the previous version. core/src/main/scala/kafka/consumer/PartitionAssignor.scala <https://reviews.apache.org/r/23655/#comment89190> FYI, this code is repetitive, but cannot avoid it easily since "this" needs to be the first statement. I would rather not pollute ZkUtils any further, so I thought it was okay to leave as is even though it incurs a redundant zk read. Or we can just remove it and avoid the current primary constructor of this class which was added to attempt to avoid the mock classes in the unit test. (See other comment also) - Joel Koshy 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 > >
