Re: Review Request 28027: Patch for KAFKA-1724

[Jun Rao <ju...@gmail.com>]

> On Nov. 14, 2014, 2:39 a.m., Jun Rao wrote:
> > Thanks for the patch. I am not sure if that's the right place to fix it though. The issue is that when KafkaController.onControllerResignation() is called, the controller wasn't actually active. A better fix is probably to guard this in onControllerResignation() and only go through the logic if the controller is active.
> 
> Sriharsha Chintalapani wrote:
>     Jun, Thanks for the review. When KafkaController.startup() calls it sets the isRunning to true before calling controllerElector.startup which calls elect and if the /controller from previous run still exists it sets leaderId and returns, meanwhile the /controller gets deleted triggering handleDataDelete which calls onControllerResgination(). As per these events KafkaController is started . I tried setting kafkaController.isRunning to false , not sure why its true as default and only do onControllerResignation if isRunning is true. This doesn't work because KafkaController.startup() makes it true before calling controllerElector.startup() which is going to trigger onControllerResignation().  
>     Regarding the patch I don't think Kafkascheduler should throw an exception when a shutdown is called.
>     On a side note onControllerResignation should be a wrapper around if (isRunning) and isRunning by default should be false.
>     Please let me know your thoughts.

Sorry for the late response. It seems that currently onControllerResignation() can be called even when the broker is not a controller (e.g. from SessionExpirationListener). So, we probably should just make the logic in onControllerResignation() more robust. I was thinking that before we shut down the KafkaScheduler, we can first check if it's started already (trunk already has such a method) and only shut down the scheduler if it's started. This is probably better than making the change at the scheduler level since in some other use cases, we do expect the shutdown call to only happen after startup.


- Jun


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28027/#review61391
-----------------------------------------------------------


On Nov. 14, 2014, 1:34 a.m., Sriharsha Chintalapani wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/28027/
> -----------------------------------------------------------
> 
> (Updated Nov. 14, 2014, 1:34 a.m.)
> 
> 
> Review request for kafka.
> 
> 
> Bugs: KAFKA-1724
>     https://issues.apache.org/jira/browse/KAFKA-1724
> 
> 
> Repository: kafka
> 
> 
> Description
> -------
> 
> KAFKA-1724. Errors after reboot in single node setup.
> 
> 
> Diffs
> -----
> 
>   core/src/main/scala/kafka/utils/KafkaScheduler.scala 9a16343d2ff7192b741f0b23a6bdf58d8f2bbd3e 
> 
> Diff: https://reviews.apache.org/r/28027/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Sriharsha Chintalapani
> 
>