Re: Review Request 15711: Patch for KAFKA-930

[Jun Rao]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/15711/#review35281
---

Ship it!


Just the following minor comment.


core/src/main/scala/kafka/controller/KafkaController.scala


Just checking isTopicQueuedUpForDeletion() is enough. It will always return 
true until the topic deletion completes.


- Jun Rao


On Feb. 24, 2014, 9:59 a.m., Sriram Subramanian wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/15711/
> ---
> 
> (Updated Feb. 24, 2014, 9:59 a.m.)
> 
> 
> Review request for kafka.
> 
> 
> Bugs: KAFKA-930
> https://issues.apache.org/jira/browse/KAFKA-930
> 
> 
> Repository: kafka
> 
> 
> Description
> ---
> 
> Merge branch 'trunk' of http://git-wip-us.apache.org/repos/asf/kafka into 
> trunk
> 
> Conflicts:
>   core/src/main/scala/kafka/controller/KafkaController.scala
> 
> Merge branch 'trunk' of http://git-wip-us.apache.org/repos/asf/kafka into 
> trunk
> 
> 
> Merge branch 'trunk' of http://git-wip-us.apache.org/repos/asf/kafka into 
> trunk
> 
> Conflicts:
>   core/src/main/scala/kafka/controller/KafkaController.scala
> 
> Merge branch 'trunk' of http://git-wip-us.apache.org/repos/asf/kafka into 
> trunk
> 
> 
> some more changes
> 
> 
> use zk for auto rebalance
> 
> 
> Address code review feedbacks
> 
> 
> Merge branch 'trunk' of http://git-wip-us.apache.org/repos/asf/kafka into 
> trunk
> 
> 
> commit missing code
> 
> 
> some more changes
> 
> 
> fix merge conflicts
> 
> 
> Add auto leader rebalance support
> 
> 
> Merge branch 'trunk' of http://git-wip-us.apache.org/repos/asf/kafka into 
> trunk
> 
> 
> Merge branch 'trunk' of http://git-wip-us.apache.org/repos/asf/kafka into 
> trunk
> 
> Conflicts:
>   core/src/main/scala/kafka/admin/AdminUtils.scala
>   core/src/main/scala/kafka/admin/TopicCommand.scala
> 
> change comments
> 
> 
> commit the remaining changes
> 
> 
> Move AddPartitions into TopicCommand
> 
> 
> Diffs
> -
> 
>   core/src/main/scala/kafka/controller/KafkaController.scala 
> 00a1f9802474a688fe725db8d6bff493ede48684 
> 
> Diff: https://reviews.apache.org/r/15711/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Sriram Subramanian
> 
>

Re: Review Request 15711: Patch for KAFKA-930

[Sriram Subramanian]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/15711/
---

(Updated Feb. 24, 2014, 9:59 a.m.)


Review request for kafka.


Bugs: KAFKA-930
https://issues.apache.org/jira/browse/KAFKA-930


Repository: kafka


Description (updated)
---

Merge branch 'trunk' of http://git-wip-us.apache.org/repos/asf/kafka into trunk

Conflicts:
core/src/main/scala/kafka/controller/KafkaController.scala

Merge branch 'trunk' of http://git-wip-us.apache.org/repos/asf/kafka into trunk


Merge branch 'trunk' of http://git-wip-us.apache.org/repos/asf/kafka into trunk

Conflicts:
core/src/main/scala/kafka/controller/KafkaController.scala

Merge branch 'trunk' of http://git-wip-us.apache.org/repos/asf/kafka into trunk


some more changes


use zk for auto rebalance


Address code review feedbacks


Merge branch 'trunk' of http://git-wip-us.apache.org/repos/asf/kafka into trunk


commit missing code


some more changes


fix merge conflicts


Add auto leader rebalance support


Merge branch 'trunk' of http://git-wip-us.apache.org/repos/asf/kafka into trunk


Merge branch 'trunk' of http://git-wip-us.apache.org/repos/asf/kafka into trunk

Conflicts:
core/src/main/scala/kafka/admin/AdminUtils.scala
core/src/main/scala/kafka/admin/TopicCommand.scala

change comments


commit the remaining changes


Move AddPartitions into TopicCommand


Diffs (updated)
-

  core/src/main/scala/kafka/controller/KafkaController.scala 
00a1f9802474a688fe725db8d6bff493ede48684 

Diff: https://reviews.apache.org/r/15711/diff/


Testing
---


Thanks,

Sriram Subramanian

Re: Review Request 15711: Patch for KAFKA-930

[Neha Narkhede]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/15711/#review33124
---

Ship it!


I hope we can hold this off until delete topic which is currently in progress. 
That patch is much harder to rebase and there were errors while rebasing the 
last time. 

- Neha Narkhede


On Jan. 27, 2014, 9:28 p.m., Sriram Subramanian wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/15711/
> ---
> 
> (Updated Jan. 27, 2014, 9:28 p.m.)
> 
> 
> Review request for kafka.
> 
> 
> Bugs: KAFKA-930
> https://issues.apache.org/jira/browse/KAFKA-930
> 
> 
> Repository: kafka
> 
> 
> Description
> ---
> 
> Merge branch 'trunk' of http://git-wip-us.apache.org/repos/asf/kafka into 
> trunk
> 
> Conflicts:
>   core/src/main/scala/kafka/controller/KafkaController.scala
> 
> Merge branch 'trunk' of http://git-wip-us.apache.org/repos/asf/kafka into 
> trunk
> 
> 
> some more changes
> 
> 
> use zk for auto rebalance
> 
> 
> Address code review feedbacks
> 
> 
> Merge branch 'trunk' of http://git-wip-us.apache.org/repos/asf/kafka into 
> trunk
> 
> 
> commit missing code
> 
> 
> some more changes
> 
> 
> fix merge conflicts
> 
> 
> Add auto leader rebalance support
> 
> 
> Merge branch 'trunk' of http://git-wip-us.apache.org/repos/asf/kafka into 
> trunk
> 
> 
> Merge branch 'trunk' of http://git-wip-us.apache.org/repos/asf/kafka into 
> trunk
> 
> Conflicts:
>   core/src/main/scala/kafka/admin/AdminUtils.scala
>   core/src/main/scala/kafka/admin/TopicCommand.scala
> 
> change comments
> 
> 
> commit the remaining changes
> 
> 
> Move AddPartitions into TopicCommand
> 
> 
> Diffs
> -
> 
>   core/src/main/scala/kafka/controller/KafkaController.scala 
> a0267ae2670e8d5f365e49ec0fa5db1f62b815bf 
> 
> Diff: https://reviews.apache.org/r/15711/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Sriram Subramanian
> 
>

Re: Review Request 15711: Patch for KAFKA-930

[Jun Rao]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/15711/#review33121
---

Ship it!


Ship It!

- Jun Rao


On Jan. 27, 2014, 9:28 p.m., Sriram Subramanian wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/15711/
> ---
> 
> (Updated Jan. 27, 2014, 9:28 p.m.)
> 
> 
> Review request for kafka.
> 
> 
> Bugs: KAFKA-930
> https://issues.apache.org/jira/browse/KAFKA-930
> 
> 
> Repository: kafka
> 
> 
> Description
> ---
> 
> Merge branch 'trunk' of http://git-wip-us.apache.org/repos/asf/kafka into 
> trunk
> 
> Conflicts:
>   core/src/main/scala/kafka/controller/KafkaController.scala
> 
> Merge branch 'trunk' of http://git-wip-us.apache.org/repos/asf/kafka into 
> trunk
> 
> 
> some more changes
> 
> 
> use zk for auto rebalance
> 
> 
> Address code review feedbacks
> 
> 
> Merge branch 'trunk' of http://git-wip-us.apache.org/repos/asf/kafka into 
> trunk
> 
> 
> commit missing code
> 
> 
> some more changes
> 
> 
> fix merge conflicts
> 
> 
> Add auto leader rebalance support
> 
> 
> Merge branch 'trunk' of http://git-wip-us.apache.org/repos/asf/kafka into 
> trunk
> 
> 
> Merge branch 'trunk' of http://git-wip-us.apache.org/repos/asf/kafka into 
> trunk
> 
> Conflicts:
>   core/src/main/scala/kafka/admin/AdminUtils.scala
>   core/src/main/scala/kafka/admin/TopicCommand.scala
> 
> change comments
> 
> 
> commit the remaining changes
> 
> 
> Move AddPartitions into TopicCommand
> 
> 
> Diffs
> -
> 
>   core/src/main/scala/kafka/controller/KafkaController.scala 
> a0267ae2670e8d5f365e49ec0fa5db1f62b815bf 
> 
> Diff: https://reviews.apache.org/r/15711/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Sriram Subramanian
> 
>

Re: Review Request 15711: Patch for KAFKA-930

[Sriram Subramanian]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/15711/
---

(Updated Jan. 27, 2014, 9:28 p.m.)


Review request for kafka.


Bugs: KAFKA-930
https://issues.apache.org/jira/browse/KAFKA-930


Repository: kafka


Description (updated)
---

Merge branch 'trunk' of http://git-wip-us.apache.org/repos/asf/kafka into trunk

Conflicts:
core/src/main/scala/kafka/controller/KafkaController.scala

Merge branch 'trunk' of http://git-wip-us.apache.org/repos/asf/kafka into trunk


some more changes


use zk for auto rebalance


Address code review feedbacks


Merge branch 'trunk' of http://git-wip-us.apache.org/repos/asf/kafka into trunk


commit missing code


some more changes


fix merge conflicts


Add auto leader rebalance support


Merge branch 'trunk' of http://git-wip-us.apache.org/repos/asf/kafka into trunk


Merge branch 'trunk' of http://git-wip-us.apache.org/repos/asf/kafka into trunk

Conflicts:
core/src/main/scala/kafka/admin/AdminUtils.scala
core/src/main/scala/kafka/admin/TopicCommand.scala

