----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/21899/#review45127 -----------------------------------------------------------
core/src/main/scala/kafka/controller/KafkaController.scala <https://reviews.apache.org/r/21899/#comment79818> Do we need to override equals()? I thought case class will do the right thing by default? core/src/main/scala/kafka/utils/ReplicationUtils.scala <https://reviews.apache.org/r/21899/#comment79806> The logic in the check is not quite right. We should only accept the written value if the LeaderIsrAndControllerEpoch from written equals to that in expected, but ignoring the zkVersion. So, we need to set the zkVersion in newLeader and writtenLeader to be the same. We probably don't need to pass in zkVersion. core/src/main/scala/kafka/utils/ReplicationUtils.scala <https://reviews.apache.org/r/21899/#comment79807> We probably should move ZkUtil.getLeaderIsrAndEpochForPartition() to this file as well. Then parseLeaderAndIsr() can be made private. core/src/main/scala/kafka/utils/ZkUtils.scala <https://reviews.apache.org/r/21899/#comment79808> Let's add a comment on what optionalChecker is intended for. Also, should this parameter be optional since all callers set it? core/src/main/scala/kafka/utils/ZkUtils.scala <https://reviews.apache.org/r/21899/#comment79790> There is no need to enclose the code in {}. core/src/main/scala/kafka/utils/ZkUtils.scala <https://reviews.apache.org/r/21899/#comment79811> The two error logging should probably be warning. core/src/test/scala/unit/kafka/utils/ReplicationUtilsTest.scala <https://reviews.apache.org/r/21899/#comment79820> There are some unused imports. - Jun Rao On June 7, 2014, 4 p.m., Sriharsha Chintalapani wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/21899/ > ----------------------------------------------------------- > > (Updated June 7, 2014, 4 p.m.) > > > Review request for kafka. > > > Bugs: KAFKA-1382 > https://issues.apache.org/jira/browse/KAFKA-1382 > > > Repository: kafka > > > Description > ------- > > KAFKA-1382. Update zkVersion on partition state update failures. > > > KAFKA-1382. Update zkVersion on partition state update failures. > > > KAFKA-1382. Update zkVersion on partition state update failures. > > > KAFKA-1382. Update zkVersion on partition state update failures. added unit > tests for ReplicationUtils > > > KAFKA-1382. Update zkVersion on partition state update failures. added unit > tests for ReplicationUtils > > > KAFKA-1382. Update zkVersion on partition state update failures. added unit > tests for ReplicationUtils. > > > Diffs > ----- > > core/src/main/scala/kafka/cluster/Partition.scala > 518d2df5ae702d8c0937e1f9603fd11a54e24be8 > core/src/main/scala/kafka/controller/KafkaController.scala > e776423b8a38da6f08b2262c8141abf2064d37d2 > core/src/main/scala/kafka/controller/PartitionStateMachine.scala > 6457b56340a1b5440b07612f69dcffe4b051f919 > core/src/main/scala/kafka/utils/ReplicationUtils.scala PRE-CREATION > core/src/main/scala/kafka/utils/ZkUtils.scala > fcbe269b6057b45793ea95f357890d5d6922e8d4 > core/src/test/scala/unit/kafka/utils/ReplicationUtilsTest.scala > PRE-CREATION > > Diff: https://reviews.apache.org/r/21899/diff/ > > > Testing > ------- > > > Thanks, > > Sriharsha Chintalapani > >
