> On April 29, 2014, 5:02 p.m., Jun Rao wrote: > > core/src/main/scala/kafka/controller/ControllerChannelManager.scala, lines > > 209-212 > > <https://reviews.apache.org/r/20745/diff/3/?file=569935#file569935line209> > > > > Is it better to do the check here or in the caller?
Hmm, at first glance not sure why it makes sense to propagate LeaderAndIsrRequest when a partition is being deleted, but moving it just to the delete topic path is probably the least impact. I think I'll move it to the caller for now. > On April 29, 2014, 5:02 p.m., Jun Rao wrote: > > core/src/main/scala/kafka/controller/TopicDeletionManager.scala, lines > > 385-386 > > <https://reviews.apache.org/r/20745/diff/3/?file=569938#file569938line385> > > > > Do we need to check here at all since the caller of doWork() already > > does the check? Good point, since isRunning must be set to false before it can shut down this is redudant. > On April 29, 2014, 5:02 p.m., Jun Rao wrote: > > core/src/main/scala/kafka/controller/TopicDeletionManager.scala, lines > > 412-413 > > <https://reviews.apache.org/r/20745/diff/3/?file=569938#file569938line412> > > > > Do we need to introduce isAnyReplicaInState? replicasInState() seems > > more general and we can just check the output to implement > > isAnyReplicaInState. I added this because the logic here only want to see if there is any replica that is in a specific state, and thought returning the whole list is a waste as I see internally it's constructing a new set. From profiling other Scala code I see that any set constructing is quite expensive as it's using immutable sets. It is more clean to only expose one API, but I thought the trade off might be worth it. More thoughts? > On April 29, 2014, 5:02 p.m., Jun Rao wrote: > > core/src/test/scala/unit/kafka/admin/DeleteTopicTest.scala, lines 86-91 > > <https://reviews.apache.org/r/20745/diff/3/?file=569942#file569942line86> > > > > 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. It's actually the reverse, where it tries to shutdown controller while the delete topic process is still in progress. Currently there is no way to gurantee that the delete topic can halt until some condition happens, so it might be not guranteed. - Timothy ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/20745/#review41714 ----------------------------------------------------------- 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 > >
