----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/20745/#review41626 -----------------------------------------------------------
core/src/main/scala/kafka/controller/ControllerChannelManager.scala <https://reviews.apache.org/r/20745/#comment75161> better to not leak the logic from delete topic manager here. It is worth exposing the check as an API in DeleteTopicManager. In any case, can you explain the motivation behind your change? core/src/main/scala/kafka/controller/KafkaController.scala <https://reviews.apache.org/r/20745/#comment75162> did you make this change since onControllerResignation already holds the same lock? In general, for tricky bugs like deadlock, it is a good idea to explain all the significant changes, so it is easier to review. core/src/main/scala/kafka/controller/TopicDeletionManager.scala <https://reviews.apache.org/r/20745/#comment75158> * delete topic wasn't initiated for the given topic Nevertheless, it seems like the loop goes over topics that are queued up for deletion, so is this comment right? core/src/main/scala/kafka/log/LogManager.scala <https://reviews.apache.org/r/20745/#comment75159> isLogExists -> doesLogExist core/src/test/scala/unit/kafka/admin/DeleteTopicTest.scala <https://reviews.apache.org/r/20745/#comment75160> Given that waitUntilTrue has been changed to fail the test, we can remove all the wrapper asserts - Neha Narkhede On April 27, 2014, 6:43 a.m., Timothy Chen wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/20745/ > ----------------------------------------------------------- > > (Updated April 27, 2014, 6:43 a.m.) > > > Review request for kafka. > > > Bugs: KAFKA-1397 > https://issues.apache.org/jira/browse/KAFKA-1397 > > > Repository: kafka > > > Description > ------- > > Fix delete topic tests and deadlock > > > Diffs > ----- > > core/src/main/scala/kafka/controller/ControllerChannelManager.scala > 919aeb26f93d2fc34d873cfb3dbfab7235a9d635 > core/src/main/scala/kafka/controller/KafkaController.scala > 933de9dd324c7086efe6aa610335ef370d9e9c12 > core/src/main/scala/kafka/controller/ReplicaStateMachine.scala > 0e47dac8cbf65a86d053a3371a18af467afd70ae > core/src/main/scala/kafka/controller/TopicDeletionManager.scala > e4bc2439ce1933c7c7571d255464ee678226a6cb > core/src/main/scala/kafka/log/LogManager.scala > ac67f081e6219fd2181479e7a2bb88ea6044e6cc > core/src/main/scala/kafka/server/KafkaApis.scala > 0b668f230c8556fdf08654ce522a11847d0bf39b > core/src/main/scala/kafka/server/KafkaServer.scala > c208f83bed7fb91f07fae42f2b66892e6d46fecc > core/src/test/scala/unit/kafka/admin/DeleteTopicTest.scala > 9c29e144bba2c9bafa91941b6ca5c263490693b3 > core/src/test/scala/unit/kafka/server/SimpleFetchTest.scala > b1c4ce9f66aa86c68f2e6988f67546fa63ed1f9b > > Diff: https://reviews.apache.org/r/20745/diff/ > > > Testing > ------- > > > Thanks, > > Timothy Chen > >
