You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@helix.apache.org by Tom Widmer <to...@gmail.com> on 2014/11/10 14:07:51 UTC
Review Request 27808: [HELIX-543] Avoid moving partitions
unnecessarily when auto-rebalancing
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/27808/
-----------------------------------------------------------
Review request for helix and Kanak Biscuitwala.
Bugs: HELIX-543
Repository: helix-git
Description
-------
commit ba45d4bc2019c4069fe810c6e61a0696391f207e
Author: Tom Widmer <to...@camcog.com>
Date: Mon Nov 10 12:40:33 2014 +0000
[HELIX-543] Add missing licence header
:100644 100644 f59be07... ef1f57e... M helix-agent/helix-agent-0.7.2-SNAPSHOT.ivy
commit 5f63c6d69d594c70e85de31d5ed67f62f348ecb0
Author: Tom Widmer <to...@camcog.com>
Date: Mon Nov 10 12:26:40 2014 +0000
[HELIX-543] Avoid moving partitions unnecessarily when auto-rebalancing
This is done by allocating capacity first to those nodes which already have
the most partitions.
:100644 100644 09b66c1... 6e0e226... M helix-core/src/main/java/org/apache/helix/controller/strategy/AutoRebalanceStrategy.java
:100644 100644 1322b40... 25c550d... M helix-core/src/test/java/org/apache/helix/controller/strategy/TestAutoRebalanceStrategy.java
Diffs
-----
helix-agent/helix-agent-0.7.2-SNAPSHOT.ivy f59be07
helix-core/src/main/java/org/apache/helix/controller/strategy/AutoRebalanceStrategy.java 09b66c1
helix-core/src/test/java/org/apache/helix/controller/strategy/TestAutoRebalanceStrategy.java 1322b40
Diff: https://reviews.apache.org/r/27808/diff/
Testing
-------
Added new test to check case (failed before, now passes). Ran other re-balancing tests.
Thanks,
Tom Widmer
Re: Review Request 27808: [HELIX-543] Avoid moving partitions
unnecessarily when auto-rebalancing
Posted by Kishore Gopalakrishna <ki...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/27808/#review60630
-----------------------------------------------------------
Ship it!
Nice work!. Thanks for identifying this. Will be great to have a integration test. I can add that after we push this code. replicaCount is using String as key that is resulting in state.tostring, it might be better to use the concrete type(state) instead of converting it to String. We should probably fix that ((not neccessarily part of this review/jira))
- Kishore Gopalakrishna
On Nov. 10, 2014, 1:07 p.m., Tom Widmer wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/27808/
> -----------------------------------------------------------
>
> (Updated Nov. 10, 2014, 1:07 p.m.)
>
>
> Review request for helix and Kanak Biscuitwala.
>
>
> Bugs: HELIX-543
>
>
> Repository: helix-git
>
>
> Description
> -------
>
> commit ba45d4bc2019c4069fe810c6e61a0696391f207e
> Author: Tom Widmer <to...@camcog.com>
> Date: Mon Nov 10 12:40:33 2014 +0000
>
> [HELIX-543] Add missing licence header
>
> :100644 100644 f59be07... ef1f57e... M helix-agent/helix-agent-0.7.2-SNAPSHOT.ivy
>
> commit 5f63c6d69d594c70e85de31d5ed67f62f348ecb0
> Author: Tom Widmer <to...@camcog.com>
> Date: Mon Nov 10 12:26:40 2014 +0000
>
> [HELIX-543] Avoid moving partitions unnecessarily when auto-rebalancing
>
> This is done by allocating capacity first to those nodes which already have
> the most partitions.
>
> :100644 100644 09b66c1... 6e0e226... M helix-core/src/main/java/org/apache/helix/controller/strategy/AutoRebalanceStrategy.java
> :100644 100644 1322b40... 25c550d... M helix-core/src/test/java/org/apache/helix/controller/strategy/TestAutoRebalanceStrategy.java
>
>
> Diffs
> -----
>
> helix-agent/helix-agent-0.7.2-SNAPSHOT.ivy f59be07
> helix-core/src/main/java/org/apache/helix/controller/strategy/AutoRebalanceStrategy.java 09b66c1
> helix-core/src/test/java/org/apache/helix/controller/strategy/TestAutoRebalanceStrategy.java 1322b40
>
> Diff: https://reviews.apache.org/r/27808/diff/
>
>
> Testing
> -------
>
> Added new test to check case (failed before, now passes). Ran other re-balancing tests.
>
>
> Thanks,
>
> Tom Widmer
>
>
Re: Review Request 27808: [HELIX-543] Avoid moving partitions
unnecessarily when auto-rebalancing
Posted by Kanak Biscuitwala <ka...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/27808/#review60959
-----------------------------------------------------------
Ship it!
Looks good, assuming that existing integration tests still pass. Some ones to try include TestAutoRebalance, TestFullAutoNodeTagging, TestAutoRebalancePartitionLimit, and TestIndependentTaskRebalancer. Only minor comments regarding the code.
helix-agent/helix-agent-0.7.2-SNAPSHOT.ivy
<https://reviews.apache.org/r/27808/#comment102379>
thanks!
helix-core/src/main/java/org/apache/helix/controller/strategy/AutoRebalanceStrategy.java
<https://reviews.apache.org/r/27808/#comment102380>
active for each participant*
helix-core/src/main/java/org/apache/helix/controller/strategy/AutoRebalanceStrategy.java
<https://reviews.apache.org/r/27808/#comment102381>
It may be cleaner to use Guava comparator helpers, but this is fine too.
Reference: https://code.google.com/p/guava-libraries/wiki/CommonObjectUtilitiesExplained#compare/compareTo
- Kanak Biscuitwala
On Nov. 10, 2014, 5:07 a.m., Tom Widmer wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/27808/
> -----------------------------------------------------------
>
> (Updated Nov. 10, 2014, 5:07 a.m.)
>
>
> Review request for helix and Kanak Biscuitwala.
>
>
> Bugs: HELIX-543
>
>
> Repository: helix-git
>
>
> Description
> -------
>
> commit ba45d4bc2019c4069fe810c6e61a0696391f207e
> Author: Tom Widmer <to...@camcog.com>
> Date: Mon Nov 10 12:40:33 2014 +0000
>
> [HELIX-543] Add missing licence header
>
> :100644 100644 f59be07... ef1f57e... M helix-agent/helix-agent-0.7.2-SNAPSHOT.ivy
>
> commit 5f63c6d69d594c70e85de31d5ed67f62f348ecb0
> Author: Tom Widmer <to...@camcog.com>
> Date: Mon Nov 10 12:26:40 2014 +0000
>
> [HELIX-543] Avoid moving partitions unnecessarily when auto-rebalancing
>
> This is done by allocating capacity first to those nodes which already have
> the most partitions.
>
> :100644 100644 09b66c1... 6e0e226... M helix-core/src/main/java/org/apache/helix/controller/strategy/AutoRebalanceStrategy.java
> :100644 100644 1322b40... 25c550d... M helix-core/src/test/java/org/apache/helix/controller/strategy/TestAutoRebalanceStrategy.java
>
>
> Diffs
> -----
>
> helix-agent/helix-agent-0.7.2-SNAPSHOT.ivy f59be07
> helix-core/src/main/java/org/apache/helix/controller/strategy/AutoRebalanceStrategy.java 09b66c1
> helix-core/src/test/java/org/apache/helix/controller/strategy/TestAutoRebalanceStrategy.java 1322b40
>
> Diff: https://reviews.apache.org/r/27808/diff/
>
>
> Testing
> -------
>
> Added new test to check case (failed before, now passes). Ran other re-balancing tests.
>
>
> Thanks,
>
> Tom Widmer
>
>