Re: Review Request 21899: Patch for KAFKA-1382

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/21899/#review46183 --- Ship it! Ship It! - Neha Narkhede On June 16, 2014, 9:19 p.m., S

Re: Review Request 21899: Patch for KAFKA-1382

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/21899/ --- (Updated June 16, 2014, 9:19 p.m.) Review request for kafka. Bugs: KAFKA-1382

Re: Review Request 21899: Patch for KAFKA-1382

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/21899/ --- (Updated June 16, 2014, 8:50 p.m.) Review request for kafka. Bugs: KAFKA-1382

Re: Review Request 21899: Patch for KAFKA-1382

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/21899/#review45499 --- Thanks for the patch. Looks good to me. Just some minor comments bel

Re: Review Request 21899: Patch for KAFKA-1382

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/21899/ --- (Updated June 11, 2014, 4:37 p.m.) Review request for kafka. Bugs: KAFKA-1382

Re: Review Request 21899: Patch for KAFKA-1382

--- 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

Re: Review Request 21899: Patch for KAFKA-1382

--- 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

Re: Review Request 21899: Patch for KAFKA-1382

> On June 9, 2014, 9:33 p.m., Jun Rao wrote: > > core/src/main/scala/kafka/utils/ZkUtils.scala, lines 384-385 > > > > > > Let's add a comment on what optionalChecker is intended for. Also, > > should this parameter be

Re: Review Request 21899: Patch for KAFKA-1382

> On June 9, 2014, 9:33 p.m., Jun Rao wrote: > > core/src/main/scala/kafka/controller/KafkaController.scala, lines 1336-1346 > > > > > > Do we need to override equals()? I thought case class will do the right > > thi

Re: Review Request 21899: Patch for KAFKA-1382

> On June 9, 2014, 9:33 p.m., Jun Rao wrote: > > core/src/main/scala/kafka/utils/ReplicationUtils.scala, lines 41-64 > > > > > > The logic in the check is not quite right. We should only accept the > > written value if

Re: Review Request 21899: Patch for KAFKA-1382

--- 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

Re: Review Request 21899: Patch for KAFKA-1382

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/21899/#review45132 --- Ship it! Ship It! - Neha Narkhede On June 7, 2014, 4 p.m., Sriha

Re: Review Request 21899: Patch for KAFKA-1382

--- 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

Re: Review Request 21899: Patch for KAFKA-1382

> On June 5, 2014, 5:30 a.m., Neha Narkhede wrote: > > This looks great overall. However, something that I missed on my last > > review that I think is important - could we add unit tests using the > > checker for conditionalUpdatePersistentPath? I understand that reproducing > > the actual pr

Re: Review Request 21899: Patch for KAFKA-1382

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/21899/#review44799 --- This looks great overall. However, something that I missed on my las

Re: Review Request 21899: Patch for KAFKA-1382

> On June 2, 2014, 9:01 p.m., Neha Narkhede wrote: > > core/src/main/scala/kafka/utils/ReplicationUtils.scala, line 46 > > > > > > Updating the zookeeper path if the contents are equal even if the > > version does not

Re: Review Request 21899: Patch for KAFKA-1382

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/21899/ --- (Updated June 4, 2014, 7:30 p.m.) Review request for kafka. Bugs: KAFKA-1382

Re: Review Request 21899: Patch for KAFKA-1382

> On June 2, 2014, 9:01 p.m., Neha Narkhede wrote: > > core/src/main/scala/kafka/utils/ReplicationUtils.scala, line 57 > > > > > > parseLeaderAndIsr can also be part of ReplicationUtils. > > > > > > Sriharsha Chin

Re: Review Request 21899: Patch for KAFKA-1382

> On June 2, 2014, 9:01 p.m., Neha Narkhede wrote: > > core/src/main/scala/kafka/utils/ReplicationUtils.scala, line 57 > > > > > > parseLeaderAndIsr can also be part of ReplicationUtils. > > > > parseLeaderAndIsr

Re: Review Request 21899: Patch for KAFKA-1382

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/21899/#review44544 --- core/src/main/scala/kafka/utils/ReplicationUtils.scala

Re: Review Request 21899: Patch for KAFKA-1382

> On May 27, 2014, 9:10 p.m., Neha Narkhede wrote: > > core/src/main/scala/kafka/utils/ZkUtils.scala, line 390 > > > > > > This is a generic API for conditional updates. Logic specific to Kafka > > shouldn't go here.

Re: Review Request 21899: Patch for KAFKA-1382

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/21899/ --- (Updated May 31, 2014, 10:50 p.m.) Review request for kafka. Bugs: KAFKA-1382

Re: Review Request 21899: Patch for KAFKA-1382

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/21899/ --- (Updated May 31, 2014, 4:19 a.m.) Review request for kafka. Bugs: KAFKA-1382

Re: Review Request 21899: Patch for KAFKA-1382

> On May 27, 2014, 9:10 p.m., Neha Narkhede wrote: > > core/src/main/scala/kafka/utils/ZkUtils.scala, line 390 > > > > > > This is a generic API for conditional updates. Logic specific to Kafka > > shouldn't go here.

Re: Review Request 21899: Patch for KAFKA-1382

> On May 27, 2014, 9:10 p.m., Neha Narkhede wrote: > > core/src/main/scala/kafka/utils/ZkUtils.scala, line 390 > > > > > > This is a generic API for conditional updates. Logic specific to Kafka > > shouldn't go here.

Re: Review Request 21899: Patch for KAFKA-1382

> On May 27, 2014, 9:10 p.m., Neha Narkhede wrote: > > core/src/main/scala/kafka/utils/ZkUtils.scala, line 423 > > > > > > ZkUtils.parseLeaderAndIsr does this. I was trying to get LeaderIsrAndControllerEpoch for passe

Re: Review Request 21899: Patch for KAFKA-1382

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/21899/#review44043 --- core/src/main/scala/kafka/utils/ZkUtils.scala