Home | About | Sematext search-lucene.com search-hadoop.com
 Search Hadoop and all its subprojects:

Switch to Threaded View
Kafka, mail # dev - Review Request 15711: Patch for KAFKA-930


Copy link to this message
-
Re: Review Request 15711: Patch for KAFKA-930
Sriram Subramanian 2013-11-21, 17:42


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