accumulo-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Sean Busbey" <s...@manvsbeard.com>
Subject Re: Review Request 20525: ACCUMULO-2694 Fix handling of tablet migrations for offline tables.
Date Thu, 24 Apr 2014 15:25:37 GMT


> 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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message