change comments


commit the remaining changes


Move AddPartitions into TopicCommand


Diffs (updated)
-

  core/src/main/scala/kafka/controller/KafkaController.scala 
a0267ae2670e8d5f365e49ec0fa5db1f62b815bf 

Diff: https://reviews.apache.org/r/15711/diff/


Testing
---


Thanks,

Sriram Subramanian

Re: Review Request 15711: Patch for KAFKA-930

[Jun Rao]

> On Dec. 23, 2013, 7:54 p.m., Jun Rao wrote:
> > core/src/main/scala/kafka/controller/KafkaController.scala, lines 964-965
> > 
> >
> > I think we should use liveOrShuttingDownBrokerIds instead.

Actually, my mistake. liveBrokerIds is correct. Sorry.


- Jun


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/15711/#review30829
---


On Dec. 20, 2013, 7:23 p.m., Sriram Subramanian wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/15711/
> ---
> 
> (Updated Dec. 20, 2013, 7:23 p.m.)
> 
> 
> Review request for kafka.
> 
> 
> Bugs: KAFKA-930
> https://issues.apache.org/jira/browse/KAFKA-930
> 
> 
> Repository: kafka
> 
> 
> Description
> ---
> 
> some more changes
> 
> 
> use zk for auto rebalance
> 
> 
> Address code review feedbacks
> 
> 
> Merge branch 'trunk' of http://git-wip-us.apache.org/repos/asf/kafka into 
> trunk
> 
> 
> commit missing code
> 
> 
> some more changes
> 
> 
> fix merge conflicts
> 
> 
> Add auto leader rebalance support
> 
> 
> Merge branch 'trunk' of http://git-wip-us.apache.org/repos/asf/kafka into 
> trunk
> 
> 
> Merge branch 'trunk' of http://git-wip-us.apache.org/repos/asf/kafka into 
> trunk
> 
> Conflicts:
>   core/src/main/scala/kafka/admin/AdminUtils.scala
>   core/src/main/scala/kafka/admin/TopicCommand.scala
> 
> change comments
> 
> 
> commit the remaining changes
> 
> 
> Move AddPartitions into TopicCommand
> 
> 
> Diffs
> -
> 
>   core/src/main/scala/kafka/controller/KafkaController.scala 
> 4c319aba97655e7c4ec97fac2e34de4e28c9f5d3 
>   core/src/main/scala/kafka/server/KafkaConfig.scala 
> b324344d0a383398db8bfe2cbeec2c1378fe13c9 
> 
> Diff: https://reviews.apache.org/r/15711/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Sriram Subramanian
> 
>

Re: Review Request 15711: Patch for KAFKA-930

[Jun Rao]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/15711/#review30829
---



core/src/main/scala/kafka/controller/KafkaController.scala


