----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/12955/#review23880 -----------------------------------------------------------
Minor comments on formatting and refactoring. Is it better to have an interface for Algorithms/constraint solver. Will be great if we can express the objectives in some form, like strategy.minimizeMovement, strategy.acceptableDeviation, strategy.balance, strategy.autoCreateOnError/disabled nodes etc. But these can come later. helix-core/src/main/java/org/apache/helix/controller/strategy/AutoRebalanceStrategy.java <https://reviews.apache.org/r/12955/#comment47698> naming convention is slightly different, it should start with _. these should be final,private helix-core/src/main/java/org/apache/helix/controller/strategy/AutoRebalanceStrategy.java <https://reviews.apache.org/r/12955/#comment47696> Remove the spaces. Follow the indentation thats used in other files. helix-core/src/main/java/org/apache/helix/controller/strategy/AutoRebalanceStrategy.java <https://reviews.apache.org/r/12955/#comment47699> Have a check on log.isinfoenabled helix-core/src/main/java/org/apache/helix/controller/strategy/AutoRebalanceStrategy.java <https://reviews.apache.org/r/12955/#comment47701> 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. helix-core/src/main/java/org/apache/helix/controller/strategy/AutoRebalanceStrategy.java <https://reviews.apache.org/r/12955/#comment47702> move these steps to separate methods. preferredAssignment, existingPreferred, existingNonPreferred, orphaned must be instance variables so that they dont get passed around. helix-core/src/main/java/org/apache/helix/controller/strategy/AutoRebalanceStrategy.java <https://reviews.apache.org/r/12955/#comment47703> should be warn helix-core/src/main/java/org/apache/helix/controller/strategy/AutoRebalanceStrategy.java <https://reviews.apache.org/r/12955/#comment47704> Add comments here helix-core/src/main/java/org/apache/helix/controller/strategy/AutoRebalanceStrategy.java <https://reviews.apache.org/r/12955/#comment47705> comment what this method is doing, same for all methods helix-core/src/main/java/org/apache/helix/controller/strategy/AutoRebalanceStrategy.java <https://reviews.apache.org/r/12955/#comment47706> javadoc/comment as to what this class is doing helix-core/src/main/java/org/apache/helix/controller/strategy/AutoRebalanceStrategy.java <https://reviews.apache.org/r/12955/#comment47707> missing javadoc/comment helix-core/src/test/java/org/apache/helix/controller/strategy/TestAutoRebalanceStrategy.java <https://reviews.apache.org/r/12955/#comment47708> use the standard formatter. removes empty spaces helix-core/src/test/java/org/apache/helix/controller/strategy/TestAutoRebalanceStrategy.java <https://reviews.apache.org/r/12955/#comment47709> remove sysouts, use loggers helix-core/src/test/java/org/apache/helix/controller/strategy/TestAutoRebalanceStrategy.java <https://reviews.apache.org/r/12955/#comment47710> set some thresholds and add asserts - Kishore Gopalakrishna 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 > >
