----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/16360/#review30767 -----------------------------------------------------------
Thanks for the patch. Some comments. core/src/main/scala/kafka/cluster/Partition.scala <https://reviews.apache.org/r/16360/#comment58909> It's kind of weird that this method returns the leader broker. Returning a boolean seems more natural. We can find out the leader broker in the caller. core/src/main/scala/kafka/cluster/Partition.scala <https://reviews.apache.org/r/16360/#comment58908> Wouldn't using case match be more natural than dealing with null? core/src/main/scala/kafka/cluster/Partition.scala <https://reviews.apache.org/r/16360/#comment58907> Could we make the comment clearer? Basically, during controller failover, the controller always first sends the current partition state info before doing the leader election. So, it could happen that the leader broker is not present in the leaderAndIsrRequest. core/src/main/scala/kafka/server/ReplicaManager.scala <https://reviews.apache.org/r/16360/#comment58910> It seems that if we are shutting down this broker, we still want to remove existing fetchers. We just don't want to add new fetchers. core/src/main/scala/kafka/server/ReplicaManager.scala <https://reviews.apache.org/r/16360/#comment58911> Could we name this to sth like successfulPartitionAndOffsets? core/src/main/scala/kafka/server/ReplicaManager.scala <https://reviews.apache.org/r/16360/#comment58912> Is it better to just log the failed partition in partition.makeFollower(), instead of here? This way, we probably log less. - Jun Rao On Dec. 19, 2013, 3:01 a.m., Guozhang Wang wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/16360/ > ----------------------------------------------------------- > > (Updated Dec. 19, 2013, 3:01 a.m.) > > > Review request for kafka. > > > Bugs: KAFKA-1188 > https://issues.apache.org/jira/browse/KAFKA-1188 > > > Repository: kafka > > > Description > ------- > > KAFKA-1188.v1 > > > Diffs > ----- > > core/src/main/scala/kafka/cluster/Partition.scala > 5c9307d71641ccc6c09a54b69d5aa2b4bc2a4cde > core/src/main/scala/kafka/server/ReplicaManager.scala > 242c18d47828b7c5e6b1fc219a0f1199fb1f9512 > > Diff: https://reviews.apache.org/r/16360/diff/ > > > Testing > ------- > > > Thanks, > > Guozhang Wang > >