Could we just write this as 
preferredREplicaForTopicsByBrokers: Map[Int, Map[TopicAndPartition, 
Seq[Int]] =
 controllerContext.controllerLock synchronized {
  ...
}

Then, we can make preferredREplicaForTopicsByBrokers a val.



core/src/main/scala/kafka/controller/KafkaController.scala


Ditto for topicsNotInPreferredReplica.

It seems that the last 5 statements don't need to be inside the controller 
lock.



core/src/main/scala/kafka/controller/KafkaController.scala


I think we should use liveOrShuttingDownBrokerIds instead.



core/src/main/scala/kafka/controller/KafkaController.scala


Not sure if it's better to do the preferred partition election through ZK 
watcher. The issue is that the controller will hold the lock while doing the 
preferred replica election for multiple partitions, which could take time. This 
means if there is another more important watcher fired (e.g., a broker 
failure), the controller won't be able to handle it immediately.

If we call onPreferredReplica() directly one partition at a time in the 
scheduler, the controller will get a chance to process other watchers.

Another way is to create the preferred replica path in ZK one partition at 
a time. However, this is probably too much overhead.


- Jun Rao


On Dec. 20, 2013, 7:23 p.m., Sriram Subramanian wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/15711/
> ---
> 
> (Updated Dec. 20, 2013, 7:23 p.m.)
> 
> 
> Review request for kafka.
> 
> 
> Bugs: KAFKA-930
> https://issues.apache.org/jira/browse/KAFKA-930
> 
> 
> Repository: kafka
> 
> 
> Description
> ---
> 
> some more changes
> 
> 
> use zk for auto rebalance
> 
> 
> Address code review feedbacks
> 
> 
> Merge branch 'trunk' of http://git-wip-us.apache.org/repos/asf/kafka into 
> trunk
> 
> 
> commit missing code
> 
> 
> some more changes
> 
> 
> fix merge conflicts
> 
> 
> Add auto leader rebalance support
> 
> 
> Merge branch 'trunk' of http://git-wip-us.apache.org/repos/asf/kafka into 
> trunk
> 
> 
> Merge branch 'trunk' of http://git-wip-us.apache.org/repos/asf/kafka into 
> trunk
> 
> Conflicts:
>   core/src/main/scala/kafka/admin/AdminUtils.scala
>   core/src/main/scala/kafka/admin/TopicCommand.scala
> 
> change comments
> 
> 
> commit the remaining changes
> 
> 
> Move AddPartitions into TopicCommand
> 
> 
> Diffs
> -
> 
>   core/src/main/scala/kafka/controller/KafkaController.scala 
> 4c319aba97655e7c4ec97fac2e34de4e28c9f5d3 
>   core/src/main/scala/kafka/server/KafkaConfig.scala 
> b324344d0a383398db8bfe2cbeec2c1378fe13c9 
> 
> Diff: https://reviews.apache.org/r/15711/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Sriram Subramanian
> 
>

Re: Review Request 15711: Patch for KAFKA-930

[Neha Narkhede]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/15711/#review30761
---

Ship it!


Ship It!

- Neha Narkhede


On Dec. 20, 2013, 7:23 p.m., Sriram Subramanian wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/15711/
> ---
> 
> (Updated Dec. 20, 2013, 7:23 p.m.)
> 
> 
> Review request for kafka.
> 
> 
> Bugs: KAFKA-930
> https://issues.apache.org/jira/browse/KAFKA-930
> 
> 
> Repository: kafka
> 
> 
> Description
> ---
> 
> some more changes
> 
> 
> use zk for auto rebalance
> 
> 
> Address code review feedbacks
> 
> 
> Merge branch 'trunk' of http://git-wip-us.apache.org/repos/asf/kafka into 
> trunk
> 
> 
> commit missing code
> 
> 
> some more changes
> 
> 
> fix merge conflicts
> 
> 
> Add auto leader rebalance support
> 
> 
> Merge branch 'trunk' of http://git-wip-us.apache.org/repos/asf/kafka into 
> trunk
> 
> 
> Merge branch 'trunk' of http://git-wip-us.apache.org/repos/asf/kafka into 
> trunk
> 
> Conflicts:
>   core/src/main/scala/kafka/admin/AdminUtils.scala
>   core/src/main/scala/kafka/admin/TopicCommand.scala
> 
> change comments
> 
> 
> commit the remaining changes
> 
> 
> Move AddPartitions into TopicCommand
> 
> 
> Diffs
> -
> 
>   core/src/main/scala/kafka/controller/KafkaController.scala 
> 4c319aba97655e7c4ec97fac2e34de4e28c9f5d3 
>   core/src/main/scala/kafka/server/KafkaConfig.scala 
> b324344d0a383398db8bfe2cbeec2c1378fe13c9 
> 
> Diff: https://reviews.apache.org/r/15711/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Sriram Subramanian
> 
>

Re: Review Request 15711: Patch for KAFKA-930

[Sriram Subramanian]

> On Dec. 12, 2013, 12:17 a.m., Neha Narkhede wrote:
> > core/src/main/scala/kafka/controller/KafkaController.scala, line 947
> > 
> >
> > If a massive admin-triggered preferred replica election is in progress, 
> > this callback might wait on the controller lock. If it waits until the next 
> > time the callback has to be triggered, the thread pool will wait to get the 
> > next task as there is no available thread to execute it (as the thread pool 
> > size is 1). I wonder if we should just return from the callback if a 
> > preferred replica election is in progress?
> 
> Neha Narkhede wrote:
> Sriram, Is the fix for this issue included in Diff 8?

Yes. Before updating zk path we check that no rebalance is in progress.


- Sriram


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/15711/#review30236
---


On Dec. 20, 2013, 7:23 p.m., Sriram Subramanian wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/15711/
> ---
> 
> (Updated Dec. 20, 2013, 7:23 p.m.)
> 
> 
> Review request for kafka.
> 
> 
> Bugs: KAFKA-930
> https://issues.apache.org/jira/browse/KAFKA-930
> 
> 
> Repository: kafka
> 
> 
> Description
> ---
> 
> some more changes
> 
> 
> use zk for auto rebalance
> 
> 
> Address code review feedbacks
> 
> 
> Merge branch 'trunk' of http://git-wip-us.apache.org/repos/asf/kafka into 
> trunk
> 
> 
> commit missing code
> 
> 
> some more changes
> 
> 
> fix merge conflicts
> 
> 
> Add auto leader rebalance support
> 
> 
> Merge branch 'trunk' of http://git-wip-us.apache.org/repos/asf/kafka into 
> trunk
> 
> 
> Merge branch 'trunk' of http://git-wip-us.apache.org/repos/asf/kafka into 
> trunk
> 
> Conflicts:
>   core/src/main/scala/kafka/admin/AdminUtils.scala
>   core/src/main/scala/kafka/admin/TopicCommand.scala
> 
> change comments
> 
> 
> commit the remaining changes
> 
> 
> Move AddPartitions into TopicCommand
> 
> 
> Diffs
> -
> 
>   core/src/main/scala/kafka/controller/KafkaController.scala 
> 4c319aba97655e7c4ec97fac2e34de4e28c9f5d3 
>   core/src/main/scala/kafka/server/KafkaConfig.scala 
> b324344d0a383398db8bfe2cbeec2c1378fe13c9 
> 
> Diff: https://reviews.apache.org/r/15711/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Sriram Subramanian
> 
>

Re: Review Request 15711: Patch for KAFKA-930

[Neha Narkhede]

> On Dec. 12, 2013, 12:17 a.m., Neha Narkhede wrote:
> > core/src/main/scala/kafka/controller/KafkaController.scala, line 947
> > 
> >
> > If a massive admin-triggered preferred replica election is in progress, 
> > this callback might wait on the controller lock. If it waits until the next 
> > time the callback has to be triggered, the thread pool will wait to get the 
> > next task as there is no available thread to execute it (as the thread pool 
> > size is 1). I wonder if we should just return from the callback if a 
> > preferred replica election is in progress?

Sriram, Is the fix for this issue included in Diff 8? 


- Neha


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/15711/#review30236
---


On Dec. 20, 2013, 7:23 p.m., Sriram Subramanian wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/15711/
> ---
> 
> (Updated Dec. 20, 2013, 7:23 p.m.)
> 
> 
> Review request for kafka.
> 
> 
> Bugs: KAFKA-930
> https://issues.apache.org/jira/browse/KAFKA-930
> 
> 
> Repository: kafka
> 
> 
> Description
> ---
> 
> some more changes
> 
> 
> use zk for auto rebalance
> 
> 
> Address code review feedbacks
> 
> 
> Merge branch 'trunk' of http://git-wip-us.apache.org/repos/asf/kafka into 
> trunk
> 
> 
> commit missing code
> 
> 
> some more changes
> 
> 
> fix merge conflicts
> 
> 
> Add auto leader rebalance support
> 
> 
> Merge branch 'trunk' of http://git-wip-us.apache.org/repos/asf/kafka into 
> trunk
> 
> 
> Merge branch 'trunk' of http://git-wip-us.apache.org/repos/asf/kafka into 
> trunk
> 
> Conflicts:
>   core/src/main/scala/kafka/admin/AdminUtils.scala
>   core/src/main/scala/kafka/admin/TopicCommand.scala
> 
> change comments
> 
> 
> commit the remaining changes
> 
> 
> Move AddPartitions into TopicCommand
> 
> 
> Diffs
> -
> 
>   core/src/main/scala/kafka/controller/KafkaController.scala 
> 4c319aba97655e7c4ec97fac2e34de4e28c9f5d3 
>   core/src/main/scala/kafka/server/KafkaConfig.scala 
> b324344d0a383398db8bfe2cbeec2c1378fe13c9 
> 
> Diff: https://reviews.apache.org/r/15711/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Sriram Subramanian
> 
>

Re: Review Request 15711: Patch for KAFKA-930

[Sriram Subramanian]

> On Dec. 12, 2013, 12:17 a.m., Neha Narkhede wrote:
> > core/src/main/scala/kafka/controller/KafkaController.scala, line 773
> > 
> >
> > Currently, all admin tools depend on state changes like preferred 
> > replica election/partition reassignment go through zookeeper. This helps 
> > the tools build a progress report. I think the ideal end state is still an 
> > admin RPC, but until we have that, it will be worth investigating if we can 
> > keep that property in this patch.

We now use zk to perform auto rebalance. If rebalance was triggered by the 
tool, we just fail auto rebalance and try the next time. If the tool was trying 
to rebalance when auto rebalance was in progress, we fail which is similar 
behavior to what it is today.


> On Dec. 12, 2013, 12:17 a.m., Neha Narkhede wrote:
> > core/src/main/scala/kafka/server/KafkaConfig.scala, line 234
> > 
> >
> > This is a config that we may not need in the future once we are sure 
> > that the zookeeper callbacks work fine in the controller. In that case, 
> > could we just depend on imbalance.check.interval.seconds to turn this 
> > feature on/off?

We have a delay config in scheduler that triggers the first rebalance. We need 
to add a config for that to make that a really large value which would be 
another config anyway. So I guess we would have to keep the enable config.


- Sriram


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/15711/#review30236
---


On Dec. 20, 2013, 7:23 p.m., Sriram Subramanian wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/15711/
> ---
> 
> (Updated Dec. 20, 2013, 7:23 p.m.)
> 
> 
> Review request for kafka.
> 
> 
> Bugs: KAFKA-930
> https://issues.apache.org/jira/browse/KAFKA-930
> 
> 
> Repository: kafka
> 
> 
> Description
> ---
> 
> some more changes
> 
> 
> use zk for auto rebalance
> 
> 
> Address code review feedbacks
> 
> 
> Merge branch 'trunk' of http://git-wip-us.apache.org/repos/asf/kafka into 
> trunk
> 
> 
> commit missing code
> 
> 
> some more changes
> 
> 
> fix merge conflicts
> 
> 
> Add auto leader rebalance support
> 
> 
> Merge branch 'trunk' of http://git-wip-us.apache.org/repos/asf/kafka into 
> trunk
> 
> 
> Merge branch 'trunk' of http://git-wip-us.apache.org/repos/asf/kafka into 
> trunk
> 
> Conflicts:
>   core/src/main/scala/kafka/admin/AdminUtils.scala
>   core/src/main/scala/kafka/admin/TopicCommand.scala
> 
> change comments
> 
> 
> commit the remaining changes
> 
> 
> Move AddPartitions into TopicCommand
> 
> 
> Diffs
> -
> 
>   core/src/main/scala/kafka/controller/KafkaController.scala 
> 4c319aba97655e7c4ec97fac2e34de4e28c9f5d3 
>   core/src/main/scala/kafka/server/KafkaConfig.scala 
> b324344d0a383398db8bfe2cbeec2c1378fe13c9 
> 
> Diff: https://reviews.apache.org/r/15711/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Sriram Subramanian
> 
>

Re: Review Request 15711: Patch for KAFKA-930

[Sriram Subramanian]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/15711/
---

(Updated Dec. 20, 2013, 7:23 p.m.)


Review request for kafka.


Bugs: KAFKA-930
https://issues.apache.org/jira/browse/KAFKA-930


Repository: kafka


Description (updated)
---

some more changes


use zk for auto rebalance


Address code review feedbacks


Merge branch 'trunk' of http://git-wip-us.apache.org/repos/asf/kafka into trunk


commit missing code


some more changes


fix merge conflicts


Add auto leader rebalance support


Merge branch 'trunk' of http://git-wip-us.apache.org/repos/asf/kafka into trunk


Merge branch 'trunk' of http://git-wip-us.apache.org/repos/asf/kafka into trunk

Conflicts:
core/src/main/scala/kafka/admin/AdminUtils.scala
core/src/main/scala/kafka/admin/TopicCommand.scala

change comments


commit the remaining changes


Move AddPartitions into TopicCommand


Diffs (updated)
-

  core/src/main/scala/kafka/controller/KafkaController.scala 
4c319aba97655e7c4ec97fac2e34de4e28c9f5d3 
  core/src/main/scala/kafka/server/KafkaConfig.scala 
b324344d0a383398db8bfe2cbeec2c1378fe13c9 

Diff: https://reviews.apache.org/r/15711/diff/


Testing
---


Thanks,

Sriram Subramanian

Re: Review Request 15711: Patch for KAFKA-930

[Sriram Subramanian]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/15711/
---

(Updated Dec. 20, 2013, 7:13 p.m.)


Review request for kafka.


Bugs: KAFKA-930
https://issues.apache.org/jira/browse/KAFKA-930


Repository: kafka


Description (updated)
---

use zk for auto rebalance


Address code review feedbacks


Merge branch 'trunk' of http://git-wip-us.apache.org/repos/asf/kafka into trunk


commit missing code


some more changes


fix merge conflicts


Add auto leader rebalance support


Merge branch 'trunk' of http://git-wip-us.apache.org/repos/asf/kafka into trunk


Merge branch 'trunk' of http://git-wip-us.apache.org/repos/asf/kafka into trunk

Conflicts:
core/src/main/scala/kafka/admin/AdminUtils.scala
core/src/main/scala/kafka/admin/TopicCommand.scala

change comments


commit the remaining changes


Move AddPartitions into TopicCommand


Diffs (updated)
-

  core/src/main/scala/kafka/controller/KafkaController.scala 
4c319aba97655e7c4ec97fac2e34de4e28c9f5d3 
  core/src/main/scala/kafka/server/KafkaConfig.scala 
b324344d0a383398db8bfe2cbeec2c1378fe13c9 

Diff: https://reviews.apache.org/r/15711/diff/


Testing
---


Thanks,

Sriram Subramanian

Re: Review Request 15711: Patch for KAFKA-930

[Neha Narkhede]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/15711/#review30236
---



core/src/main/scala/kafka/controller/KafkaController.scala


Currently, all admin tools depend on state changes like preferred replica 
election/partition reassignment go through zookeeper. This helps the tools 
build a progress report. I think the ideal end state is still an admin RPC, but 
until we have that, it will be worth investigating if we can keep that property 
in this patch.



core/src/main/scala/kafka/controller/KafkaController.scala


If a massive admin-triggered preferred replica election is in progress, 
this callback might wait on the controller lock. If it waits until the next 
time the callback has to be triggered, the thread pool will wait to get the 
next task as there is no available thread to execute it (as the thread pool 
size is 1). I wonder if we should just return from the callback if a preferred 
replica election is in progress?



core/src/main/scala/kafka/server/KafkaConfig.scala


This is a config that we may not need in the future once we are sure that 
the zookeeper callbacks work fine in the controller. In that case, could we 
just depend on imbalance.check.interval.seconds to turn this feature on/off?


As discussed offline - overall, looks good. Just have a few suggestions on this 
patch that we should think about. 

- Neha Narkhede


On Dec. 10, 2013, 6:52 a.m., Sriram Subramanian wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/15711/
> ---
> 
> (Updated Dec. 10, 2013, 6:52 a.m.)
> 
> 
> Review request for kafka.
> 
> 
> Bugs: KAFKA-930
> https://issues.apache.org/jira/browse/KAFKA-930
> 
> 
> Repository: kafka
> 
> 
> Description
> ---
> 
> Address code review feedbacks
> 
> 
> Merge branch 'trunk' of http://git-wip-us.apache.org/repos/asf/kafka into 
> trunk
> 
> 
> commit missing code
> 
> 
> some more changes
> 
> 
> fix merge conflicts
> 
> 
> Add auto leader rebalance support
> 
> 
> Merge branch 'trunk' of http://git-wip-us.apache.org/repos/asf/kafka into 
> trunk
> 
> 
> Merge branch 'trunk' of http://git-wip-us.apache.org/repos/asf/kafka into 
> trunk
> 
> Conflicts:
>   core/src/main/scala/kafka/admin/AdminUtils.scala
>   core/src/main/scala/kafka/admin/TopicCommand.scala
> 
> change comments
> 
> 
> commit the remaining changes
> 
> 
> Move AddPartitions into TopicCommand
> 
> 
> Diffs
> -
> 
>   core/src/main/scala/kafka/controller/KafkaController.scala 
> 4c319aba97655e7c4ec97fac2e34de4e28c9f5d3 
>   core/src/main/scala/kafka/server/KafkaConfig.scala 
> b324344d0a383398db8bfe2cbeec2c1378fe13c9 
> 
> Diff: https://reviews.apache.org/r/15711/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Sriram Subramanian
> 
>

Re: Review Request 15711: Patch for KAFKA-930

[Guozhang Wang]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/15711/#review30149
---

Ship it!



core/src/main/scala/kafka/controller/KafkaController.scala


We can delay the creation of autoRebalanceScheduler here, setting it as var 
and initialize to null first. Hence in shutdown we can do 
if(autoRebalanceScheduler != null) {shutdown and set to null}


- Guozhang Wang


On Dec. 10, 2013, 6:52 a.m., Sriram Subramanian wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/15711/
> ---
> 
> (Updated Dec. 10, 2013, 6:52 a.m.)
> 
> 
> Review request for kafka.
> 
> 
> Bugs: KAFKA-930
> https://issues.apache.org/jira/browse/KAFKA-930
> 
> 
> Repository: kafka
> 
> 
> Description
> ---
> 
> Address code review feedbacks
> 
> 
> Merge branch 'trunk' of http://git-wip-us.apache.org/repos/asf/kafka into 
> trunk
> 
> 
> commit missing code
> 
> 
> some more changes
> 
> 
> fix merge conflicts
> 
> 
> Add auto leader rebalance support
> 
> 
> Merge branch 'trunk' of http://git-wip-us.apache.org/repos/asf/kafka into 
> trunk
> 
> 
> Merge branch 'trunk' of http://git-wip-us.apache.org/repos/asf/kafka into 
> trunk
> 
> Conflicts:
>   core/src/main/scala/kafka/admin/AdminUtils.scala
>   core/src/main/scala/kafka/admin/TopicCommand.scala
> 
> change comments
> 
> 
> commit the remaining changes
> 
> 
> Move AddPartitions into TopicCommand
> 
> 
> Diffs
> -
> 
>   core/src/main/scala/kafka/controller/KafkaController.scala 
> 4c319aba97655e7c4ec97fac2e34de4e28c9f5d3 
>   core/src/main/scala/kafka/server/KafkaConfig.scala 
> b324344d0a383398db8bfe2cbeec2c1378fe13c9 
> 
> Diff: https://reviews.apache.org/r/15711/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Sriram Subramanian
> 
>

Re: Review Request 15711: Patch for KAFKA-930

[Sriram Subramanian]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/15711/
---

(Updated Dec. 10, 2013, 6:52 a.m.)


Review request for kafka.


Bugs: KAFKA-930
https://issues.apache.org/jira/browse/KAFKA-930


Repository: kafka


Description (updated)
---

Address code review feedbacks


Merge branch 'trunk' of http://git-wip-us.apache.org/repos/asf/kafka into trunk


commit missing code


some more changes


fix merge conflicts


Add auto leader rebalance support


Merge branch 'trunk' of http://git-wip-us.apache.org/repos/asf/kafka into trunk


Merge branch 'trunk' of http://git-wip-us.apache.org/repos/asf/kafka into trunk

Conflicts:
core/src/main/scala/kafka/admin/AdminUtils.scala
core/src/main/scala/kafka/admin/TopicCommand.scala

change comments


commit the remaining changes


Move AddPartitions into TopicCommand


Diffs (updated)
-

  core/src/main/scala/kafka/controller/KafkaController.scala 
4c319aba97655e7c4ec97fac2e34de4e28c9f5d3 
  core/src/main/scala/kafka/server/KafkaConfig.scala 
b324344d0a383398db8bfe2cbeec2c1378fe13c9 

Diff: https://reviews.apache.org/r/15711/diff/


Testing
---


Thanks,

Sriram Subramanian

Re: Review Request 15711: Patch for KAFKA-930

[Sriram Subramanian]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/15711/
---

(Updated Dec. 10, 2013, 6:51 a.m.)


Review request for kafka.


Bugs: KAFKA-930
https://issues.apache.org/jira/browse/KAFKA-930


Repository: kafka


Description
---

Merge branch 'trunk' of http://git-wip-us.apache.org/repos/asf/kafka into trunk


commit missing code


some more changes


fix merge conflicts


Add auto leader rebalance support


Merge branch 'trunk' of http://git-wip-us.apache.org/repos/asf/kafka into trunk


Merge branch 'trunk' of http://git-wip-us.apache.org/repos/asf/kafka into trunk

Conflicts:
core/src/main/scala/kafka/admin/AdminUtils.scala
core/src/main/scala/kafka/admin/TopicCommand.scala

change comments


commit the remaining changes


Move AddPartitions into TopicCommand


Diffs (updated)
-

  core/src/main/scala/kafka/controller/KafkaController.scala 
4c319aba97655e7c4ec97fac2e34de4e28c9f5d3 
  core/src/main/scala/kafka/server/KafkaConfig.scala 
b324344d0a383398db8bfe2cbeec2c1378fe13c9 

Diff: https://reviews.apache.org/r/15711/diff/


Testing
---


Thanks,

Sriram Subramanian

Re: Review Request 15711: Patch for KAFKA-930

[Jun Rao]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/15711/#review29816
---


It would also be useful to add a jmx for the count of partitions not on the 
preferred replicas.

- Jun Rao


On Nov. 21, 2013, 5:42 p.m., Sriram Subramanian wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/15711/
> ---
> 
> (Updated Nov. 21, 2013, 5:42 p.m.)
> 
> 
> Review request for kafka.
> 
> 
> Bugs: KAFKA-930
> https://issues.apache.org/jira/browse/KAFKA-930
> 
> 
> Repository: kafka
> 
> 
> Description
> ---
> 
> Merge branch 'trunk' of http://git-wip-us.apache.org/repos/asf/kafka into 
> trunk
> 
> 
> commit missing code
> 
> 
> some more changes
> 
> 
> fix merge conflicts
> 
> 
> Add auto leader rebalance support
> 
> 
> Merge branch 'trunk' of http://git-wip-us.apache.org/repos/asf/kafka into 
> trunk
> 
> 
> Merge branch 'trunk' of http://git-wip-us.apache.org/repos/asf/kafka into 
> trunk
> 
> Conflicts:
>   core/src/main/scala/kafka/admin/AdminUtils.scala
>   core/src/main/scala/kafka/admin/TopicCommand.scala
> 
> change comments
> 
> 
> commit the remaining changes
> 
> 
> Move AddPartitions into TopicCommand
> 
> 
> Diffs
> -
> 
>   core/src/main/scala/kafka/controller/KafkaController.scala 
> 4c319aba97655e7c4ec97fac2e34de4e28c9f5d3 
>   core/src/main/scala/kafka/server/KafkaConfig.scala 
> b324344d0a383398db8bfe2cbeec2c1378fe13c9 
> 
> Diff: https://reviews.apache.org/r/15711/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Sriram Subramanian
> 
>

Re: Review Request 15711: Patch for KAFKA-930

[Jun Rao]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/15711/#review29814
---


It would also be useful to add a jmx for the count of partitions not on the 
preferred replicas.

- Jun Rao


On Nov. 21, 2013, 5:42 p.m., Sriram Subramanian wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/15711/
> ---
> 
> (Updated Nov. 21, 2013, 5:42 p.m.)
> 
> 
> Review request for kafka.
> 
> 
> Bugs: KAFKA-930
> https://issues.apache.org/jira/browse/KAFKA-930
> 
> 
> Repository: kafka
> 
> 
> Description
> ---
> 
> Merge branch 'trunk' of http://git-wip-us.apache.org/repos/asf/kafka into 
> trunk
> 
> 
> commit missing code
> 
> 
> some more changes
> 
> 
> fix merge conflicts
> 
> 
> Add auto leader rebalance support
> 
> 
> Merge branch 'trunk' of http://git-wip-us.apache.org/repos/asf/kafka into 
> trunk
> 
> 
> Merge branch 'trunk' of http://git-wip-us.apache.org/repos/asf/kafka into 
> trunk
> 
> Conflicts:
>   core/src/main/scala/kafka/admin/AdminUtils.scala
>   core/src/main/scala/kafka/admin/TopicCommand.scala
> 
> change comments
> 
> 
> commit the remaining changes
> 
> 
> Move AddPartitions into TopicCommand
> 
> 
> Diffs
> -
> 
>   core/src/main/scala/kafka/controller/KafkaController.scala 
> 4c319aba97655e7c4ec97fac2e34de4e28c9f5d3 
>   core/src/main/scala/kafka/server/KafkaConfig.scala 
> b324344d0a383398db8bfe2cbeec2c1378fe13c9 
> 
> Diff: https://reviews.apache.org/r/15711/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Sriram Subramanian
> 
>

Re: Review Request 15711: Patch for KAFKA-930

[Neha Narkhede]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/15711/#review29606
---



core/src/main/scala/kafka/server/KafkaConfig.scala


can we disable this feature until 
https://issues.apache.org/jira/browse/KAFKA-1155 is solved?


- Neha Narkhede


On Nov. 21, 2013, 5:42 p.m., Sriram Subramanian wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/15711/
> ---
> 
> (Updated Nov. 21, 2013, 5:42 p.m.)
> 
> 
> Review request for kafka.
> 
> 
> Bugs: KAFKA-930
> https://issues.apache.org/jira/browse/KAFKA-930
> 
> 
> Repository: kafka
> 
> 
> Description
> ---
> 
> Merge branch 'trunk' of http://git-wip-us.apache.org/repos/asf/kafka into 
> trunk
> 
> 
> commit missing code
> 
> 
> some more changes
> 
> 
> fix merge conflicts
> 
> 
> Add auto leader rebalance support
> 
> 
> Merge branch 'trunk' of http://git-wip-us.apache.org/repos/asf/kafka into 
> trunk
> 
> 
> Merge branch 'trunk' of http://git-wip-us.apache.org/repos/asf/kafka into 
> trunk
> 
> Conflicts:
>   core/src/main/scala/kafka/admin/AdminUtils.scala
>   core/src/main/scala/kafka/admin/TopicCommand.scala
> 
> change comments
> 
> 
> commit the remaining changes
> 
> 
> Move AddPartitions into TopicCommand
> 
> 
> Diffs
> -
> 
>   core/src/main/scala/kafka/controller/KafkaController.scala 
> 4c319aba97655e7c4ec97fac2e34de4e28c9f5d3 
>   core/src/main/scala/kafka/server/KafkaConfig.scala 
> b324344d0a383398db8bfe2cbeec2c1378fe13c9 
> 
> Diff: https://reviews.apache.org/r/15711/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Sriram Subramanian
> 
>

Re: Review Request 15711: Patch for KAFKA-930

[Neha Narkhede]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/15711/#review29597
---



core/src/main/scala/kafka/controller/KafkaController.scala


instead of hardcoding this to 5 seconds, how about delaying it by 
leaderImbalanceCheckIntervalSeconds?



core/src/main/scala/kafka/controller/KafkaController.scala


this API is now a little awkward due to the updateZK parameter. Do we 
really need it? Another way is for the partition-rebalance-thread to always 
ensure creating the path and let this API delete it. This will keep the API 
clean.



core/src/main/scala/kafka/controller/KafkaController.scala


it seems we only need the preferred replica per partition, not the entire 
set of replicas right? In that case, we can simplify 
preferredReplicasForTopicsByBrokers to Map[Int, Map[TopicAndPartition, Int]] 
and call it preferredReplicaForPartitionsByBrokers



core/src/main/scala/kafka/controller/KafkaController.scala


It seems we don't need the brokerIds variable since it is never reused 
beyond the check in the if statement



core/src/main/scala/kafka/controller/KafkaController.scala


we also don't have semicolons as a coding convention. Difficult to switch 
between java and scala, eh? :)



