Re: Review Request 27990: Patch for KAFKA-1751
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27990/ --- (Updated Nov. 27, 2014, 11:39 a.m.) Review request for kafka. Bugs: KAFKA-1751 https://issues.apache.org/jira/browse/KAFKA-1751 Repository: kafka Description (updated) --- KAFKA-1751 / CR fixes KAFKA-1751 / CR fixes #2 KAFKA-1751: CR#2 Diffs (updated) - core/src/main/scala/kafka/admin/AdminUtils.scala 28b12c7b89a56c113b665fbde1b95f873f8624a3 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
Re: Review Request 27990: Patch for KAFKA-1751
--- 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
Re: Review Request 27990: Patch for KAFKA-1751
--- 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 (updated) --- KAFKA-1751 / CR fixes KAFKA-1751 / CR fixes #2 Diffs (updated) - 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
Re: Review Request 27990: Patch for KAFKA-1751
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27990/ --- (Updated Nov. 17, 2014, 2:25 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 (updated) - 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
Re: Review Request 27990: Patch for KAFKA-1751
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27990/ --- (Updated Nov. 17, 2014, 2:33 p.m.) Review request for kafka. Bugs: KAFKA-1751 https://issues.apache.org/jira/browse/KAFKA-1751 Repository: kafka Description (updated) --- KAFKA-1751 / CR fixes Diffs (updated) - 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
Re: Review Request 27990: Patch for KAFKA-1751
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27990/#review61855 --- Ship it! Looks good! Two minor readability comments from my side, and a non-binding +1. core/src/main/scala/kafka/admin/ReassignPartitionsCommand.scala https://reviews.apache.org/r/27990/#comment103764 I still think its pretty difficult to tell here that entry._2 refers to replicas. core/src/test/scala/unit/kafka/admin/AdminTest.scala https://reviews.apache.org/r/27990/#comment103766 Maybe comment on why you want to fail here. Unlike assertions, these tests are not self documenting. - Gwen Shapira On Nov. 17, 2014, 2:33 p.m., Dmitry Pekar wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27990/ --- (Updated Nov. 17, 2014, 2:33 p.m.) Review request for kafka. Bugs: KAFKA-1751 https://issues.apache.org/jira/browse/KAFKA-1751 Repository: kafka Description --- KAFKA-1751 / CR fixes 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
Re: Review Request 27990: Patch for KAFKA-1751
--- 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
Review Request 27990: Patch for KAFKA-1751
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27990/ --- 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
