You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@helix.apache.org by Kanak Biscuitwala <ka...@hotmail.com> on 2013/08/08 00:30:32 UTC
Review Request 13390: [HELIX-173] Move rebalancing strategies to separate
classes that implement Rebalancer
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/13390/
-----------------------------------------------------------
Review request for helix, Zhen Zhang, Kishore Gopalakrishna, and Shi Lu.
Bugs: HELIX-173
Repository: helix-git
Description
-------
Fix for HELIX-173. Right now, the code to compute mappings from partitions to nodes and states is mostly in a single file, regardless of the rebalancing mode. Moving these operations cleans up the code and is more logical for pluggable rebalancers.
Diffs
-----
helix-core/src/main/java/org/apache/helix/controller/rebalancer/AutoRebalancer.java PRE-CREATION
helix-core/src/main/java/org/apache/helix/controller/rebalancer/CustomRebalancer.java PRE-CREATION
helix-core/src/main/java/org/apache/helix/controller/rebalancer/Rebalancer.java 62a73b3
helix-core/src/main/java/org/apache/helix/controller/rebalancer/SemiAutoRebalancer.java PRE-CREATION
helix-core/src/main/java/org/apache/helix/controller/stages/BestPossibleStateCalcStage.java aca0e74
helix-core/src/main/java/org/apache/helix/controller/stages/util/BestStateCalculator.java PRE-CREATION
helix-core/src/main/java/org/apache/helix/controller/strategy/AutoRebalanceStrategy.java f013937
helix-core/src/test/java/org/apache/helix/controller/strategy/TestAutoRebalanceStrategy.java 50a2f81
helix-core/src/test/java/org/apache/helix/integration/TestCustomizedIdealStateRebalancer.java df32c4e
Diff: https://reviews.apache.org/r/13390/diff/
Testing
-------
Tests pass locally.
Thanks,
Kanak Biscuitwala
Re: Review Request 13390: [HELIX-173] Move rebalancing strategies to
separate classes that implement Rebalancer
Posted by Kanak Biscuitwala <ka...@hotmail.com>.
> On Aug. 7, 2013, 11:33 p.m., Kishore Gopalakrishna wrote:
> > Can you see if the test coverage increase/decrease after this change
The change is minimal, if any. There are a few more error checks, leading to a slight decrease.
> On Aug. 7, 2013, 11:33 p.m., Kishore Gopalakrishna wrote:
> > helix-core/src/main/java/org/apache/helix/controller/rebalancer/CustomRebalancer.java, line 44
> > <https://reviews.apache.org/r/13390/diff/2/?file=338469#file338469line44>
> >
> > remove this line,
Done
> On Aug. 7, 2013, 11:33 p.m., Kishore Gopalakrishna wrote:
> > helix-core/src/main/java/org/apache/helix/controller/rebalancer/SemiAutoRebalancer.java, line 42
> > <https://reviews.apache.org/r/13390/diff/2/?file=338471#file338471line42>
> >
> > remove this line
Done
> On Aug. 7, 2013, 11:33 p.m., Kishore Gopalakrishna wrote:
> > helix-core/src/main/java/org/apache/helix/controller/strategy/AutoRebalanceStrategy.java, line 46
> > <https://reviews.apache.org/r/13390/diff/2/?file=338474#file338474line46>
> >
> > javadoc
Done
> On Aug. 7, 2013, 11:33 p.m., Kishore Gopalakrishna wrote:
> > helix-core/src/main/java/org/apache/helix/controller/rebalancer/SemiAutoRebalancer.java, line 37
> > <https://reviews.apache.org/r/13390/diff/2/?file=338471#file338471line37>
> >
> > java doc
Done
> On Aug. 7, 2013, 11:33 p.m., Kishore Gopalakrishna wrote:
> > helix-core/src/main/java/org/apache/helix/controller/rebalancer/CustomRebalancer.java, line 38
> > <https://reviews.apache.org/r/13390/diff/2/?file=338469#file338469line38>
> >
> > javadoc for this, give an example input/output
Done
> On Aug. 7, 2013, 11:33 p.m., Kishore Gopalakrishna wrote:
> > helix-core/src/main/java/org/apache/helix/controller/rebalancer/Rebalancer.java, line 68
> > <https://reviews.apache.org/r/13390/diff/2/?file=338470#file338470line68>
> >
> > Having a map<string,map<String,string> is confusing. Have a new class called ResourceMapping or something like that
Done
> On Aug. 7, 2013, 11:33 p.m., Kishore Gopalakrishna wrote:
> > helix-core/src/main/java/org/apache/helix/controller/stages/util/BestStateCalculator.java, line 1
> > <https://reviews.apache.org/r/13390/diff/2/?file=338473#file338473line1>
> >
> > should probably be under rebalancer.util. This is kind of the constraint solver right. need a better name.
Done
> On Aug. 7, 2013, 11:33 p.m., Kishore Gopalakrishna wrote:
> > helix-core/src/main/java/org/apache/helix/controller/stages/BestPossibleStateCalcStage.java, line 219
> > <https://reviews.apache.org/r/13390/diff/2/?file=338472#file338472line219>
> >
> > see the OSGi patch. Using class.forname is not liked by OSGi
Done
- Kanak
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/13390/#review24847
-----------------------------------------------------------
On Aug. 7, 2013, 11:26 p.m., Kanak Biscuitwala wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/13390/
> -----------------------------------------------------------
>
> (Updated Aug. 7, 2013, 11:26 p.m.)
>
>
> Review request for helix, Zhen Zhang, Kishore Gopalakrishna, and Shi Lu.
>
>
> Bugs: HELIX-173
>
>
> Repository: helix-git
>
>
> Description
> -------
>
> Fix for HELIX-173. Right now, the code to compute mappings from partitions to nodes and states is mostly in a single file, regardless of the rebalancing mode. Moving these operations cleans up the code and is more logical for pluggable rebalancers.
>
>
> Diffs
> -----
>
> helix-core/src/main/java/org/apache/helix/controller/rebalancer/AutoRebalancer.java PRE-CREATION
> helix-core/src/main/java/org/apache/helix/controller/rebalancer/CustomRebalancer.java PRE-CREATION
> helix-core/src/main/java/org/apache/helix/controller/rebalancer/Rebalancer.java 62a73b3
> helix-core/src/main/java/org/apache/helix/controller/rebalancer/SemiAutoRebalancer.java PRE-CREATION
> helix-core/src/main/java/org/apache/helix/controller/stages/BestPossibleStateCalcStage.java aca0e74
> helix-core/src/main/java/org/apache/helix/controller/stages/util/BestStateCalculator.java PRE-CREATION
> helix-core/src/main/java/org/apache/helix/controller/strategy/AutoRebalanceStrategy.java f013937
> helix-core/src/test/java/org/apache/helix/controller/strategy/TestAutoRebalanceStrategy.java 50a2f81
> helix-core/src/test/java/org/apache/helix/integration/TestCustomizedIdealStateRebalancer.java df32c4e
>
> Diff: https://reviews.apache.org/r/13390/diff/
>
>
> Testing
> -------
>
> Tests pass locally.
>
>
> Thanks,
>
> Kanak Biscuitwala
>
>
Re: Review Request 13390: [HELIX-173] Move rebalancing strategies to
separate classes that implement Rebalancer
Posted by Kishore Gopalakrishna <ki...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/13390/#review24847
-----------------------------------------------------------
Ship it!
Can you see if the test coverage increase/decrease after this change
helix-core/src/main/java/org/apache/helix/controller/rebalancer/CustomRebalancer.java
<https://reviews.apache.org/r/13390/#comment49006>
javadoc for this, give an example input/output
helix-core/src/main/java/org/apache/helix/controller/rebalancer/CustomRebalancer.java
<https://reviews.apache.org/r/13390/#comment49007>
remove this line,
helix-core/src/main/java/org/apache/helix/controller/rebalancer/Rebalancer.java
<https://reviews.apache.org/r/13390/#comment49008>
Having a map<string,map<String,string> is confusing. Have a new class called ResourceMapping or something like that
helix-core/src/main/java/org/apache/helix/controller/rebalancer/SemiAutoRebalancer.java
<https://reviews.apache.org/r/13390/#comment49009>
java doc
helix-core/src/main/java/org/apache/helix/controller/rebalancer/SemiAutoRebalancer.java
<https://reviews.apache.org/r/13390/#comment49010>
remove this line
helix-core/src/main/java/org/apache/helix/controller/stages/BestPossibleStateCalcStage.java
<https://reviews.apache.org/r/13390/#comment49011>
see the OSGi patch. Using class.forname is not liked by OSGi
helix-core/src/main/java/org/apache/helix/controller/stages/util/BestStateCalculator.java
<https://reviews.apache.org/r/13390/#comment49012>
should probably be under rebalancer.util. This is kind of the constraint solver right. need a better name.
helix-core/src/main/java/org/apache/helix/controller/strategy/AutoRebalanceStrategy.java
<https://reviews.apache.org/r/13390/#comment49013>
javadoc
- Kishore Gopalakrishna
On Aug. 7, 2013, 11:26 p.m., Kanak Biscuitwala wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/13390/
> -----------------------------------------------------------
>
> (Updated Aug. 7, 2013, 11:26 p.m.)
>
>
> Review request for helix, Zhen Zhang, Kishore Gopalakrishna, and Shi Lu.
>
>
> Bugs: HELIX-173
>
>
> Repository: helix-git
>
>
> Description
> -------
>
> Fix for HELIX-173. Right now, the code to compute mappings from partitions to nodes and states is mostly in a single file, regardless of the rebalancing mode. Moving these operations cleans up the code and is more logical for pluggable rebalancers.
>
>
> Diffs
> -----
>
> helix-core/src/main/java/org/apache/helix/controller/rebalancer/AutoRebalancer.java PRE-CREATION
> helix-core/src/main/java/org/apache/helix/controller/rebalancer/CustomRebalancer.java PRE-CREATION
> helix-core/src/main/java/org/apache/helix/controller/rebalancer/Rebalancer.java 62a73b3
> helix-core/src/main/java/org/apache/helix/controller/rebalancer/SemiAutoRebalancer.java PRE-CREATION
> helix-core/src/main/java/org/apache/helix/controller/stages/BestPossibleStateCalcStage.java aca0e74
> helix-core/src/main/java/org/apache/helix/controller/stages/util/BestStateCalculator.java PRE-CREATION
> helix-core/src/main/java/org/apache/helix/controller/strategy/AutoRebalanceStrategy.java f013937
> helix-core/src/test/java/org/apache/helix/controller/strategy/TestAutoRebalanceStrategy.java 50a2f81
> helix-core/src/test/java/org/apache/helix/integration/TestCustomizedIdealStateRebalancer.java df32c4e
>
> Diff: https://reviews.apache.org/r/13390/diff/
>
>
> Testing
> -------
>
> Tests pass locally.
>
>
> Thanks,
>
> Kanak Biscuitwala
>
>
Re: Review Request 13390: [HELIX-173] Move rebalancing strategies to
separate classes that implement Rebalancer
Posted by Kanak Biscuitwala <ka...@hotmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/13390/
-----------------------------------------------------------
(Updated Aug. 9, 2013, 5:04 p.m.)
Review request for helix, Zhen Zhang, Kishore Gopalakrishna, and Shi Lu.
Bugs: HELIX-173
Repository: helix-git
Description
-------
Fix for HELIX-173. Right now, the code to compute mappings from partitions to nodes and states is mostly in a single file, regardless of the rebalancing mode. Moving these operations cleans up the code and is more logical for pluggable rebalancers.
Diffs (updated)
-----
helix-core/src/main/java/org/apache/helix/controller/rebalancer/AutoRebalancer.java PRE-CREATION
helix-core/src/main/java/org/apache/helix/controller/rebalancer/CustomRebalancer.java PRE-CREATION
helix-core/src/main/java/org/apache/helix/controller/rebalancer/Rebalancer.java 62a73b3
helix-core/src/main/java/org/apache/helix/controller/rebalancer/SemiAutoRebalancer.java PRE-CREATION
helix-core/src/main/java/org/apache/helix/controller/rebalancer/util/ConstraintBasedAssignment.java PRE-CREATION
helix-core/src/main/java/org/apache/helix/controller/stages/BestPossibleStateCalcStage.java aca0e74
helix-core/src/main/java/org/apache/helix/controller/stages/RebalanceIdealStateStage.java 7ba0dbe
helix-core/src/main/java/org/apache/helix/controller/stages/ResourceMapping.java PRE-CREATION
helix-core/src/main/java/org/apache/helix/controller/strategy/AutoRebalanceStrategy.java f013937
helix-core/src/main/java/org/apache/helix/util/HelixUtil.java 34ffdc4
helix-core/src/test/java/org/apache/helix/controller/strategy/TestAutoRebalanceStrategy.java 50a2f81
helix-core/src/test/java/org/apache/helix/integration/TestCustomizedIdealStateRebalancer.java df32c4e
Diff: https://reviews.apache.org/r/13390/diff/
Testing
-------
Tests pass locally.
Thanks,
Kanak Biscuitwala
Re: Review Request 13390: [HELIX-173] Move rebalancing strategies to
separate classes that implement Rebalancer
Posted by Kanak Biscuitwala <ka...@hotmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/13390/
-----------------------------------------------------------
(Updated Aug. 7, 2013, 11:26 p.m.)
Review request for helix, Zhen Zhang, Kishore Gopalakrishna, and Shi Lu.
Bugs: HELIX-173
Repository: helix-git
Description
-------
Fix for HELIX-173. Right now, the code to compute mappings from partitions to nodes and states is mostly in a single file, regardless of the rebalancing mode. Moving these operations cleans up the code and is more logical for pluggable rebalancers.
Diffs (updated)
-----
helix-core/src/main/java/org/apache/helix/controller/rebalancer/AutoRebalancer.java PRE-CREATION
helix-core/src/main/java/org/apache/helix/controller/rebalancer/CustomRebalancer.java PRE-CREATION
helix-core/src/main/java/org/apache/helix/controller/rebalancer/Rebalancer.java 62a73b3
helix-core/src/main/java/org/apache/helix/controller/rebalancer/SemiAutoRebalancer.java PRE-CREATION
helix-core/src/main/java/org/apache/helix/controller/stages/BestPossibleStateCalcStage.java aca0e74
helix-core/src/main/java/org/apache/helix/controller/stages/util/BestStateCalculator.java PRE-CREATION
helix-core/src/main/java/org/apache/helix/controller/strategy/AutoRebalanceStrategy.java f013937
helix-core/src/test/java/org/apache/helix/controller/strategy/TestAutoRebalanceStrategy.java 50a2f81
helix-core/src/test/java/org/apache/helix/integration/TestCustomizedIdealStateRebalancer.java df32c4e
Diff: https://reviews.apache.org/r/13390/diff/
Testing
-------
Tests pass locally.
Thanks,
Kanak Biscuitwala