You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@kafka.apache.org by Sriram Subramanian <sr...@gmail.com> on 2013/11/20 02:19:08 UTC
Review Request 15711: Patch for KAFKA-930
-----------------------------------------------------------
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
Re: Review Request 15711: Patch for KAFKA-930
Posted by Sriram Subramanian <sr...@gmail.com>.
> On Nov. 21, 2013, 2:43 a.m., Joel Koshy wrote:
> > core/src/main/scala/kafka/controller/KafkaController.scala, line 944
> > <https://reviews.apache.org/r/15711/diff/3/?file=388714#file388714line944>
> >
> > 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
Posted by Joel Koshy <jj...@gmail.com>.
-----------------------------------------------------------
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
<https://reviews.apache.org/r/15711/#comment56352>
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
<https://reviews.apache.org/r/15711/#comment56351>
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
Posted by Neha Narkhede <ne...@gmail.com>.
-----------------------------------------------------------
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
<https://reviews.apache.org/r/15711/#comment57005>
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
Posted by Neha Narkhede <ne...@gmail.com>.
-----------------------------------------------------------
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
<https://reviews.apache.org/r/15711/#comment56993>
instead of hardcoding this to 5 seconds, how about delaying it by leaderImbalanceCheckIntervalSeconds?
core/src/main/scala/kafka/controller/KafkaController.scala
<https://reviews.apache.org/r/15711/#comment57002>
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
<https://reviews.apache.org/r/15711/#comment57001>
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
<https://reviews.apache.org/r/15711/#comment56995>
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
<https://reviews.apache.org/r/15711/#comment56999>
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
<https://reviews.apache.org/r/15711/#comment56997>
"trigger a leader rebalance for partitions that should have a leader on this broker" ?
core/src/main/scala/kafka/controller/KafkaController.scala
<https://reviews.apache.org/r/15711/#comment57000>
could we rename topicPartition to replicasPerPartition?
core/src/main/scala/kafka/server/KafkaConfig.scala
<https://reviews.apache.org/r/15711/#comment56992>
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
Posted by Jun Rao <ju...@gmail.com>.
-----------------------------------------------------------
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
<https://reviews.apache.org/r/15711/#comment56377>
Does this need to be at info level?
core/src/main/scala/kafka/controller/KafkaController.scala
<https://reviews.apache.org/r/15711/#comment56373>
Could we rewrite it as the following to make it clear?
groupBy { case(topicAndParttion, assignedReplicas) : => .. }
core/src/main/scala/kafka/controller/KafkaController.scala
<https://reviews.apache.org/r/15711/#comment56374>
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
<https://reviews.apache.org/r/15711/#comment56375>
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
<https://reviews.apache.org/r/15711/#comment56376>
Should this be info level logging?
core/src/main/scala/kafka/controller/KafkaController.scala
<https://reviews.apache.org/r/15711/#comment56378>
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
Posted by Neha Narkhede <ne...@gmail.com>.
-----------------------------------------------------------
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
Posted by Neha Narkhede <ne...@gmail.com>.
-----------------------------------------------------------
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
Posted by Jun Rao <ju...@gmail.com>.
-----------------------------------------------------------
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
<https://reviews.apache.org/r/15711/#comment65733>
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
Posted by Sriram Subramanian <sr...@gmail.com>.
-----------------------------------------------------------
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
Posted by Jun Rao <ju...@gmail.com>.
-----------------------------------------------------------
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
Posted by Sriram Subramanian <sr...@gmail.com>.
-----------------------------------------------------------
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
Posted by Jun Rao <ju...@gmail.com>.
> On Dec. 23, 2013, 7:54 p.m., Jun Rao wrote:
> > core/src/main/scala/kafka/controller/KafkaController.scala, lines 964-965
> > <https://reviews.apache.org/r/15711/diff/8/?file=401724#file401724line964>
> >
> > 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
Posted by Jun Rao <ju...@gmail.com>.
-----------------------------------------------------------
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
<https://reviews.apache.org/r/15711/#comment59019>
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
<https://reviews.apache.org/r/15711/#comment59023>
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
<https://reviews.apache.org/r/15711/#comment59031>
I think we should use liveOrShuttingDownBrokerIds instead.
core/src/main/scala/kafka/controller/KafkaController.scala
<https://reviews.apache.org/r/15711/#comment59030>
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
Posted by Sriram Subramanian <sr...@gmail.com>.
-----------------------------------------------------------
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
Posted by Sriram Subramanian <sr...@gmail.com>.
-----------------------------------------------------------
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
Posted by Guozhang Wang <gu...@linkedin.com>.
-----------------------------------------------------------
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
<https://reviews.apache.org/r/15711/#comment57721>
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
Posted by Sriram Subramanian <sr...@gmail.com>.
> On Dec. 12, 2013, 12:17 a.m., Neha Narkhede wrote:
> > core/src/main/scala/kafka/controller/KafkaController.scala, line 947
> > <https://reviews.apache.org/r/15711/diff/6/?file=396094#file396094line947>
> >
> > 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
Posted by Sriram Subramanian <sr...@gmail.com>.
> On Dec. 12, 2013, 12:17 a.m., Neha Narkhede wrote:
> > core/src/main/scala/kafka/controller/KafkaController.scala, line 773
> > <https://reviews.apache.org/r/15711/diff/6/?file=396094#file396094line773>
> >
> > 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
> > <https://reviews.apache.org/r/15711/diff/6/?file=396095#file396095line234>
> >
> > 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
Posted by Neha Narkhede <ne...@gmail.com>.
> On Dec. 12, 2013, 12:17 a.m., Neha Narkhede wrote:
> > core/src/main/scala/kafka/controller/KafkaController.scala, line 947
> > <https://reviews.apache.org/r/15711/diff/6/?file=396094#file396094line947>
> >
> > 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
Posted by Neha Narkhede <ne...@gmail.com>.
-----------------------------------------------------------
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
<https://reviews.apache.org/r/15711/#comment57883>
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
<https://reviews.apache.org/r/15711/#comment57884>
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
<https://reviews.apache.org/r/15711/#comment57885>
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
Posted by Sriram Subramanian <sr...@gmail.com>.
-----------------------------------------------------------
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
Posted by Sriram Subramanian <sr...@gmail.com>.
-----------------------------------------------------------
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
Posted by Jun Rao <ju...@gmail.com>.
-----------------------------------------------------------
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
Posted by Guozhang Wang <gu...@linkedin.com>.
-----------------------------------------------------------
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
<https://reviews.apache.org/r/15711/#comment56380>
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
Posted by Jun Rao <ju...@gmail.com>.
-----------------------------------------------------------
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
Posted by Sriram Subramanian <sr...@gmail.com>.
-----------------------------------------------------------
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
Posted by Sriram Subramanian <sr...@gmail.com>.
> On Nov. 20, 2013, 6:30 p.m., Jun Rao wrote:
> > core/src/main/scala/kafka/controller/KafkaController.scala, lines 745-746
> > <https://reviews.apache.org/r/15711/diff/3/?file=388714#file388714line745>
> >
> > 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
> > <https://reviews.apache.org/r/15711/diff/3/?file=388715#file388715line236>
> >
> > 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
Posted by Jun Rao <ju...@gmail.com>.
> On Nov. 20, 2013, 6:30 p.m., Jun Rao wrote:
> > core/src/main/scala/kafka/server/KafkaConfig.scala, lines 236-239
> > <https://reviews.apache.org/r/15711/diff/3/?file=388715#file388715line236>
> >
> > 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
> > <https://reviews.apache.org/r/15711/diff/3/?file=388714#file388714line745>
> >
> > 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
Posted by Sriram Subramanian <sr...@gmail.com>.
> On Nov. 20, 2013, 6:30 p.m., Jun Rao wrote:
> > core/src/main/scala/kafka/controller/KafkaController.scala, lines 260-261
> > <https://reviews.apache.org/r/15711/diff/3/?file=388714#file388714line260>
> >
> > 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
> > <https://reviews.apache.org/r/15711/diff/3/?file=388714#file388714line745>
> >
> > 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
> > <https://reviews.apache.org/r/15711/diff/3/?file=388715#file388715line236>
> >
> > 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
Posted by Jun Rao <ju...@gmail.com>.
-----------------------------------------------------------
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
<https://reviews.apache.org/r/15711/#comment56317>
Instead of delaying by 5 secs, should we just delay by config.leaderImbalanceCheckIntervalSeconds?
core/src/main/scala/kafka/controller/KafkaController.scala
<https://reviews.apache.org/r/15711/#comment56319>
Could we rename updateZk to sth like isTriggeredByCommandLine?
core/src/main/scala/kafka/server/KafkaConfig.scala
<https://reviews.apache.org/r/15711/#comment56318>
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
Posted by Sriram Subramanian <sr...@gmail.com>.
> On Nov. 21, 2013, 3:41 a.m., Swapnil Ghike wrote:
> > core/src/main/scala/kafka/controller/KafkaController.scala, line 926
> > <https://reviews.apache.org/r/15711/diff/3/?file=388714#file388714line926>
> >
> > 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
Posted by Sriram Subramanian <sr...@gmail.com>.
> On Nov. 21, 2013, 3:41 a.m., Swapnil Ghike wrote:
> > core/src/main/scala/kafka/controller/KafkaController.scala, lines 745-746
> > <https://reviews.apache.org/r/15711/diff/3/?file=388714#file388714line745>
> >
> > 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
> > <https://reviews.apache.org/r/15711/diff/3/?file=388714#file388714line918>
> >
> > 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
> > <https://reviews.apache.org/r/15711/diff/3/?file=388714#file388714line946>
> >
> > 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
> > <https://reviews.apache.org/r/15711/diff/3/?file=388715#file388715line236>
> >
> > 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
Posted by Swapnil Ghike <sg...@linkedin.com>.
-----------------------------------------------------------
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
<https://reviews.apache.org/r/15711/#comment56354>
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
<https://reviews.apache.org/r/15711/#comment56356>
rename to allReplicasForTopicPartitionsPerBroker? (I saw the per convention used somewhere else)
core/src/main/scala/kafka/controller/KafkaController.scala
<https://reviews.apache.org/r/15711/#comment56355>
rename to topicPartitionsNotLedByPreferredReplica?
core/src/main/scala/kafka/controller/KafkaController.scala
<https://reviews.apache.org/r/15711/#comment56353>
we should be able to pass the entire set of partitions in one call, right?
core/src/main/scala/kafka/server/KafkaConfig.scala
<https://reviews.apache.org/r/15711/#comment56357>
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
Posted by Sriram Subramanian <sr...@gmail.com>.
-----------------------------------------------------------
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
Posted by Sriram Subramanian <sr...@gmail.com>.
-----------------------------------------------------------
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