Re: Review Request 17460: Patch for KAFKA-330

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/17460/ --- (Updated Feb. 8, 2014, 7:07 p.m.) Review request for kafka. Bugs: KAFKA-330

Re: Review Request 17460: Patch for KAFKA-330

> On Feb. 8, 2014, 2:43 a.m., Joel Koshy wrote: > > Already checked-in so this is really a follow-up review. > > > > My overall take on the implementation is that it is (perhaps - because I'm > > not 100 percent sure myself) complex mainly to handle corner cases which are > > rare but I think re

Re: Review Request 17460: Patch for KAFKA-330

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/17460/#review33961 --- Already checked-in so this is really a follow-up review. My overall

Re: Review Request 17460: Patch for KAFKA-330

> On Feb. 6, 2014, 11:48 p.m., Jun Rao wrote: > > core/src/main/scala/kafka/controller/PartitionStateMachine.scala, lines > > 457-458 > > > > > > | should be ||. > > Neha Narkhede wrote: > No, it shouldn't. This

Re: Review Request 17460: Patch for KAFKA-330

> On Feb. 6, 2014, 11:48 p.m., Jun Rao wrote: > > core/src/main/scala/kafka/controller/PartitionStateMachine.scala, lines > > 457-458 > > > > > > | should be ||. > > Neha Narkhede wrote: > No, it shouldn't. This

Re: Review Request 17460: Patch for KAFKA-330

> On Feb. 6, 2014, 11:48 p.m., Jun Rao wrote: > > core/src/main/scala/kafka/controller/PartitionStateMachine.scala, lines > > 94-96 > > > > > > The inner "if" is not properly indented. That is due to line wrapping

Re: Review Request 17460: Patch for KAFKA-330

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/17460/#review33858 --- Ship it! Some minor comments below. core/src/main/scala/kafka/con

Re: Review Request 17460: Patch for KAFKA-330

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/17460/ --- (Updated Feb. 6, 2014, 7:37 p.m.) Review request for kafka. Bugs: KAFKA-330

Re: Review Request 17460: Patch for KAFKA-330

> On Feb. 6, 2014, 7:15 p.m., Guozhang Wang wrote: > > core/src/main/scala/kafka/controller/TopicDeletionManager.scala, line 330 > > > > > > Shall we check if isTopicDeletionHalted(topic) here at the first place? > >

Re: Review Request 17460: Patch for KAFKA-330

> On Feb. 6, 2014, 7:15 p.m., Guozhang Wang wrote: > > core/src/main/scala/kafka/controller/TopicDeletionManager.scala, line 330 > > > > > > Shall we check if isTopicDeletionHalted(topic) here at the first place? No,

Re: Review Request 17460: Patch for KAFKA-330

> On Feb. 5, 2014, 10:23 p.m., Guozhang Wang wrote: > > In the follow-up patch that serialize all the admin tasks in the back > > ground thread, I would suggest switching away from using the callbacks to > > trigger state change while executing the process but depending on some ZK > > path cha

Re: Review Request 17460: Patch for KAFKA-330

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/17460/#review33813 --- core/src/main/scala/kafka/controller/KafkaController.scala

Re: Review Request 17460: Patch for KAFKA-330

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/17460/ --- (Updated Feb. 6, 2014, 6:29 p.m.) Review request for kafka. Bugs: KAFKA-330

Re: Review Request 17460: Patch for KAFKA-330

> On Feb. 5, 2014, 10:23 p.m., Guozhang Wang wrote: > > core/src/main/scala/kafka/controller/TopicDeletionManager.scala, line 131 > > > > > > It is an exception case if topicsTobeDeleted.!contains(topics). > > Neha Na

Re: Review Request 17460: Patch for KAFKA-330

> On Feb. 5, 2014, 10:23 p.m., Guozhang Wang wrote: > > core/src/main/scala/kafka/controller/ControllerChannelManager.scala, line > > 215 > > > > > > Instead of checking the replicaId == -1 case here, I feel it is bet

Re: Review Request 17460: Patch for KAFKA-330

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/17460/ --- (Updated Feb. 6, 2014, 5:42 p.m.) Review request for kafka. Bugs: KAFKA-330

Re: Review Request 17460: Patch for KAFKA-330

> On Feb. 5, 2014, 10:23 p.m., Guozhang Wang wrote: > > In the follow-up patch that serialize all the admin tasks in the back > > ground thread, I would suggest switching away from using the callbacks to > > trigger state change while executing the process but depending on some ZK > > path cha

Re: Review Request 17460: Patch for KAFKA-330

> On Feb. 5, 2014, 10:34 p.m., Jun Rao wrote: > > Some high level comments. > > > > 1. While most of the replica states are now managed in ReplicaStateMachine, > > there are a few still managed in TopicDeletionManager through > > haltedTopicsForDeletion and topicDeletionInProgress. It probably

Re: Review Request 17460: Patch for KAFKA-330

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/17460/ --- (Updated Feb. 6, 2014, 3:49 p.m.) Review request for kafka. Bugs: KAFKA-330

Re: Review Request 17460: Patch for KAFKA-330

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/17460/#review33699 --- Some high level comments. 1. While most of the replica states are n

Re: Review Request 17460: Patch for KAFKA-330

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/17460/#review33711 --- In the follow-up patch that serialize all the admin tasks in the bac

Re: Review Request 17460: Patch for KAFKA-330

> On Feb. 5, 2014, 2:50 a.m., Joel Koshy wrote: > > core/src/main/scala/kafka/controller/ControllerChannelManager.scala, line > > 304 > > > > > > You mean just put all in a single StopReplicaRequest? If so, any reason

Re: Review Request 17460: Patch for KAFKA-330

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/17460/ --- (Updated Feb. 5, 2014, 5:31 p.m.) Review request for kafka. Bugs: KAFKA-330

Re: Review Request 17460: Patch for KAFKA-330

> On Feb. 5, 2014, 2:50 a.m., Joel Koshy wrote: > > core/src/main/scala/kafka/controller/ControllerChannelManager.scala, line > > 220 > > > > > > Why not only limit to the topic-partitions relevant to this > > leader

Re: Review Request 17460: Patch for KAFKA-330

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/17460/#review33668 --- I haven't finished reviewing but will continue when I get time. I ha

Re: Review Request 17460: Patch for KAFKA-330

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/17460/ --- (Updated Feb. 1, 2014, 10:58 p.m.) Review request for kafka. Bugs: KAFKA-330

Re: Review Request 17460: Patch for KAFKA-330

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/17460/ --- (Updated Feb. 1, 2014, 1:45 a.m.) Review request for kafka. Bugs: KAFKA-330

Re: Review Request 17460: Patch for KAFKA-330

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

Re: Review Request 17460: Patch for KAFKA-330

> On Jan. 30, 2014, 1:33 a.m., Jun Rao wrote: > > A few high level comments: > > > > 1. Do we need to block adding partitions when a topic is being deleted? > > > > 2. StopReplica callback: When the callback is created, we can probably let > > it reference a local val for replica id. This way,

Re: Review Request 17460: Patch for KAFKA-330

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/17460/#review33179 --- A few high level comments: 1. Do we need to block adding partitions

Re: Review Request 17460: Patch for KAFKA-330

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/17460/ --- (Updated Jan. 29, 2014, 6:01 a.m.) Review request for kafka. Bugs: KAFKA-330

Re: Review Request 17460: Patch for KAFKA-330

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/17460/ --- (Updated Jan. 29, 2014, 1:38 a.m.) Review request for kafka. Bugs: KAFKA-330

Re: Review Request 17460: Patch for KAFKA-330

> On Jan. 28, 2014, 11:16 p.m., Guozhang Wang wrote: > > In all, I would vote for marking the deletion as failed when some brokers > > are in-available, but with this I still feel the delete-topic logic is > > pretty complicated interleaving with other operations. Since in most > > operational

Re: Review Request 17460: Patch for KAFKA-330

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/17460/ --- (Updated Jan. 28, 2014, 11:19 p.m.) Review request for kafka. Bugs: KAFKA-330

Re: Review Request 17460: Patch for KAFKA-330

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/17460/#review33056 --- In all, I would vote for marking the deletion as failed when some br

Re: Review Request 17460: Patch for KAFKA-330

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/17460/#review33050 --- core/src/main/scala/kafka/controller/KafkaController.scala

Re: Review Request 17460: Patch for KAFKA-330

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/17460/ --- (Updated Jan. 28, 2014, 9:25 p.m.) Review request for kafka. Bugs: KAFKA-330

Re: Review Request 17460: Patch for KAFKA-330

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/17460/#review33046 --- core/src/main/scala/kafka/controller/ControllerChannelManager.scala

Re: Review Request 17460: Patch for KAFKA-330

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/17460/ --- (Updated Jan. 28, 2014, 7:15 p.m.) Review request for kafka. Bugs: KAFKA-330

Re: Review Request 17460: Patch for KAFKA-330

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/17460/ --- (Updated Jan. 28, 2014, 7:13 p.m.) Review request for kafka. Bugs: KAFKA-330

Review Request 17460: Patch for KAFKA-330

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/17460/ --- Review request for kafka. Bugs: KAFKA-330 https://issues.apache.org/jira/br

Re: Review Request 17460: Patch for KAFKA-330

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/17460/ --- (Updated Jan. 28, 2014, 7:06 p.m.) Review request for kafka. Bugs: KAFKA-330