core/src/main/scala/kafka/controller/KafkaController.scala


"trigger a leader rebalance for partitions that should have a leader on 
this broker" ?



core/src/main/scala/kafka/controller/KafkaController.scala


could we rename topicPartition to replicasPerPartition?



core/src/main/scala/kafka/server/KafkaConfig.scala


do we need this config option? It seems that the same could be achieved by 
setting a very high value for leader.imbalance.check.interval.seconds.


- Neha Narkhede


On Nov. 21, 2013, 5:42 p.m., Sriram Subramanian wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/15711/
> ---
> 
> (Updated Nov. 21, 2013, 5:42 p.m.)
> 
> 
> Review request for kafka.
> 
> 
> Bugs: KAFKA-930
> https://issues.apache.org/jira/browse/KAFKA-930
> 
> 
> Repository: kafka
> 
> 
> Description
> ---
> 
> Merge branch 'trunk' of http://git-wip-us.apache.org/repos/asf/kafka into 
> trunk
> 
> 
> commit missing code
> 
> 
> some more changes
> 
> 
> fix merge conflicts
> 
> 
> Add auto leader rebalance support
> 
> 
> Merge branch 'trunk' of http://git-wip-us.apache.org/repos/asf/kafka into 
> trunk
> 
> 
> Merge branch 'trunk' of http://git-wip-us.apache.org/repos/asf/kafka into 
> trunk
> 
> Conflicts:
>   core/src/main/scala/kafka/admin/AdminUtils.scala
>   core/src/main/scala/kafka/admin/TopicCommand.scala
> 
> change comments
> 
> 
> commit the remaining changes
> 
> 
> Move AddPartitions into TopicCommand
> 
> 
> Diffs
> -
> 
>   core/src/main/scala/kafka/controller/KafkaController.scala 
> 4c319aba97655e7c4ec97fac2e34de4e28c9f5d3 
>   core/src/main/scala/kafka/server/KafkaConfig.scala 
> b324344d0a383398db8bfe2cbeec2c1378fe13c9 
> 
> Diff: https://reviews.apache.org/r/15711/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Sriram Subramanian
> 
>

