TestRegionRebalancing

[Ted Yu <yu...@gmail.com>]

Hi,
I have a question about TestRegionRebalancing.assertRegionsAreBalanced()
I see hardcoded slop of 0.1
In 0.90 and beyond, sloppiness is gone.
Therefore:
    // TODO: Fix this test.  Old balancer used to run with 'slop'.  New
    // balancer does not.
I want to get your opinion on how this test should be rewritten.

Thanks

Re: TestRegionRebalancing

[Ted Yu <yu...@gmail.com>]

This test was failing after I made AssignmentManager.getAssignments() filter
out regions whose request count was zero.
The rationale is that table created with multiple regions may hold some
empty regions for a while because user may not precisely predict the range
of row keys at the time of table creation.
TestRegionRebalancing uses the following to obtain average load:
      double avg = cluster.getMaster().getServerManager().getAverageLoad();
However, considering hbase-3676, ServerManager wouldn't be the best
candidate to give master such information.

I am thinking of moving the above method to AssignmentManager:
  public double getAverageLoad()
so that AssignmentManager becomes the single class that is aware of details
of region load.

Please comment.

On Sat, Mar 19, 2011 at 7:33 PM, Stack <st...@duboce.net> wrote:

> On Sat, Mar 19, 2011 at 2:52 PM, Ted Yu <yu...@gmail.com> wrote:
> > Hi,
> > I have a question about TestRegionRebalancing.assertRegionsAreBalanced()
> > I see hardcoded slop of 0.1
> > In 0.90 and beyond, sloppiness is gone.
> > Therefore:
> >    // TODO: Fix this test.  Old balancer used to run with 'slop'.  New
> >    // balancer does not.
> > I want to get your opinion on how this test should be rewritten.
> >
>
> Yes slop is gone.
>
> But that method still uses 'slop'.  IIRC -- and the method javadoc
> agrees -- the new balancer should make it so we are +/- 1 either side
> of the 'average'.  The 'slop' here is being used to do the +/- 1.
>
> You could change the method to do +/- 1 explicitly and clean up dead
> code (we set slop == 0.1 and then test for <= 0 which is unnecessary).
>
> Or just clean dead code and add comments about what 'slop' means in
> this context.
>
> St.Ack
>

Re: TestRegionRebalancing

[Stack <st...@duboce.net>]

On Sat, Mar 19, 2011 at 2:52 PM, Ted Yu <yu...@gmail.com> wrote:
> Hi,
> I have a question about TestRegionRebalancing.assertRegionsAreBalanced()
> I see hardcoded slop of 0.1
> In 0.90 and beyond, sloppiness is gone.
> Therefore:
>    // TODO: Fix this test.  Old balancer used to run with 'slop'.  New
>    // balancer does not.
> I want to get your opinion on how this test should be rewritten.
>

Yes slop is gone.

But that method still uses 'slop'.  IIRC -- and the method javadoc
agrees -- the new balancer should make it so we are +/- 1 either side
of the 'average'.  The 'slop' here is being used to do the +/- 1.

You could change the method to do +/- 1 explicitly and clean up dead
code (we set slop == 0.1 and then test for <= 0 which is unnecessary).

Or just clean dead code and add comments about what 'slop' means in
this context.

St.Ack