----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/20745/#review41714 -----------------------------------------------------------
Some comments: 1. Should we restore the commented out test in ServerShutdownTest.scala? core/src/main/scala/kafka/controller/ControllerChannelManager.scala <https://reviews.apache.org/r/20745/#comment75277> Is it better to do the check here or in the caller? core/src/main/scala/kafka/controller/TopicDeletionManager.scala <https://reviews.apache.org/r/20745/#comment75278> The shutdown sequence is a bit complicated now. Could you add some comments to describe the sequence of actions? core/src/main/scala/kafka/controller/TopicDeletionManager.scala <https://reviews.apache.org/r/20745/#comment75282> Do we need to check here at all since the caller of doWork() already does the check? core/src/main/scala/kafka/controller/TopicDeletionManager.scala <https://reviews.apache.org/r/20745/#comment75283> Do we need to introduce isAnyReplicaInState? replicasInState() seems more general and we can just check the output to implement isAnyReplicaInState. core/src/main/scala/kafka/server/KafkaApis.scala <https://reviews.apache.org/r/20745/#comment75284> It's better to do this in the ReplicaManager when handling the StopReplicaRequest. Then we don't have to expose LogManager to KafkaApis. core/src/test/scala/unit/kafka/admin/DeleteTopicTest.scala <https://reviews.apache.org/r/20745/#comment75285> There seems to be no guarantee that the delete topic process is completed before the controller was shutdown. So, I am not sure how reliable the test is. core/src/test/scala/unit/kafka/admin/DeleteTopicTest.scala <https://reviews.apache.org/r/20745/#comment75286> The comment is wrong. We are shutting down the leader, not the controller. core/src/test/scala/unit/kafka/admin/DeleteTopicTest.scala <https://reviews.apache.org/r/20745/#comment75287> Could we explicitly specify whether leaderIdOpt is for the old leader or the new leader in waitUntilLeaderIsElectedOrChanged()? core/src/test/scala/unit/kafka/admin/DeleteTopicTest.scala <https://reviews.apache.org/r/20745/#comment75289> I am not sure how reliable the test is since since by the time delete topic is started, the preferred leader election could have completed. core/src/test/scala/unit/kafka/admin/DeleteTopicTest.scala <https://reviews.apache.org/r/20745/#comment75291> Again, this test is not reliable. There are multiple possible outcomes. 1. Delete topic completes before partition reassignment is started. In this case partition reassignment should fail. Then, we shouldn't verify the reassigned replicas since the topic doesn't exist. 2. Reassignment is started/completed before the topic deletion completes. I am not sure which case this test is for. core/src/test/scala/unit/kafka/admin/DeleteTopicTest.scala <https://reviews.apache.org/r/20745/#comment75292> Same comment as the above. I am not sure how we guarantee whether partition reassignment or topic deletion completes first. core/src/test/scala/unit/kafka/admin/DeleteTopicTest.scala <https://reviews.apache.org/r/20745/#comment75296> Not sure if this adds any value than the previous send. - Jun Rao On April 29, 2014, 12:08 a.m., Timothy Chen wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/20745/ > ----------------------------------------------------------- > > (Updated April 29, 2014, 12:08 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 > >