Re: Review Request 15711: Patch for KAFKA-930

[Jun Rao]

> On Nov. 20, 2013, 6:30 p.m., Jun Rao wrote:
> > core/src/main/scala/kafka/server/KafkaConfig.scala, lines 236-239
> > 
> >
> > I am wondering if this config is really necessary. Wouldn't it be 
> > simpler to always do the balancing on all partitions that are not already 
> > on the preferred replica?
> 
> Sriram Subramanian wrote:
> I do think there is value in this. To ensure rebalance happens always we 
> can set it to 0. There are cases where few topic partition movements does not 
> enforce a rebalance and hence cause unavailability. It is useful to have this 
> to operationalize this feature and understand its behavior in production.

I still don't see a strong reason why someone would want to leave some 
partitions' leader unbalanced. Even one unbalanced leader can cause significant 
extra load on the broker if the amount of data on that partition is large. The 
logic in KafkaController is also simplified if we always try to balance all 
leaders.


> On Nov. 20, 2013, 6:30 p.m., Jun Rao wrote:
> > core/src/main/scala/kafka/controller/KafkaController.scala, lines 745-746
> > 
> >
> > Could we rename updateZk to sth like isTriggeredByCommandLine?
> 
> Sriram Subramanian wrote:
> I dont like the external usage to leak into the code. I see your intent 
> to make the usage of this flag more explicit. How about 
> isTriggeredByAutoRebalance and not update zk if it is set?

This is fine. My only concern is that updateZK is a bit misleading. We do 
update the ISR path in ZK. We just don't update the leader balancing path.


- Jun


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/15711/#review29177
---


On Nov. 20, 2013, 1:38 a.m., Sriram Subramanian wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/15711/
> ---
> 
> (Updated Nov. 20, 2013, 1:38 a.m.)
> 
> 
> Review request for kafka.
> 
> 
> Bugs: KAFKA-930
> https://issues.apache.org/jira/browse/KAFKA-930
> 
> 
> Repository: kafka
> 
> 
> Description
> ---
> 
> commit missing code
> 
> 
> some more changes
> 
> 
> fix merge conflicts
> 
> 
> Add auto leader rebalance support
> 
> 
> Merge branch 'trunk' of http://git-wip-us.apache.org/repos/asf/kafka into 
> trunk
> 
> 
> Merge branch 'trunk' of http://git-wip-us.apache.org/repos/asf/kafka into 
> trunk
> 
> Conflicts:
>   core/src/main/scala/kafka/admin/AdminUtils.scala
>   core/src/main/scala/kafka/admin/TopicCommand.scala
> 
> change comments
> 
> 
> commit the remaining changes
> 
> 
> Move AddPartitions into TopicCommand
> 
> 
> Diffs
> -
> 
>   core/src/main/scala/kafka/controller/KafkaController.scala 
> 88792c2b2a360e928ab9cd00de151e5d5f94452d 
>   core/src/main/scala/kafka/server/KafkaConfig.scala 
> b324344d0a383398db8bfe2cbeec2c1378fe13c9 
> 
> Diff: https://reviews.apache.org/r/15711/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Sriram Subramanian
> 
>

Re: Review Request 15711: Patch for KAFKA-930

[Guozhang Wang]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/15711/#review29242
---



core/src/main/scala/kafka/controller/KafkaController.scala


One other way is that we can let the watcher handler call back function to 
not explicitly execute the election procedure but enqueue the request into this 
scheduler so that all replica election procedure will be done by this thread, 
and hence we can to batch election? Also this can help make the handling 
function very light so the chance of missing an event can be reduced.


- Guozhang Wang


On Nov. 21, 2013, 5:42 p.m., Sriram Subramanian wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/15711/
> ---
> 
> (Updated Nov. 21, 2013, 5:42 p.m.)
> 
> 
> Review request for kafka.
> 
> 
> Bugs: KAFKA-930
> https://issues.apache.org/jira/browse/KAFKA-930
> 
> 
> Repository: kafka
> 
> 
> Description
> ---
> 
> Merge branch 'trunk' of http://git-wip-us.apache.org/repos/asf/kafka into 
> trunk
> 
> 
> commit missing code
> 
> 
> some more changes
> 
> 
> fix merge conflicts
> 
> 
> Add auto leader rebalance support
> 
> 
> Merge branch 'trunk' of http://git-wip-us.apache.org/repos/asf/kafka into 
> trunk
> 
> 
> Merge branch 'trunk' of http://git-wip-us.apache.org/repos/asf/kafka into 
> trunk
> 
> Conflicts:
>   core/src/main/scala/kafka/admin/AdminUtils.scala
>   core/src/main/scala/kafka/admin/TopicCommand.scala
> 
> change comments
> 
> 
> commit the remaining changes
> 
> 
> Move AddPartitions into TopicCommand
> 
> 
> Diffs
> -
> 
>   core/src/main/scala/kafka/controller/KafkaController.scala 
> 4c319aba97655e7c4ec97fac2e34de4e28c9f5d3 
>   core/src/main/scala/kafka/server/KafkaConfig.scala 
> b324344d0a383398db8bfe2cbeec2c1378fe13c9 
> 
> Diff: https://reviews.apache.org/r/15711/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Sriram Subramanian
> 
>

