----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/21899/#review45246 -----------------------------------------------------------
core/src/main/scala/kafka/controller/KafkaController.scala <https://reviews.apache.org/r/21899/#comment79982> For our use case, we don't need to override the equal method. This is because we just want to make sure what's stored in ZK is exactly the same as that we want to write. So, it's probably more intuitive to use the default equal() and let the caller set the same zkVersion for comparison. core/src/main/scala/kafka/utils/ReplicationUtils.scala <https://reviews.apache.org/r/21899/#comment79977> All usage of conditionalUpdates are for updating the leaderAndIsr path. So, we probably should let all callers use updateIsr(), instead of directly calling conditionalUpdates. We can change the method name to sth more general, e.g., updateLeaderAndIsr. Also, change brokerId to leaderId. core/src/main/scala/kafka/utils/ReplicationUtils.scala <https://reviews.apache.org/r/21899/#comment79979> Could we change newLeaderData to sth like expectedLeaderAndIsrInfo? core/src/main/scala/kafka/utils/ZkUtils.scala <https://reviews.apache.org/r/21899/#comment79983> We can remove some unused imports now. core/src/main/scala/kafka/utils/ZkUtils.scala <https://reviews.apache.org/r/21899/#comment79980> For the comment, how about we change it to the following? When there is a ConnectionLossException during the conditional update, zkClient will retry the update and may fail since the previous update may have succeeded (but the stored zkVersion no longer matches the expected one). In this case, we will run the optionalChecker to further check if the write indeed have succeeded. core/src/test/scala/unit/kafka/utils/ReplicationUtilsTest.scala <https://reviews.apache.org/r/21899/#comment79984> If we use the default equal for leaderAndIsr, do we still need this test? - Jun Rao On June 10, 2014, 1:23 a.m., Sriharsha Chintalapani wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/21899/ > ----------------------------------------------------------- > > (Updated June 10, 2014, 1:23 a.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. > > > KAFKA-1382. Update zkVersion on partition state update failures. > > > 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/controller/ReplicaStateMachine.scala > 2f0f29d9b76d847700bb64d6d54515b6a926a253 > 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 > >
