> On April 28, 2014, 7:43 p.m., Neha Narkhede wrote: > > core/src/main/scala/kafka/controller/ControllerChannelManager.scala, line > > 210 > > <https://reviews.apache.org/r/20745/diff/1/?file=569160#file569160line210> > > > > 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?
Sorry forgot to add more comments to the rb, have been chatting mostly with Jun about my fixes. One test that was failing in delete topic test was a test that was handling request while topic being deleted, and it made sure it can never finish by shutting down one of the follower broker. Sending a fetch request to a topic being deleted returns a NotLeaderForPartition error code instead of expected UnknownTopicOrPartition. The cause of this is that as part of the metadata cache changes in Kafka api, we removed the check if a topic exists in the cache when serving fetching request and now it tries to read message set from the replica manager, and even though a stop replica request with delete partition is already been sent and processed, the partition was somehow still in replica manager and it returns NotLeaderForPartition exception since there is no leader for this topic. I found out that the partition was being recreated in the replica manager, because in the process of retrying deleting the same topic it change the failed replica state into OfflineReplica again that triggers a LeaderAndIsrRequest to all brokers for shrinking the Isr for that replica. The brokers that received the LeaderAndIsrRequest recreates the partition with getOrCreatePartition, and therefore future fetch requests hits it and throws that error code. I think the correct thing to do is to not send LeaderAndIsrRequest if the topic is being deleted when bring replica to an offline state, as it is not necessary anymore. > On April 28, 2014, 7:43 p.m., Neha Narkhede wrote: > > core/src/main/scala/kafka/controller/KafkaController.scala, line 652 > > <https://reviews.apache.org/r/20745/diff/1/?file=569161#file569161line652> > > > > 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. The latest fix for the deadlock decouples the await topic deletion with a new lock internal to delete topic manager, so that during the shutdown the await on the topic deletion condition and resume since it's not based on the controller lock anymore. We're still seeing occasional deadlock hanging problem, and with investigation Jun and I believe it's because if the topic deletion resumes and right before it acquires the controller lock, if a shutdown happens and acquired the lock, the delete topic thread will be waiting forever as the shutdown also blocks and waits. There are a variety of fixes, but I felt one of the cleaner fix is to shutdown the delete topic manager outside of the controller lock as it doesn't really require it anymore. Once delete topic thread shuts down then we can proceed holding the controller lock and finish the broker shutdown. > On April 28, 2014, 7:43 p.m., Neha Narkhede wrote: > > core/src/main/scala/kafka/controller/TopicDeletionManager.scala, line 403 > > <https://reviews.apache.org/r/20745/diff/1/?file=569163#file569163line403> > > > > * 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? There isn't really actual bug here, but I was trying to fix the comments and logic as if a topic was queued to be deleted and it's the first time calling delete topic thread, the current code and comment seems to suggest all topics that are queued that is not in completed has one failed replica. But it could be that it is just queued up to be deleted the first time. The effect is that in the logs you will see that the current code will log that it attempts to retry topic deletion again, while it's just being deleted the first time. I was trying to say that this is the first time the topic has been queued for deletion and not resumed to be completed or retried. I'll try to rewrite it. - Timothy ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/20745/#review41626 ----------------------------------------------------------- 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 > >
