Mailing-List: contact dev-help@accumulo.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@accumulo.apache.org
Content-Type: multipart/alternative;
 boundary="===============2531858964088636142=="
MIME-Version: 1.0
Subject: Re: Review Request 20525: ACCUMULO-2694 Fix handling of tablet
 migrations
 for offline tables.
From: "Sean Busbey" <sean@manvsbeard.com>
To: "Eric Newton" <eric.newton@gmail.com>, "Mike Drob" <mdrob@mdrob.com>
Cc: "Bill Havanki" <bhavanki@clouderagovt.com>,
 "Sean Busbey" <sean@manvsbeard.com>, "accumulo" <dev@accumulo.apache.org>
Date: Thu, 24 Apr 2014 15:25:37 -0000
Message-ID: <20140424152537.2045.15769@reviews.apache.org>
Auto-Submitted: auto-generated
Sender: "Sean Busbey" <noreply@reviews.apache.org>
References: <20140422142711.24202.23248@reviews.apache.org>
In-Reply-To: <20140422142711.24202.23248@reviews.apache.org>
Reply-To: "Sean Busbey" <sean@manvsbeard.com>

--===============2531858964088636142==
Content-Type: text/plain; charset="utf-8"
MIME-Version: 1.0
Content-Transfer-Encoding: 7bit


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


--===============2531858964088636142==--