You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@hbase.apache.org by Timothy Brown <ti...@siftscience.com> on 2017/02/15 22:47:43 UTC
Possible bookkeeping error in BaseLoadBalancer
Hi,
I was wondering if someone could confirm or deny my suspicion that the
"numMaxRegionsPerTable" is not being updated properly at line 670 of
BaseLoadBalancer.java
<https://github.com/apache/hbase/blob/master/hbase-server/src/main/java/org/apache/hadoop/hbase/master/balancer/BaseLoadBalancer.java>.
The comment says "check whether this caused maxRegionsPerTable in the new
Server to be updated" but then does not update numMaxRegionsPerTable. I
stepped through 'testRegionAvailabilityWithRegionMoves' in
TestBaseLoadBalancer.java and saw the maxRegionsPerTable never gets
increased to 2 after the region is moved to a RegionServer that already has
a region. If the maxRegionsPerTable isn't updated with the new max, then
the balancer will properly penalize this potential move which causes a
worse Table Skew.
Code in question:
//check whether this caused maxRegionsPerTable in the new Server to be
updated
if (numRegionsPerServerPerTable[newServer][tableIndex] >
numMaxRegionsPerTable[tableIndex]) {
numMaxRegionsPerTable[tableIndex] =
numRegionsPerServerPerTable[newServer][tableIndex];
}
Thanks,
Tim
Re: Possible bookkeeping error in BaseLoadBalancer
Posted by Timothy Brown <ti...@siftscience.com>.
Ok I created HBASE-17658 <https://issues.apache.org/jira/browse/HBASE-17658> to
track this. I've also found a test that I can modify to prove the issue.
On Wed, Feb 15, 2017 at 4:45 PM, Ted Yu <yu...@gmail.com> wrote:
> Looks like the current code has the assignment reverted.
>
> Can you log a JIRA ?
>
> If you can show the defect through modified unit test, that would be nice.
>
> On Wed, Feb 15, 2017 at 4:20 PM, Timothy Brown <ti...@siftscience.com>
> wrote:
>
> > Sorry I pasted code that I had modified. I am looking at the master
> branch
> > though. Why would the numRegionsPerServerPerTable[newServer][tableIndex]
> > be
> > set to the existing numMaxRegionsPerTable[tableIndex]? it seems like
> this
> > should be switched to what I pasted.
> >
> > On Wed, Feb 15, 2017 at 2:58 PM, Ted Yu <yu...@gmail.com> wrote:
> >
> > > In master branch, I see this:
> > >
> > > if (numRegionsPerServerPerTable[newServer][tableIndex] >
> > > numMaxRegionsPerTable[tableIndex]) {
> > >
> > > numRegionsPerServerPerTable[newServer][tableIndex] =
> > > numMaxRegionsPerTable[tableIndex];
> > >
> > > which branch are you looking at ?
> > >
> > > Thanks
> > >
> > > On Wed, Feb 15, 2017 at 2:47 PM, Timothy Brown <ti...@siftscience.com>
> > > wrote:
> > >
> > > > Hi,
> > > >
> > > > I was wondering if someone could confirm or deny my suspicion that
> the
> > > > "numMaxRegionsPerTable" is not being updated properly at line 670 of
> > > > BaseLoadBalancer.java
> > > > <https://github.com/apache/hbase/blob/master/hbase-
> > > > server/src/main/java/org/apache/hadoop/hbase/master/
> > > > balancer/BaseLoadBalancer.java>.
> > > > The comment says "check whether this caused maxRegionsPerTable in the
> > new
> > > > Server to be updated" but then does not update
> numMaxRegionsPerTable. I
> > > > stepped through 'testRegionAvailabilityWithRegionMoves' in
> > > > TestBaseLoadBalancer.java and saw the maxRegionsPerTable never gets
> > > > increased to 2 after the region is moved to a RegionServer that
> already
> > > has
> > > > a region. If the maxRegionsPerTable isn't updated with the new max,
> > then
> > > > the balancer will properly penalize this potential move which causes
> a
> > > > worse Table Skew.
> > > >
> > > >
> > > > Code in question:
> > > >
> > > > //check whether this caused maxRegionsPerTable in the new Server to
> be
> > > > updated
> > > > if (numRegionsPerServerPerTable[newServer][tableIndex] >
> > > > numMaxRegionsPerTable[tableIndex]) {
> > > > numMaxRegionsPerTable[tableIndex] =
> > > > numRegionsPerServerPerTable[newServer][tableIndex];
> > > > }
> > > >
> > > >
> > > > Thanks,
> > > > Tim
> > > >
> > >
> >
>
Re: Possible bookkeeping error in BaseLoadBalancer
Posted by Ted Yu <yu...@gmail.com>.
Looks like the current code has the assignment reverted.
Can you log a JIRA ?
If you can show the defect through modified unit test, that would be nice.
On Wed, Feb 15, 2017 at 4:20 PM, Timothy Brown <ti...@siftscience.com> wrote:
> Sorry I pasted code that I had modified. I am looking at the master branch
> though. Why would the numRegionsPerServerPerTable[newServer][tableIndex]
> be
> set to the existing numMaxRegionsPerTable[tableIndex]? it seems like this
> should be switched to what I pasted.
>
> On Wed, Feb 15, 2017 at 2:58 PM, Ted Yu <yu...@gmail.com> wrote:
>
> > In master branch, I see this:
> >
> > if (numRegionsPerServerPerTable[newServer][tableIndex] >
> > numMaxRegionsPerTable[tableIndex]) {
> >
> > numRegionsPerServerPerTable[newServer][tableIndex] =
> > numMaxRegionsPerTable[tableIndex];
> >
> > which branch are you looking at ?
> >
> > Thanks
> >
> > On Wed, Feb 15, 2017 at 2:47 PM, Timothy Brown <ti...@siftscience.com>
> > wrote:
> >
> > > Hi,
> > >
> > > I was wondering if someone could confirm or deny my suspicion that the
> > > "numMaxRegionsPerTable" is not being updated properly at line 670 of
> > > BaseLoadBalancer.java
> > > <https://github.com/apache/hbase/blob/master/hbase-
> > > server/src/main/java/org/apache/hadoop/hbase/master/
> > > balancer/BaseLoadBalancer.java>.
> > > The comment says "check whether this caused maxRegionsPerTable in the
> new
> > > Server to be updated" but then does not update numMaxRegionsPerTable. I
> > > stepped through 'testRegionAvailabilityWithRegionMoves' in
> > > TestBaseLoadBalancer.java and saw the maxRegionsPerTable never gets
> > > increased to 2 after the region is moved to a RegionServer that already
> > has
> > > a region. If the maxRegionsPerTable isn't updated with the new max,
> then
> > > the balancer will properly penalize this potential move which causes a
> > > worse Table Skew.
> > >
> > >
> > > Code in question:
> > >
> > > //check whether this caused maxRegionsPerTable in the new Server to be
> > > updated
> > > if (numRegionsPerServerPerTable[newServer][tableIndex] >
> > > numMaxRegionsPerTable[tableIndex]) {
> > > numMaxRegionsPerTable[tableIndex] =
> > > numRegionsPerServerPerTable[newServer][tableIndex];
> > > }
> > >
> > >
> > > Thanks,
> > > Tim
> > >
> >
>
Re: Possible bookkeeping error in BaseLoadBalancer
Posted by Timothy Brown <ti...@siftscience.com>.
Sorry I pasted code that I had modified. I am looking at the master branch
though. Why would the numRegionsPerServerPerTable[newServer][tableIndex] be
set to the existing numMaxRegionsPerTable[tableIndex]? it seems like this
should be switched to what I pasted.
On Wed, Feb 15, 2017 at 2:58 PM, Ted Yu <yu...@gmail.com> wrote:
> In master branch, I see this:
>
> if (numRegionsPerServerPerTable[newServer][tableIndex] >
> numMaxRegionsPerTable[tableIndex]) {
>
> numRegionsPerServerPerTable[newServer][tableIndex] =
> numMaxRegionsPerTable[tableIndex];
>
> which branch are you looking at ?
>
> Thanks
>
> On Wed, Feb 15, 2017 at 2:47 PM, Timothy Brown <ti...@siftscience.com>
> wrote:
>
> > Hi,
> >
> > I was wondering if someone could confirm or deny my suspicion that the
> > "numMaxRegionsPerTable" is not being updated properly at line 670 of
> > BaseLoadBalancer.java
> > <https://github.com/apache/hbase/blob/master/hbase-
> > server/src/main/java/org/apache/hadoop/hbase/master/
> > balancer/BaseLoadBalancer.java>.
> > The comment says "check whether this caused maxRegionsPerTable in the new
> > Server to be updated" but then does not update numMaxRegionsPerTable. I
> > stepped through 'testRegionAvailabilityWithRegionMoves' in
> > TestBaseLoadBalancer.java and saw the maxRegionsPerTable never gets
> > increased to 2 after the region is moved to a RegionServer that already
> has
> > a region. If the maxRegionsPerTable isn't updated with the new max, then
> > the balancer will properly penalize this potential move which causes a
> > worse Table Skew.
> >
> >
> > Code in question:
> >
> > //check whether this caused maxRegionsPerTable in the new Server to be
> > updated
> > if (numRegionsPerServerPerTable[newServer][tableIndex] >
> > numMaxRegionsPerTable[tableIndex]) {
> > numMaxRegionsPerTable[tableIndex] =
> > numRegionsPerServerPerTable[newServer][tableIndex];
> > }
> >
> >
> > Thanks,
> > Tim
> >
>
Re: Possible bookkeeping error in BaseLoadBalancer
Posted by Ted Yu <yu...@gmail.com>.
In master branch, I see this:
if (numRegionsPerServerPerTable[newServer][tableIndex] >
numMaxRegionsPerTable[tableIndex]) {
numRegionsPerServerPerTable[newServer][tableIndex] =
numMaxRegionsPerTable[tableIndex];
which branch are you looking at ?
Thanks
On Wed, Feb 15, 2017 at 2:47 PM, Timothy Brown <ti...@siftscience.com> wrote:
> Hi,
>
> I was wondering if someone could confirm or deny my suspicion that the
> "numMaxRegionsPerTable" is not being updated properly at line 670 of
> BaseLoadBalancer.java
> <https://github.com/apache/hbase/blob/master/hbase-
> server/src/main/java/org/apache/hadoop/hbase/master/
> balancer/BaseLoadBalancer.java>.
> The comment says "check whether this caused maxRegionsPerTable in the new
> Server to be updated" but then does not update numMaxRegionsPerTable. I
> stepped through 'testRegionAvailabilityWithRegionMoves' in
> TestBaseLoadBalancer.java and saw the maxRegionsPerTable never gets
> increased to 2 after the region is moved to a RegionServer that already has
> a region. If the maxRegionsPerTable isn't updated with the new max, then
> the balancer will properly penalize this potential move which causes a
> worse Table Skew.
>
>
> Code in question:
>
> //check whether this caused maxRegionsPerTable in the new Server to be
> updated
> if (numRegionsPerServerPerTable[newServer][tableIndex] >
> numMaxRegionsPerTable[tableIndex]) {
> numMaxRegionsPerTable[tableIndex] =
> numRegionsPerServerPerTable[newServer][tableIndex];
> }
>
>
> Thanks,
> Tim
>