> On April 22, 2014, 2:27 p.m., Eric Newton wrote: > > src/server/src/main/java/org/apache/accumulo/server/master/balancer/DefaultLoadBalancer.java, > > line 315 > > <https://reviews.apache.org/r/20525/diff/2/?file=563456#file563456line315> > > > > noservers is constant, should be NO_SERVERS.
fixed > On April 22, 2014, 2:27 p.m., Eric Newton wrote: > > src/server/src/main/java/org/apache/accumulo/server/master/balancer/DefaultLoadBalancer.java, > > line 317 > > <https://reviews.apache.org/r/20525/diff/2/?file=563456#file563456line317> > > > > Duplicate code. Create a public class, add a setter for migrations (or > > provide in the ctor). > > Sean Busbey wrote: > I had started to do this, as helper classes in TabletBalancer. The issue > I ran into was then the wrong logger is in scope. That is, I want to know the > DefaultLoadBalancer or the ChaoticBalancer has a problem, not just that the > top level TabletBalancer. > > I could pass a Logger into the constructor, but it looked super awkward > in comparison to the duplicated code. preferable? > > Bill Havanki wrote: > Since the point of running a BalancerProblem is to log, I think passing a > logger would be OK. You could also pass the balancer to it and have the > balancer allow logging through it, that's sorta complicated though. moved to TabletBalancer and passed Logger. kept it protected, since these are only needed by subclasses of TabletBalancer. > On April 22, 2014, 2:27 p.m., Eric Newton wrote: > > src/server/src/main/java/org/apache/accumulo/server/master/balancer/DefaultLoadBalancer.java, > > line 333 > > <https://reviews.apache.org/r/20525/diff/2/?file=563456#file563456line333> > > > > At this point balancing has not been performed, it could throw an error > > and balanceSuccessful doesn't really reflect a successful balance call. > > Rename to resetBalanceError, or move to post-balance. renamed to resetBalancerErrors - Sean ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/20525/#review41013 ----------------------------------------------------------- On April 21, 2014, 9:32 p.m., Sean Busbey wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/20525/ > ----------------------------------------------------------- > > (Updated April 21, 2014, 9:32 p.m.) > > > Review request for accumulo, Eric Newton and Mike Drob. > > > Bugs: ACCUMULO-2694 > https://issues.apache.org/jira/browse/ACCUMULO-2694 > > > Repository: accumulo > > > Description > ------- > > ACCUMULO-2694 Fix handling of tablet migrations for offline tables. > > * Adds a funtional test that fails due to not rebalancing > * Fix master to clear migrations when it learns that a table has gone > offline > * Update master to periodically clean up migrations for offline tables > * Fix balancers to make sure they log if they can't balance. > > > Diffs > ----- > > src/server/pom.xml dbe4fb4 > src/server/src/main/java/org/apache/accumulo/server/master/Master.java > fb7be51 > > src/server/src/main/java/org/apache/accumulo/server/master/balancer/ChaoticLoadBalancer.java > 02a4e89 > > src/server/src/main/java/org/apache/accumulo/server/master/balancer/DefaultLoadBalancer.java > 4826097 > > src/server/src/main/java/org/apache/accumulo/server/master/balancer/TabletBalancer.java > ad62360 > test/system/auto/stress/migrations.py d07d7a8 > > Diff: https://reviews.apache.org/r/20525/diff/ > > > Testing > ------- > > Ran functional test without other changes -> failed. After full patch > functional test passes. > > > Thanks, > > Sean Busbey > >