Re: Review Request 15711: Patch for KAFKA-930

[Sriram Subramanian]

> On Nov. 21, 2013, 3:41 a.m., Swapnil Ghike wrote:
> > core/src/main/scala/kafka/controller/KafkaController.scala, lines 745-746
> > 
> >
> > Should we always delete the admin path? Because if auto rebalance 
> > achieved leader balance, then the manual rebalance has no work to do 
> > anyways.

This is for backwards compatibility.


> On Nov. 21, 2013, 3:41 a.m., Swapnil Ghike wrote:
> > core/src/main/scala/kafka/controller/KafkaController.scala, line 918
> > 
> >
> > rename to allReplicasForTopicPartitionsPerBroker? (I saw the per 
> > convention used somewhere else)

variables should be named based on the context. Here we refer to preferred 
replicas


> On Nov. 21, 2013, 3:41 a.m., Swapnil Ghike wrote:
> > core/src/main/scala/kafka/controller/KafkaController.scala, line 946
> > 
> >
> > we should be able to pass the entire set of partitions in one call, 
> > right?

see answer for Joel's comment


> On Nov. 21, 2013, 3:41 a.m., Swapnil Ghike wrote:
> > core/src/main/scala/kafka/server/KafkaConfig.scala, lines 236-239
> > 
> >
> > Will it be simpler to have a per cluster config instead of a per broker 
> > config? i cant think of any downsides.

per cluster config does not make much sense because it does not tell you about 
how well a broker needs to be balanced. balancing is per broker.


- Sriram


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/15711/#review29207
---


On Nov. 21, 2013, 5:42 p.m., Sriram Subramanian wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/15711/
> ---
> 
> (Updated Nov. 21, 2013, 5:42 p.m.)
> 
> 
> Review request for kafka.
> 
> 
> Bugs: KAFKA-930
> https://issues.apache.org/jira/browse/KAFKA-930
> 
> 
> Repository: kafka
> 
> 
> Description
> ---
> 
> Merge branch 'trunk' of http://git-wip-us.apache.org/repos/asf/kafka into 
> trunk
> 
> 
> commit missing code
> 
> 
> some more changes
> 
> 
> fix merge conflicts
> 
> 
> Add auto leader rebalance support
> 
> 
> Merge branch 'trunk' of http://git-wip-us.apache.org/repos/asf/kafka into 
> trunk
> 
> 
> Merge branch 'trunk' of http://git-wip-us.apache.org/repos/asf/kafka into 
> trunk
> 
> Conflicts:
>   core/src/main/scala/kafka/admin/AdminUtils.scala
>   core/src/main/scala/kafka/admin/TopicCommand.scala
> 
> change comments
> 
> 
> commit the remaining changes
> 
> 
> Move AddPartitions into TopicCommand
> 
> 
> Diffs
> -
> 
>   core/src/main/scala/kafka/controller/KafkaController.scala 
> 4c319aba97655e7c4ec97fac2e34de4e28c9f5d3 
>   core/src/main/scala/kafka/server/KafkaConfig.scala 
> b324344d0a383398db8bfe2cbeec2c1378fe13c9 
> 
> Diff: https://reviews.apache.org/r/15711/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Sriram Subramanian
> 
>

Re: Review Request 15711: Patch for KAFKA-930

[Sriram Subramanian]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/15711/
---

(Updated Nov. 21, 2013, 5:42 p.m.)


Review request for kafka.


Bugs: KAFKA-930
https://issues.apache.org/jira/browse/KAFKA-930


Repository: kafka


Description (updated)
---

Merge branch 'trunk' of http://git-wip-us.apache.org/repos/asf/kafka into trunk


commit missing code


some more changes


fix merge conflicts


Add auto leader rebalance support


Merge branch 'trunk' of http://git-wip-us.apache.org/repos/asf/kafka into trunk


Merge branch 'trunk' of http://git-wip-us.apache.org/repos/asf/kafka into trunk

Conflicts:
core/src/main/scala/kafka/admin/AdminUtils.scala
core/src/main/scala/kafka/admin/TopicCommand.scala

change comments


commit the remaining changes


Move AddPartitions into TopicCommand


Diffs (updated)
-

  core/src/main/scala/kafka/controller/KafkaController.scala 
4c319aba97655e7c4ec97fac2e34de4e28c9f5d3 
  core/src/main/scala/kafka/server/KafkaConfig.scala 
b324344d0a383398db8bfe2cbeec2c1378fe13c9 

Diff: https://reviews.apache.org/r/15711/diff/


Testing
---


Thanks,

Sriram Subramanian

Re: Review Request 15711: Patch for KAFKA-930

[Sriram Subramanian]

> On Nov. 21, 2013, 2:43 a.m., Joel Koshy wrote:
> > core/src/main/scala/kafka/controller/KafkaController.scala, line 944
> > 
> >
> > Any reason to not call onPreferredReplicaElection on the entire set of 
> > partitions (instead of one at a time). Doing it all at once within the 
> > controller context lock would also prevent a concurrent preferred replica 
> > election tool from proceeding into onPreferredReplicaElection (although if 
> > this feature is turned on you wouldn't need to use the command-line tool 
> > anyway).

We want to keep the locking fine grained to avoid zk related bug where a 
watcher trigger gets missed when the operation takes a long time.


- Sriram


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/15711/#review29203
---


On Nov. 21, 2013, 5:42 p.m., Sriram Subramanian wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/15711/
> ---
> 
> (Updated Nov. 21, 2013, 5:42 p.m.)
> 
> 
> Review request for kafka.
> 
> 
> Bugs: KAFKA-930
> https://issues.apache.org/jira/browse/KAFKA-930
> 
> 
> Repository: kafka
> 
> 
> Description
> ---
> 
> Merge branch 'trunk' of http://git-wip-us.apache.org/repos/asf/kafka into 
> trunk
> 
> 
> commit missing code
> 
> 
> some more changes
> 
> 
> fix merge conflicts
> 
> 
> Add auto leader rebalance support
> 
> 
> Merge branch 'trunk' of http://git-wip-us.apache.org/repos/asf/kafka into 
> trunk
> 
> 
> Merge branch 'trunk' of http://git-wip-us.apache.org/repos/asf/kafka into 
> trunk
> 
> Conflicts:
>   core/src/main/scala/kafka/admin/AdminUtils.scala
>   core/src/main/scala/kafka/admin/TopicCommand.scala
> 
> change comments
> 
> 
> commit the remaining changes
> 
> 
> Move AddPartitions into TopicCommand
> 
> 
> Diffs
> -
> 
>   core/src/main/scala/kafka/controller/KafkaController.scala 
> 4c319aba97655e7c4ec97fac2e34de4e28c9f5d3 
>   core/src/main/scala/kafka/server/KafkaConfig.scala 
> b324344d0a383398db8bfe2cbeec2c1378fe13c9 
> 
> Diff: https://reviews.apache.org/r/15711/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Sriram Subramanian
> 
>

Re: Review Request 15711: Patch for KAFKA-930

[Sriram Subramanian]

> On Nov. 20, 2013, 6:30 p.m., Jun Rao wrote:
> > core/src/main/scala/kafka/controller/KafkaController.scala, lines 745-746
> > 
> >
> > Could we rename updateZk to sth like isTriggeredByCommandLine?
> 
> Sriram Subramanian wrote:
> I dont like the external usage to leak into the code. I see your intent 
> to make the usage of this flag more explicit. How about 
> isTriggeredByAutoRebalance and not update zk if it is set?
> 
> Jun Rao wrote:
> This is fine. My only concern is that updateZK is a bit misleading. We do 
> update the ISR path in ZK. We just don't update the leader balancing path.

Changed it to isTriggeredByAutoRebalance


> On Nov. 20, 2013, 6:30 p.m., Jun Rao wrote:
> > core/src/main/scala/kafka/server/KafkaConfig.scala, lines 236-239
> > 
> >
> > I am wondering if this config is really necessary. Wouldn't it be 
> > simpler to always do the balancing on all partitions that are not already 
> > on the preferred replica?
> 
> Sriram Subramanian wrote:
> I do think there is value in this. To ensure rebalance happens always we 
> can set it to 0. There are cases where few topic partition movements does not 
> enforce a rebalance and hence cause unavailability. It is useful to have this 
> to operationalize this feature and understand its behavior in production.
> 
> Jun Rao wrote:
> I still don't see a strong reason why someone would want to leave some 
> partitions' leader unbalanced. Even one unbalanced leader can cause 
> significant extra load on the broker if the amount of data on that partition 
> is large. The logic in KafkaController is also simplified if we always try to 
> balance all leaders.

Our balancing of topics across topics is very primitive. We do not balance 
based on load or storage. What you suggest will happen even if the number of 
topics are balanced across the brokers. It is valuable to avoid unnecessary 
churn till we understand this feature better in production. I do see a benefit 
in not balancing when only a few topic partitions are imbalanced. Additionally 
the logic to do this is very minimum.


- Sriram


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/15711/#review29177
---


