----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27990/#review62922 -----------------------------------------------------------
In addition to ensuring that the reassignment command doesn't reassign non-existent topics to non-existent brokers, but it may not be sufficient. We need to see how the controller handles these cases. There can be a window after the command checks for non-existent topics/brokers and another admin command sneaking in to change the state. core/src/main/scala/kafka/admin/ReassignPartitionsCommand.scala <https://reviews.apache.org/r/27990/#comment105096> Looks like this is meant to check for non existent topics. However, I'm not sure that this quite works. For example, if non existent topics are specified, getReplicaAssignmentForTopics throws an exception instead of dropping the topic out of the list. I think we should just the check topic API from AdminUtils, create a list of topics that don't exist and fail the operation with the appropriate error message that lists all non-existent topics. core/src/main/scala/kafka/admin/ReassignPartitionsCommand.scala <https://reviews.apache.org/r/27990/#comment105097> Both these APIs could be moved to AdminUtils core/src/main/scala/kafka/admin/ReassignPartitionsCommand.scala <https://reviews.apache.org/r/27990/#comment105098> It is worth checking for the topic explicitly (using topicExists) and give an appropriate error message. core/src/test/scala/unit/kafka/admin/AdminTest.scala <https://reviews.apache.org/r/27990/#comment105101> We should add 2 test cases that expliclity try to reassign topics that don't exist and brokers that don't exist. core/src/test/scala/unit/kafka/admin/AdminTest.scala <https://reviews.apache.org/r/27990/#comment105099> Can you change this to use intercept[AdminCommandFailedException] - Neha Narkhede On Nov. 19, 2014, 9:57 a.m., Dmitry Pekar wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/27990/ > ----------------------------------------------------------- > > (Updated Nov. 19, 2014, 9:57 a.m.) > > > Review request for kafka. > > > Bugs: KAFKA-1751 > https://issues.apache.org/jira/browse/KAFKA-1751 > > > Repository: kafka > > > Description > ------- > > KAFKA-1751 / CR fixes > > > KAFKA-1751 / CR fixes #2 > > > 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 > >
