> On Aug. 28, 2014, 1:32 a.m., Jun Rao wrote: > > core/src/main/scala/kafka/consumer/ConsumerConfig.scala, line 52 > > <https://reviews.apache.org/r/23655/diff/5/?file=668390#file668390line52> > > > > This probably should be named DefaultPartitionAssignmentStrategy.
Ok - it seems a number of defaults are improperly named. > On Aug. 28, 2014, 1:32 a.m., Jun Rao wrote: > > core/src/main/scala/kafka/consumer/PartitionAssignor.scala, line 47 > > <https://reviews.apache.org/r/23655/diff/5/?file=668391#file668391line47> > > > > partitionAssginment is a bit confusing now that we have the partition > > assignor. Perhaps both the val and the method name in ZkUtils should be > > called replicaAssignmentPerTopic. Agreed - there is already another replicaAssignmentPerTopic utility in ZkUtils, but it turns out I can actually use ZkUtils.getPartitionsForTopic. > On Aug. 28, 2014, 1:32 a.m., Jun Rao wrote: > > core/src/test/scala/unit/kafka/consumer/PartitionAssignorTest.scala, lines > > 214-222 > > <https://reviews.apache.org/r/23655/diff/5/?file=668397#file668397line214> > > > > I am not sure if this is necessary. The input is a map with > > topicAndPartition as the key. This implies that each partition has only 1 > > owner. That's very true - dumb mistake. I actually do want to check for conflicts so I refactored the test a bit and managed to get rid of the "forConsumer" API version of the roundrobin assignor. - Joel ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/23655/#review51694 ----------------------------------------------------------- On Aug. 25, 2014, 7:36 p.m., Joel Koshy wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/23655/ > ----------------------------------------------------------- > > (Updated Aug. 25, 2014, 7:36 p.m.) > > > Review request for kafka. > > > Bugs: KAFKA-687 > https://issues.apache.org/jira/browse/KAFKA-687 > > > Repository: kafka > > > Description > ------- > > v5 > > > Diffs > ----- > > > clients/src/main/java/org/apache/kafka/common/requests/MetadataResponse.java > d97962d3840179b1abf01459522c58e59102ac8d > 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/tools/MirrorMaker.scala > b8698ee1469c8fbc92ccc176d916eb3e28b87867 > 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 > >