On Nov. 21, 2013, 5:42 p.m., Sriram Subramanian wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/15711/
> ---
> 
> (Updated Nov. 21, 2013, 5:42 p.m.)
> 
> 
> Review request for kafka.
> 
> 
> Bugs: KAFKA-930
> https://issues.apache.org/jira/browse/KAFKA-930
> 
> 
> Repository: kafka
> 
> 
> Description
> ---
> 
> Merge branch 'trunk' of http://git-wip-us.apache.org/repos/asf/kafka into 
> trunk
> 
> 
> commit missing code
> 
> 
> some more changes
> 
> 
> fix merge conflicts
> 
> 
> Add auto leader rebalance support
> 
> 
> Merge branch 'trunk' of http://git-wip-us.apache.org/repos/asf/kafka into 
> trunk
> 
> 
> Merge branch 'trunk' of http://git-wip-us.apache.org/repos/asf/kafka into 
> trunk
> 
> Conflicts:
>   core/src/main/scala/kafka/admin/AdminUtils.scala
>   core/src/main/scala/kafka/admin/TopicCommand.scala
> 
> change comments
> 
> 
> commit the remaining changes
> 
> 
> Move AddPartitions into TopicCommand
> 
> 
> Diffs
> -
> 
>   core/src/main/scala/kafka/controller/KafkaController.scala 
> 4c319aba97655e7c4ec97fac2e34de4e28c9f5d3 
>   core/src/main/scala/kafka/server/KafkaConfig.scala 
> b324344d0a383398db8bfe2cbeec2c1378fe13c9 
> 
> Diff: https://reviews.apache.org/r/15711/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Sriram Subramanian
> 
>

Re: Review Request 15711: Patch for KAFKA-930

[Sriram Subramanian]

> On Nov. 21, 2013, 3:41 a.m., Swapnil Ghike wrote:
> > core/src/main/scala/kafka/controller/KafkaController.scala, line 926
> > 
> >
> > rename to topicPartitionsNotLedByPreferredReplica?

PreferredReplica cannot lead multiple topic partitions.


- Sriram


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/15711/#review29207
---


On Nov. 21, 2013, 5:42 p.m., Sriram Subramanian wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/15711/
> ---
> 
> (Updated Nov. 21, 2013, 5:42 p.m.)
> 
> 
> Review request for kafka.
> 
> 
> Bugs: KAFKA-930
> https://issues.apache.org/jira/browse/KAFKA-930
> 
> 
> Repository: kafka
> 
> 
> Description
> ---
> 
> Merge branch 'trunk' of http://git-wip-us.apache.org/repos/asf/kafka into 
> trunk
> 
> 
> commit missing code
> 
> 
> some more changes
> 
> 
> fix merge conflicts
> 
> 
> Add auto leader rebalance support
> 
> 
> Merge branch 'trunk' of http://git-wip-us.apache.org/repos/asf/kafka into 
> trunk
> 
> 
> Merge branch 'trunk' of http://git-wip-us.apache.org/repos/asf/kafka into 
> trunk
> 
> Conflicts:
>   core/src/main/scala/kafka/admin/AdminUtils.scala
>   core/src/main/scala/kafka/admin/TopicCommand.scala
> 
> change comments
> 
> 
> commit the remaining changes
> 
> 
> Move AddPartitions into TopicCommand
> 
> 
> Diffs
> -
> 
>   core/src/main/scala/kafka/controller/KafkaController.scala 
> 4c319aba97655e7c4ec97fac2e34de4e28c9f5d3 
>   core/src/main/scala/kafka/server/KafkaConfig.scala 
> b324344d0a383398db8bfe2cbeec2c1378fe13c9 
> 
> Diff: https://reviews.apache.org/r/15711/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Sriram Subramanian
> 
>

Re: Review Request 15711: Patch for KAFKA-930

[Jun Rao]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/15711/#review29240
---



core/src/main/scala/kafka/controller/KafkaController.scala


Does this need to be at info level?



core/src/main/scala/kafka/controller/KafkaController.scala


Could we rewrite it as the following to make it clear?

groupBy { case(topicAndParttion, assignedReplicas) : => .. }



core/src/main/scala/kafka/controller/KafkaController.scala


Can we do the following so that we can use named variables?

