> On Oct. 7, 2014, 12:15 a.m., Guozhang Wang wrote: > > Since now the first iteration of "if" statements is only used for logging, > > could we just merge it into the second check? > > Jiangjie Qin wrote: > Guozhang, thanks for the review. I actually thought about it before. I > agree that the code looks simpler if we just call partition.makeFollower > without checking whether leader is up or not. The reason I retained the first > if statement is that it seems more reasonable to create the remote replicas > only when the leader broker is online, which is done in > partition.makeFollower. And I'm not 100% sure whether it matters if we just > create the remote replicas objects without checking the liveliness of leader > broker. > > Guozhang Wang wrote: > The checking of the liveness of the leader is just for assertation (i.e. > the leader should always be alive when the leaderAndISR request is received, > otherwise something bad has happended and we need to log the errors, but > still proceed by skiping this request).
Just checked the code again. The reason we delay the process of adding fetchers is just to optimize the case when the broker is already shutting down. On the other hand, makeFollower will only return false if the newly assigned leader is the same as the current leader, and I do not remember why we need to check this. In fact, now I think the make follower logic should proceed even if the newly assigned leader is the same as the current leader. With that, and the fact that if leaders.find(_.id == newLeaderBrokerId) passed then leaders.find(_.id == partition.leaderReplicaIdOpt.get) would also pass, I think what we can do the following process: 1. partition.makeFollowers, and only log errors if it returns false (i.e. do not exclude the partition) 2. remove fetcher, truncate log, etc. 3. if broker is not shutting down AND the new leader is up, add fetchers. - Guozhang ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26373/#review55615 ----------------------------------------------------------- On Oct. 6, 2014, 5:06 p.m., Jiangjie Qin wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/26373/ > ----------------------------------------------------------- > > (Updated Oct. 6, 2014, 5:06 p.m.) > > > Review request for kafka. > > > Bugs: KAFKA-1647 > https://issues.apache.org/jira/browse/KAFKA-1647 > > > Repository: kafka > > > Description > ------- > > Fix for Kafka-1647. > > > Diffs > ----- > > core/src/main/scala/kafka/server/ReplicaManager.scala > 78b7514cc109547c562e635824684fad581af653 > > Diff: https://reviews.apache.org/r/26373/diff/ > > > Testing > ------- > > > Thanks, > > Jiangjie Qin > >
