----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/29831/#review70581 -----------------------------------------------------------
Onur, we should be able to check in after these review comments are addressed. Also, how would deleting offsets for a group work when the offset storage is Kafka? It's fine to not address it in this patch. Can you please create a JIRA to handle that? core/src/main/scala/kafka/admin/AdminUtils.scala <https://reviews.apache.org/r/29831/#comment115788> This is confusing. I think we decided we will keep the definition of a valid delete consistent. In this case, we should just detect the inactive consumer groups for this topic and delete those. This check can be removed, I think. core/src/main/scala/kafka/admin/ConsumerGroupCommand.scala <https://reviews.apache.org/r/29831/#comment115798> lines too long core/src/main/scala/kafka/admin/ConsumerGroupCommand.scala <https://reviews.apache.org/r/29831/#comment115799> same here core/src/main/scala/kafka/admin/ConsumerGroupCommand.scala <https://reviews.apache.org/r/29831/#comment115797> Suggest you create a new local variable for the option docs. That way the lines won't be so long. Try to stick to within a width of 100. core/src/main/scala/kafka/utils/ZkUtils.scala <https://reviews.apache.org/r/29831/#comment115789> Let's add consumerGroupOwners inside ZKGroupDirs so we don't have to hardcode zk paths like this. core/src/main/scala/kafka/utils/ZkUtils.scala <https://reviews.apache.org/r/29831/#comment115790> same here core/src/test/scala/unit/kafka/admin/DeleteConsumerGroupTest.scala <https://reviews.apache.org/r/29831/#comment115791> this test probably should have multiple topics and ensure that the API deletes only the topic provided? core/src/test/scala/unit/kafka/admin/DeleteConsumerGroupTest.scala <https://reviews.apache.org/r/29831/#comment115792> Maybe include a topic unrelated to both consumer groups here as well? core/src/test/scala/unit/kafka/admin/DeleteConsumerGroupTest.scala <https://reviews.apache.org/r/29831/#comment115793> I think these test names are very long. It is definitely useful to have test names that convey the purpose but since all tests here include deleting consumer groups, I suggest you take that out of all the test names. So "DeleteConsumerGroupInfo" can be dropped. core/src/test/scala/unit/kafka/admin/DeleteConsumerGroupTest.scala <https://reviews.apache.org/r/29831/#comment115795> This test is odd. As I suggested earlier, valid delete should only have one rule - active consumer groups will not be deleted. It is alright to delete consumer group information for a topic if the topic exists. core/src/test/scala/unit/kafka/admin/DeleteConsumerGroupTest.scala <https://reviews.apache.org/r/29831/#comment115796> verifyTopicDeletion is duplicated across 2 tests now. It is useful to refactor it into maybe TestUtils? - Neha Narkhede On Jan. 30, 2015, 7:10 p.m., Onur Karaman wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/29831/ > ----------------------------------------------------------- > > (Updated Jan. 30, 2015, 7:10 p.m.) > > > Review request for kafka. > > > Bugs: KAFKA-1476 > https://issues.apache.org/jira/browse/KAFKA-1476 > > > Repository: kafka > > > Description > ------- > > Merged in work for KAFKA-1476 and sub-task KAFKA-1826 > > > Diffs > ----- > > bin/kafka-consumer-groups.sh PRE-CREATION > core/src/main/scala/kafka/admin/AdminUtils.scala > 28b12c7b89a56c113b665fbde1b95f873f8624a3 > core/src/main/scala/kafka/admin/ConsumerGroupCommand.scala PRE-CREATION > core/src/main/scala/kafka/utils/ZkUtils.scala > c14bd455b6642f5e6eb254670bef9f57ae41d6cb > core/src/test/scala/unit/kafka/admin/DeleteConsumerGroupTest.scala > PRE-CREATION > core/src/test/scala/unit/kafka/utils/TestUtils.scala > 54755e8dd3f23ced313067566cd4ea867f8a496e > > Diff: https://reviews.apache.org/r/29831/diff/ > > > Testing > ------- > > > Thanks, > > Onur Karaman > >