foreach { case(leaderBroker, (topicAndPartition, assignedReplicas)) => ..





core/src/main/scala/kafka/controller/KafkaController.scala


Same here. Could we give item._1 a name so that it's clear what the 
referenced value is?



core/src/main/scala/kafka/controller/KafkaController.scala


Should this be info level logging?



core/src/main/scala/kafka/controller/KafkaController.scala


Same here. It would be better to give a referenced name for 
topicPartition._1.


- Jun Rao


On Nov. 21, 2013, 5:42 p.m., Sriram Subramanian wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/15711/
> ---
> 
> (Updated Nov. 21, 2013, 5:42 p.m.)
> 
> 
> Review request for kafka.
> 
> 
> Bugs: KAFKA-930
> https://issues.apache.org/jira/browse/KAFKA-930
> 
> 
> Repository: kafka
> 
> 
> Description
> ---
> 
> Merge branch 'trunk' of http://git-wip-us.apache.org/repos/asf/kafka into 
> trunk
> 
> 
> commit missing code
> 
> 
> some more changes
> 
> 
> fix merge conflicts
> 
> 
> Add auto leader rebalance support
> 
> 
> Merge branch 'trunk' of http://git-wip-us.apache.org/repos/asf/kafka into 
> trunk
> 
> 
> Merge branch 'trunk' of http://git-wip-us.apache.org/repos/asf/kafka into 
> trunk
> 
> Conflicts:
>   core/src/main/scala/kafka/admin/AdminUtils.scala
>   core/src/main/scala/kafka/admin/TopicCommand.scala
> 
> change comments
> 
> 
> commit the remaining changes
> 
> 
> Move AddPartitions into TopicCommand
> 
> 
> Diffs
> -
> 
>   core/src/main/scala/kafka/controller/KafkaController.scala 
> 4c319aba97655e7c4ec97fac2e34de4e28c9f5d3 
>   core/src/main/scala/kafka/server/KafkaConfig.scala 
> b324344d0a383398db8bfe2cbeec2c1378fe13c9 
> 
> Diff: https://reviews.apache.org/r/15711/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Sriram Subramanian
> 
>

Re: Review Request 15711: Patch for KAFKA-930

[Swapnil Ghike]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/15711/#review29207
---



core/src/main/scala/kafka/controller/KafkaController.scala


Should we always delete the admin path? Because if auto rebalance achieved 
leader balance, then the manual rebalance has no work to do anyways.



core/src/main/scala/kafka/controller/KafkaController.scala


rename to allReplicasForTopicPartitionsPerBroker? (I saw the per convention 
used somewhere else)



core/src/main/scala/kafka/controller/KafkaController.scala


rename to topicPartitionsNotLedByPreferredReplica?



core/src/main/scala/kafka/controller/KafkaController.scala


we should be able to pass the entire set of partitions in one call, right?



core/src/main/scala/kafka/server/KafkaConfig.scala


Will it be simpler to have a per cluster config instead of a per broker 
config? i cant think of any downsides.


- Swapnil Ghike


On Nov. 20, 2013, 1:38 a.m., Sriram Subramanian wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/15711/
> ---
> 
> (Updated Nov. 20, 2013, 1:38 a.m.)
> 
> 
> Review request for kafka.
> 
> 
> Bugs: KAFKA-930
> https://issues.apache.org/jira/browse/KAFKA-930
> 
> 
> Repository: kafka
> 
> 
> Description
> ---
> 
> commit missing code
> 
> 
> some more changes
> 
> 
> fix merge conflicts
> 
> 
> Add auto leader rebalance support
> 
> 
> Merge branch 'trunk' of http://git-wip-us.apache.org/repos/asf/kafka into 
> trunk
> 
> 
> Merge branch 'trunk' of http://git-wip-us.apache.org/repos/asf/kafka into 
> trunk
> 
> Conflicts:
>   core/src/main/scala/kafka/admin/AdminUtils.scala
>   core/src/main/scala/kafka/admin/TopicCommand.scala
> 
> change comments
> 
> 
> commit the remaining changes
> 
> 
> Move AddPartitions into TopicCommand
> 
> 
> Diffs
> -
> 
>   core/src/main/scala/kafka/controller/KafkaController.scala 
> 88792c2b2a360e928ab9cd00de151e5d5f94452d 
>   core/src/main/scala/kafka/server/KafkaConfig.scala 
> b324344d0a383398db8bfe2cbeec2c1378fe13c9 
> 
> Diff: https://reviews.apache.org/r/15711/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Sriram Subramanian
> 
>

Re: Review Request 15711: Patch for KAFKA-930

[Joel Koshy]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/15711/#review29203
---



core/src/main/scala/kafka/controller/KafkaController.scala


Another option is to _always_ try and delete the path - although if the 
znode doesn't exist it will give a spurious warning ("deleted during connection 
loss").



core/src/main/scala/kafka/controller/KafkaController.scala


Any reason to not call onPreferredReplicaElection on the entire set of 
partitions (instead of one at a time). Doing it all at once within the 
controller context lock would also prevent a concurrent preferred replica 
election tool from proceeding into onPreferredReplicaElection (although if this 
feature is turned on you wouldn't need to use the command-line tool anyway).


- Joel Koshy


On Nov. 20, 2013, 1:38 a.m., Sriram Subramanian wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/15711/
> ---
> 
> (Updated Nov. 20, 2013, 1:38 a.m.)
> 
> 
> Review request for kafka.
> 
> 
> Bugs: KAFKA-930
> https://issues.apache.org/jira/browse/KAFKA-930
> 
> 
> Repository: kafka
> 
> 
> Description
> ---
> 
> commit missing code
> 
> 
> some more changes
> 
> 
> fix merge conflicts
> 
> 
> Add auto leader rebalance support
> 
> 
> Merge branch 'trunk' of http://git-wip-us.apache.org/repos/asf/kafka into 
> trunk
> 
> 
> Merge branch 'trunk' of http://git-wip-us.apache.org/repos/asf/kafka into 
> trunk
> 
> Conflicts:
>   core/src/main/scala/kafka/admin/AdminUtils.scala
>   core/src/main/scala/kafka/admin/TopicCommand.scala
> 
> change comments
> 
> 
> commit the remaining changes
> 
> 
> Move AddPartitions into TopicCommand
> 
> 
> Diffs
> -
> 
>   core/src/main/scala/kafka/controller/KafkaController.scala 
> 88792c2b2a360e928ab9cd00de151e5d5f94452d 
>   core/src/main/scala/kafka/server/KafkaConfig.scala 
> b324344d0a383398db8bfe2cbeec2c1378fe13c9 
> 
> Diff: https://reviews.apache.org/r/15711/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Sriram Subramanian
> 
>

Re: Review Request 15711: Patch for KAFKA-930

[Sriram Subramanian]

> On Nov. 20, 2013, 6:30 p.m., Jun Rao wrote:
> > core/src/main/scala/kafka/controller/KafkaController.scala, lines 260-261
> > 
> >
> > Instead of delaying by 5 secs, should we just delay by 
> > config.leaderImbalanceCheckIntervalSeconds?

The problem is that leaderImbalanceCheckIntervalSeconds could be set to any 
value. If it was set to an hour or more than that and you have a controller 
failover because of an intermittent GC on the prev controller, and you want to 
immediately run the rebalnce, it will not happen. There are other cases where 
this is true. It is better to decouple the values. I can make it configurable 
but did not see a strong reason to do so.


> On Nov. 20, 2013, 6:30 p.m., Jun Rao wrote:
> > core/src/main/scala/kafka/controller/KafkaController.scala, lines 745-746
> > 
> >
> > Could we rename updateZk to sth like isTriggeredByCommandLine?

I dont like the external usage to leak into the code. I see your intent to make 
the usage of this flag more explicit. How about isTriggeredByAutoRebalance and 
not update zk if it is set?


> On Nov. 20, 2013, 6:30 p.m., Jun Rao wrote:
> > core/src/main/scala/kafka/server/KafkaConfig.scala, lines 236-239
> > 
> >
> > I am wondering if this config is really necessary. Wouldn't it be 
> > simpler to always do the balancing on all partitions that are not already 
> > on the preferred replica?

I do think there is value in this. To ensure rebalance happens always we can 
set it to 0. There are cases where few topic partition movements does not 
enforce a rebalance and hence cause unavailability. It is useful to have this 
to operationalize this feature and understand its behavior in production.


- Sriram


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/15711/#review29177
---


On Nov. 20, 2013, 1:38 a.m., Sriram Subramanian wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/15711/
> ---
> 
> (Updated Nov. 20, 2013, 1:38 a.m.)
> 
> 
> Review request for kafka.
> 
> 
> Bugs: KAFKA-930
> https://issues.apache.org/jira/browse/KAFKA-930
> 
> 
> Repository: kafka
> 
> 
> Description
> ---
> 
> commit missing code
> 
> 
> some more changes
> 
> 
> fix merge conflicts
> 
> 
> Add auto leader rebalance support
> 
> 
> Merge branch 'trunk' of http://git-wip-us.apache.org/repos/asf/kafka into 
> trunk
> 
> 
> Merge branch 'trunk' of http://git-wip-us.apache.org/repos/asf/kafka into 
> trunk
> 
> Conflicts:
>   core/src/main/scala/kafka/admin/AdminUtils.scala
>   core/src/main/scala/kafka/admin/TopicCommand.scala
> 
> change comments
> 
> 
> commit the remaining changes
> 
> 
> Move AddPartitions into TopicCommand
> 
> 
> Diffs
> -
> 
>   core/src/main/scala/kafka/controller/KafkaController.scala 
> 88792c2b2a360e928ab9cd00de151e5d5f94452d 
>   core/src/main/scala/kafka/server/KafkaConfig.scala 
> b324344d0a383398db8bfe2cbeec2c1378fe13c9 
> 
> Diff: https://reviews.apache.org/r/15711/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Sriram Subramanian
> 
>

Re: Review Request 15711: Patch for KAFKA-930

[Jun Rao]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/15711/#review29177
---


Thanks for the patch. A few comments.


core/src/main/scala/kafka/controller/KafkaController.scala


Instead of delaying by 5 secs, should we just delay by 
config.leaderImbalanceCheckIntervalSeconds?



core/src/main/scala/kafka/controller/KafkaController.scala


Could we rename updateZk to sth like isTriggeredByCommandLine?



core/src/main/scala/kafka/server/KafkaConfig.scala


I am wondering if this config is really necessary. Wouldn't it be simpler 
to always do the balancing on all partitions that are not already on the 
preferred replica?


- Jun Rao


On Nov. 20, 2013, 1:38 a.m., Sriram Subramanian wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/15711/
> ---
> 
> (Updated Nov. 20, 2013, 1:38 a.m.)
> 
> 
> Review request for kafka.
> 
> 
> Bugs: KAFKA-930
> https://issues.apache.org/jira/browse/KAFKA-930
> 
> 
> Repository: kafka
> 
> 
> Description
> ---
> 
> commit missing code
> 
> 
> some more changes
> 
> 
> fix merge conflicts
> 
> 
> Add auto leader rebalance support
> 
> 
> Merge branch 'trunk' of http://git-wip-us.apache.org/repos/asf/kafka into 
> trunk
> 
> 
> Merge branch 'trunk' of http://git-wip-us.apache.org/repos/asf/kafka into 
> trunk
> 
> Conflicts:
>   core/src/main/scala/kafka/admin/AdminUtils.scala
>   core/src/main/scala/kafka/admin/TopicCommand.scala
> 
> change comments
> 
> 
> commit the remaining changes
> 
> 
> Move AddPartitions into TopicCommand
> 
> 
> Diffs
> -
> 
>   core/src/main/scala/kafka/controller/KafkaController.scala 
> 88792c2b2a360e928ab9cd00de151e5d5f94452d 
>   core/src/main/scala/kafka/server/KafkaConfig.scala 
> b324344d0a383398db8bfe2cbeec2c1378fe13c9 
> 
> Diff: https://reviews.apache.org/r/15711/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Sriram Subramanian
> 
>

Re: Review Request 15711: Patch for KAFKA-930

[Sriram Subramanian]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/15711/
---

(Updated Nov. 20, 2013, 1:38 a.m.)


Review request for kafka.


Bugs: KAFKA-930
https://issues.apache.org/jira/browse/KAFKA-930


Repository: kafka


Description (updated)
---

commit missing code


some more changes


fix merge conflicts


Add auto leader rebalance support


Merge branch 'trunk' of http://git-wip-us.apache.org/repos/asf/kafka into trunk


Merge branch 'trunk' of http://git-wip-us.apache.org/repos/asf/kafka into trunk

Conflicts:
core/src/main/scala/kafka/admin/AdminUtils.scala
core/src/main/scala/kafka/admin/TopicCommand.scala

change comments


commit the remaining changes


Move AddPartitions into TopicCommand


Diffs (updated)
-

  core/src/main/scala/kafka/controller/KafkaController.scala 
88792c2b2a360e928ab9cd00de151e5d5f94452d 
  core/src/main/scala/kafka/server/KafkaConfig.scala 
b324344d0a383398db8bfe2cbeec2c1378fe13c9 

Diff: https://reviews.apache.org/r/15711/diff/


Testing
---


Thanks,

Sriram Subramanian

Re: Review Request 15711: Patch for KAFKA-930

[Sriram Subramanian]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/15711/
---

(Updated Nov. 20, 2013, 1:37 a.m.)


Review request for kafka.


Bugs: KAFKA-930
https://issues.apache.org/jira/browse/KAFKA-930


Repository: kafka


Description
---

some more changes


fix merge conflicts


Add auto leader rebalance support


Merge branch 'trunk' of http://git-wip-us.apache.org/repos/asf/kafka into trunk


Merge branch 'trunk' of http://git-wip-us.apache.org/repos/asf/kafka into trunk

Conflicts:
core/src/main/scala/kafka/admin/AdminUtils.scala
core/src/main/scala/kafka/admin/TopicCommand.scala

change comments


commit the remaining changes


Move AddPartitions into TopicCommand


Diffs (updated)
-

  core/src/main/scala/kafka/controller/KafkaController.scala 
88792c2b2a360e928ab9cd00de151e5d5f94452d 
  core/src/main/scala/kafka/server/KafkaConfig.scala 
b324344d0a383398db8bfe2cbeec2c1378fe13c9 

Diff: https://reviews.apache.org/r/15711/diff/


Testing
---


Thanks,

Sriram Subramanian

Review Request 15711: Patch for KAFKA-930

[Sriram Subramanian]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/15711/
---

Review request for kafka.


Bugs: KAFKA-930
https://issues.apache.org/jira/browse/KAFKA-930


Repository: kafka


Description
---

some more changes


fix merge conflicts


Add auto leader rebalance support


Merge branch 'trunk' of http://git-wip-us.apache.org/repos/asf/kafka into trunk


Merge branch 'trunk' of http://git-wip-us.apache.org/repos/asf/kafka into trunk

Conflicts:
core/src/main/scala/kafka/admin/AdminUtils.scala
core/src/main/scala/kafka/admin/TopicCommand.scala

change comments


commit the remaining changes


Move AddPartitions into TopicCommand


Diffs
-

  core/src/main/scala/kafka/controller/KafkaController.scala 
88792c2b2a360e928ab9cd00de151e5d5f94452d 
  core/src/main/scala/kafka/server/KafkaConfig.scala 
b324344d0a383398db8bfe2cbeec2c1378fe13c9 

Diff: https://reviews.apache.org/r/15711/diff/


Testing
---


Thanks,

Sriram Subramanian