----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27990/#review61644 -----------------------------------------------------------
Looks great! Few comments and nitpicks below. core/src/main/scala/kafka/admin/ReassignPartitionsCommand.scala <https://reviews.apache.org/r/27990/#comment103376> Should be unnecessary core/src/main/scala/kafka/admin/ReassignPartitionsCommand.scala <https://reviews.apache.org/r/27990/#comment103378> _1 is "topicAndPartition" and _2 is list of brokers we are going to assign them to? I'd use matching to give them explicit names. Our code guide discourages _1 and _2 unless its really obvious. core/src/main/scala/kafka/admin/ReassignPartitionsCommand.scala <https://reviews.apache.org/r/27990/#comment103379> I think we can do better here - more readable and better use of Scala's lists. We are just looking for items in "topicsToReassign" that don't exist in "topicPartitionsToReassign", right? Why not convert both to list of topics and then use listA -- listB to find the diff? core/src/main/scala/kafka/admin/ReassignPartitionsCommand.scala <https://reviews.apache.org/r/27990/#comment103381> Shouldn't we be validating partitions here too? core/src/main/scala/kafka/admin/ReassignPartitionsCommand.scala <https://reviews.apache.org/r/27990/#comment103382> I'd probably optimize it to be a list operation - i.e. take a list of brokers and compare with result of getAllBrokersInCluster - to avoid too many ZK calls. But this isn't exactly a place where optimizations matter, so feel free to leave this alone :) core/src/main/scala/kafka/admin/ReassignPartitionsCommand.scala <https://reviews.apache.org/r/27990/#comment103383> This pattern appears twice (here and in "validate"), maybe give it its own function. core/src/test/scala/unit/kafka/admin/AdminTest.scala <https://reviews.apache.org/r/27990/#comment103388> I really like the test cleanup. Looks much better now. We still need to make sure this test will fail if the reassignment command magically succeeds and not exception is thrown. core/src/test/scala/unit/kafka/admin/DeleteTopicTest.scala <https://reviews.apache.org/r/27990/#comment103389> Any reason for this change? - Gwen Shapira On Nov. 13, 2014, 2:39 p.m., Dmitry Pekar wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/27990/ > ----------------------------------------------------------- > > (Updated Nov. 13, 2014, 2:39 p.m.) > > > Review request for kafka. > > > Bugs: KAFKA-1751 > https://issues.apache.org/jira/browse/KAFKA-1751 > > > Repository: kafka > > > Description > ------- > > KAFKA-1751: handle "broker not exists" and "topic not exists" scenarios > > > Diffs > ----- > > core/src/main/scala/kafka/admin/ReassignPartitionsCommand.scala > 979992b68af3723cd229845faff81c641123bb88 > core/src/test/scala/unit/kafka/admin/AdminTest.scala > e28979827110dfbbb92fe5b152e7f1cc973de400 > core/src/test/scala/unit/kafka/admin/DeleteTopicTest.scala > 29cc01bcef9cacd8dec1f5d662644fc6fe4994bc > > Diff: https://reviews.apache.org/r/27990/diff/ > > > Testing > ------- > > > Thanks, > > Dmitry Pekar > >
