> On July 25, 2013, 10:39 p.m., Kishore Gopalakrishna wrote: > > helix-core/src/test/java/org/apache/helix/controller/strategy/TestAutoRebalanceStrategy.java, > > line 193 > > <https://reviews.apache.org/r/12955/diff/1/?file=327965#file327965line193> > > > > set some thresholds and add asserts
what are reasonable thresholds? > On July 25, 2013, 10:39 p.m., Kishore Gopalakrishna wrote: > > helix-core/src/test/java/org/apache/helix/controller/strategy/TestAutoRebalanceStrategy.java, > > line 127 > > <https://reviews.apache.org/r/12955/diff/1/?file=327965#file327965line127> > > > > remove sysouts, use loggers done > On July 25, 2013, 10:39 p.m., Kishore Gopalakrishna wrote: > > helix-core/src/test/java/org/apache/helix/controller/strategy/TestAutoRebalanceStrategy.java, > > line 24 > > <https://reviews.apache.org/r/12955/diff/1/?file=327965#file327965line24> > > > > use the standard formatter. removes empty spaces done > On July 25, 2013, 10:39 p.m., Kishore Gopalakrishna wrote: > > helix-core/src/main/java/org/apache/helix/controller/strategy/AutoRebalanceStrategy.java, > > line 406 > > <https://reviews.apache.org/r/12955/diff/1/?file=327964#file327964line406> > > > > missing javadoc/comment done > On July 25, 2013, 10:39 p.m., Kishore Gopalakrishna wrote: > > helix-core/src/main/java/org/apache/helix/controller/strategy/AutoRebalanceStrategy.java, > > line 360 > > <https://reviews.apache.org/r/12955/diff/1/?file=327964#file327964line360> > > > > javadoc/comment as to what this class is doing done > On July 25, 2013, 10:39 p.m., Kishore Gopalakrishna wrote: > > helix-core/src/main/java/org/apache/helix/controller/strategy/AutoRebalanceStrategy.java, > > line 236 > > <https://reviews.apache.org/r/12955/diff/1/?file=327964#file327964line236> > > > > comment what this method is doing, same for all methods done > On July 25, 2013, 10:39 p.m., Kishore Gopalakrishna wrote: > > helix-core/src/main/java/org/apache/helix/controller/strategy/AutoRebalanceStrategy.java, > > line 202 > > <https://reviews.apache.org/r/12955/diff/1/?file=327964#file327964line202> > > > > Add comments here done > On July 25, 2013, 10:39 p.m., Kishore Gopalakrishna wrote: > > helix-core/src/main/java/org/apache/helix/controller/strategy/AutoRebalanceStrategy.java, > > line 198 > > <https://reviews.apache.org/r/12955/diff/1/?file=327964#file327964line198> > > > > should be warn done > On July 25, 2013, 10:39 p.m., Kishore Gopalakrishna wrote: > > helix-core/src/main/java/org/apache/helix/controller/strategy/AutoRebalanceStrategy.java, > > line 130 > > <https://reviews.apache.org/r/12955/diff/1/?file=327964#file327964line130> > > > > move these steps to separate methods. preferredAssignment, > > existingPreferred, existingNonPreferred, orphaned must be instance > > variables so that they dont get passed around. done - what else should be instance variables? I also made nodeMap and liveNodesList instance variables since they were being passed around everywhere > On July 25, 2013, 10:39 p.m., Kishore Gopalakrishna wrote: > > helix-core/src/main/java/org/apache/helix/controller/strategy/AutoRebalanceStrategy.java, > > line 80 > > <https://reviews.apache.org/r/12955/diff/1/?file=327964#file327964line80> > > > > break this method into multiple methods. We should probably take all > > parameters in a constructor or have separate class called Config that can > > be internal. Some of the parameters can be optional. Also constructor > > should probably take partitions,states, max per node. > > > > computeNIS should be renamed to something better. > > > > computeNIS should take currentMapping and liveNodes, allnodes. Since > > these are the things that change often. > > done - added a constructor > On July 25, 2013, 10:39 p.m., Kishore Gopalakrishna wrote: > > helix-core/src/main/java/org/apache/helix/controller/strategy/AutoRebalanceStrategy.java, > > line 57 > > <https://reviews.apache.org/r/12955/diff/1/?file=327964#file327964line57> > > > > Have a check on log.isinfoenabled done > On July 25, 2013, 10:39 p.m., Kishore Gopalakrishna wrote: > > helix-core/src/main/java/org/apache/helix/controller/strategy/AutoRebalanceStrategy.java, > > line 56 > > <https://reviews.apache.org/r/12955/diff/1/?file=327964#file327964line56> > > > > Remove the spaces. > > Follow the indentation thats used in other files. done > On July 25, 2013, 10:39 p.m., Kishore Gopalakrishna wrote: > > helix-core/src/main/java/org/apache/helix/controller/strategy/AutoRebalanceStrategy.java, > > line 28 > > <https://reviews.apache.org/r/12955/diff/1/?file=327964#file327964line28> > > > > naming convention is slightly different, it should start with _. > > > > these should be final,private done - could not make it final because it's initialized in init, not the constructor - Kanak ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/12955/#review23880 ----------------------------------------------------------- On July 25, 2013, 10:18 p.m., Kanak Biscuitwala wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/12955/ > ----------------------------------------------------------- > > (Updated July 25, 2013, 10:18 p.m.) > > > Review request for helix, Zhen Zhang, Kishore Gopalakrishna, and Shi Lu. > > > Bugs: HELIX-141 > > > Repository: helix-git > > > Description > ------- > > Fix for HELIX-141. Auto rebalancer that works for arbitrary partitions, > states, replicas, and nodes while keeping the load balanced and minimizing > movement. > > > Diffs > ----- > > > helix-core/src/main/java/org/apache/helix/controller/strategy/AutoRebalanceStrategy.java > PRE-CREATION > > helix-core/src/test/java/org/apache/helix/controller/strategy/TestAutoRebalanceStrategy.java > PRE-CREATION > > Diff: https://reviews.apache.org/r/12955/diff/ > > > Testing > ------- > > Including a tester class that randomly adds, removes, and resurrects nodes > and checks summary statistics and verifies baseline correctness. It's worked > for the test cases I've tried. > > > Thanks, > > Kanak Biscuitwala > >
