HBase
  1. HBase
  2. HBASE-5737

Minor Improvements related to balancer.

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 0.94.0, 0.95.0
    • Component/s: master
    • Labels:
      None

      Description

      Currently in Am.getAssignmentByTable() we use a result map which is currenly a hashmap. It could be better if we have a treeMap. Even in MetaReader.fullScan we have the treeMap only so that we have the naming order maintained. I felt this change could be very useful in cases where we are extending the DefaultLoadBalancer.

      1. HBASE-5737.patch
        3 kB
        ramkrishna.s.vasudevan
      2. HBASE-5737_1.patch
        4 kB
        ramkrishna.s.vasudevan
      3. HBASE-5737_2.patch
        3 kB
        ramkrishna.s.vasudevan
      4. HBASE-5737_3.patch
        7 kB
        ramkrishna.s.vasudevan

        Activity

        Hide
        ramkrishna.s.vasudevan added a comment -

        There are few more changes that we can do here
        -> Currently two different balancer objects are created in AssignmentManager and HMaster. Unify them
        -> balancer.randomAssignment() is getting called once but based on whether the existingplan is null or if it is a force plan we either use the randomplan or not.
        Ideally if we are extending a new balancer then randomAssignment will be called but later the plan given by randomAssignment may not be used.

        Show
        ramkrishna.s.vasudevan added a comment - There are few more changes that we can do here -> Currently two different balancer objects are created in AssignmentManager and HMaster. Unify them -> balancer.randomAssignment() is getting called once but based on whether the existingplan is null or if it is a force plan we either use the randomplan or not. Ideally if we are extending a new balancer then randomAssignment will be called but later the plan given by randomAssignment may not be used.
        Hide
        ramkrishna.s.vasudevan added a comment -

        Patch is minor..but important one incase we provide pluggability for the loadbalancer.

        Show
        ramkrishna.s.vasudevan added a comment - Patch is minor..but important one incase we provide pluggability for the loadbalancer.
        Hide
        Ted Yu added a comment -

        For AssignmentManager.setBalancer(), there is whitespace in javadoc.
        'balancer to set' should be on the same line as @param line.

        w.r.t. introducing TreeMap, I would prefer to see the benefit in a real world scenario. I think HashMap serves us well currently.

        Show
        Ted Yu added a comment - For AssignmentManager.setBalancer(), there is whitespace in javadoc. 'balancer to set' should be on the same line as @param line. w.r.t. introducing TreeMap, I would prefer to see the benefit in a real world scenario. I think HashMap serves us well currently.
        Hide
        ramkrishna.s.vasudevan added a comment -

        @Zhihong
        Thanks for your review.
        If you see the code base where we deal with the regions assignment like the

          private final NavigableMap<ServerName, Set<HRegionInfo>> servers =
            new TreeMap<ServerName, Set<HRegionInfo>>();
        
          private final SortedMap<HRegionInfo, ServerName> regions =
            new TreeMap<HRegionInfo, ServerName>();
        

        We use treemap. Here in getAssignmentsByTable we try to give the table to region, server mapping.
        The places where we do rebuildUserRegions also we try to maintain the order as retrieved from the meta.
        Here also we try to iterate the 'servers' tree map and form the result. Hence i felt using a tree map will maintain some consistency.
        And for some real world scenario, if i have a pair of tables may be named TableA and TableA_xxxx then when i want to do a balancing for these two tables thro an extended balancer there is a chance that TableA_xxxx comes first and then TableA. But this use case is very specific.

        Show
        ramkrishna.s.vasudevan added a comment - @Zhihong Thanks for your review. If you see the code base where we deal with the regions assignment like the private final NavigableMap<ServerName, Set<HRegionInfo>> servers = new TreeMap<ServerName, Set<HRegionInfo>>(); private final SortedMap<HRegionInfo, ServerName> regions = new TreeMap<HRegionInfo, ServerName>(); We use treemap. Here in getAssignmentsByTable we try to give the table to region, server mapping. The places where we do rebuildUserRegions also we try to maintain the order as retrieved from the meta. Here also we try to iterate the 'servers' tree map and form the result. Hence i felt using a tree map will maintain some consistency. And for some real world scenario, if i have a pair of tables may be named TableA and TableA_xxxx then when i want to do a balancing for these two tables thro an extended balancer there is a chance that TableA_xxxx comes first and then TableA. But this use case is very specific.
        Hide
        ramkrishna.s.vasudevan added a comment - - edited

        Pls review the latest one. If you feel still TreeMap change is not needed, i think the other changes are needed.

        {edit}
        Pls review the latest one. If you feel still TreeMap change is not needed, i think the other changes are needed like
            this.balancer.setMasterServices(this);
        
        

        This is needed because as part of join cluster we call assign(). If we extend the load balancer and want to use the masterservices we will not be able to use it. Hence the change is needed.{edit}
        Show
        ramkrishna.s.vasudevan added a comment - - edited Pls review the latest one. If you feel still TreeMap change is not needed, i think the other changes are needed. {edit} Pls review the latest one. If you feel still TreeMap change is not needed, i think the other changes are needed like this .balancer.setMasterServices( this ); This is needed because as part of join cluster we call assign(). If we extend the load balancer and want to use the masterservices we will not be able to use it. Hence the change is needed.{edit}
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12522135/HBASE-5737.patch
        against trunk revision .

        +1 @author. The patch does not contain any @author tags.

        -1 tests included. The patch doesn't appear to include any new or modified tests.
        Please justify why no new tests are needed for this patch.
        Also please list what manual steps were performed to verify this patch.

        +1 javadoc. The javadoc tool did not generate any warning messages.

        +1 javac. The applied patch does not increase the total number of javac compiler warnings.

        +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings.

        +1 release audit. The applied patch does not increase the total number of release audit warnings.

        +1 core tests. The patch passed unit tests in .

        Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/1465//testReport/
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/1465//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html
        Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/1465//console

        This message is automatically generated.

        Show
        Hadoop QA added a comment - -1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12522135/HBASE-5737.patch against trunk revision . +1 @author. The patch does not contain any @author tags. -1 tests included. The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. +1 core tests. The patch passed unit tests in . Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/1465//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/1465//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/1465//console This message is automatically generated.
        Hide
        Ted Yu added a comment -

        There is one more whitespace @ line 2986.
        Please keep HashMap for now. For the case of two tables cited above, there is time limit on the actual region movement per balance() call. Meaning the balancing of the two tables may be completed in several iterations.

        Show
        Ted Yu added a comment - There is one more whitespace @ line 2986. Please keep HashMap for now. For the case of two tables cited above, there is time limit on the actual region movement per balance() call. Meaning the balancing of the two tables may be completed in several iterations.
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12522142/HBASE-5737_1.patch
        against trunk revision .

        +1 @author. The patch does not contain any @author tags.

        -1 tests included. The patch doesn't appear to include any new or modified tests.
        Please justify why no new tests are needed for this patch.
        Also please list what manual steps were performed to verify this patch.

        +1 javadoc. The javadoc tool did not generate any warning messages.

        +1 javac. The applied patch does not increase the total number of javac compiler warnings.

        +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings.

        +1 release audit. The applied patch does not increase the total number of release audit warnings.

        -1 core tests. The patch failed these unit tests:
        org.apache.hadoop.hbase.master.TestAssignmentManager

        Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/1467//testReport/
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/1467//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html
        Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/1467//console

        This message is automatically generated.

        Show
        Hadoop QA added a comment - -1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12522142/HBASE-5737_1.patch against trunk revision . +1 @author. The patch does not contain any @author tags. -1 tests included. The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. -1 core tests. The patch failed these unit tests: org.apache.hadoop.hbase.master.TestAssignmentManager Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/1467//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/1467//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/1467//console This message is automatically generated.
        Hide
        ramkrishna.s.vasudevan added a comment -

        Ok.. I will do that Ted. Thanks for your review.
        By today evening i will upload the patch.

        Show
        ramkrishna.s.vasudevan added a comment - Ok.. I will do that Ted. Thanks for your review. By today evening i will upload the patch.
        Hide
        ramkrishna.s.vasudevan added a comment -

        Updated patch.

        Show
        ramkrishna.s.vasudevan added a comment - Updated patch.
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12522284/HBASE-5737_2.patch
        against trunk revision .

        +1 @author. The patch does not contain any @author tags.

        -1 tests included. The patch doesn't appear to include any new or modified tests.
        Please justify why no new tests are needed for this patch.
        Also please list what manual steps were performed to verify this patch.

        +1 javadoc. The javadoc tool did not generate any warning messages.

        +1 javac. The applied patch does not increase the total number of javac compiler warnings.

        -1 findbugs. The patch appears to introduce 1 new Findbugs (version 1.3.9) warnings.

        +1 release audit. The applied patch does not increase the total number of release audit warnings.

        -1 core tests. The patch failed these unit tests:
        org.apache.hadoop.hbase.master.TestSplitLogManager

        Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/1480//testReport/
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/1480//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html
        Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/1480//console

        This message is automatically generated.

        Show
        Hadoop QA added a comment - -1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12522284/HBASE-5737_2.patch against trunk revision . +1 @author. The patch does not contain any @author tags. -1 tests included. The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. -1 findbugs. The patch appears to introduce 1 new Findbugs (version 1.3.9) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. -1 core tests. The patch failed these unit tests: org.apache.hadoop.hbase.master.TestSplitLogManager Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/1480//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/1480//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/1480//console This message is automatically generated.
        Hide
        ramkrishna.s.vasudevan added a comment -

        TestSplitLogManager passes locally.

        Show
        ramkrishna.s.vasudevan added a comment - TestSplitLogManager passes locally.
        Hide
        stack added a comment -

        Ram, the AM#setBalancer is not right? Doesn't AM make a balancer instance of its own up in its constructor? We should at least remove that. Could we pass in the load balancer to use into the AM's constructor rather than call a setBalancer method?

        Show
        stack added a comment - Ram, the AM#setBalancer is not right? Doesn't AM make a balancer instance of its own up in its constructor? We should at least remove that. Could we pass in the load balancer to use into the AM's constructor rather than call a setBalancer method?
        Hide
        ramkrishna.s.vasudevan added a comment -

        @Stack
        I prefer passing as constructor but that needs changes in many places.
        Actually i had 2 ways of doing this. But the TestAssignmentManager was failing if we remove the balancer in the AssignmentManager.

        Though the changes are many passing in constructor should be fine.

        Show
        ramkrishna.s.vasudevan added a comment - @Stack I prefer passing as constructor but that needs changes in many places. Actually i had 2 ways of doing this. But the TestAssignmentManager was failing if we remove the balancer in the AssignmentManager. Though the changes are many passing in constructor should be fine.
        Hide
        ramkrishna.s.vasudevan added a comment -

        Latest patch as per Stack's suggestion.

        Show
        ramkrishna.s.vasudevan added a comment - Latest patch as per Stack's suggestion.
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12522713/HBASE-5737_3.patch
        against trunk revision .

        +1 @author. The patch does not contain any @author tags.

        +1 tests included. The patch appears to include 3 new or modified tests.

        +1 javadoc. The javadoc tool did not generate any warning messages.

        +1 javac. The applied patch does not increase the total number of javac compiler warnings.

        -1 findbugs. The patch appears to introduce 3 new Findbugs (version 1.3.9) warnings.

        +1 release audit. The applied patch does not increase the total number of release audit warnings.

        -1 core tests. The patch failed these unit tests:

        Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/1533//testReport/
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/1533//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html
        Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/1533//console

        This message is automatically generated.

        Show
        Hadoop QA added a comment - -1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12522713/HBASE-5737_3.patch against trunk revision . +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 3 new or modified tests. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. -1 findbugs. The patch appears to introduce 3 new Findbugs (version 1.3.9) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. -1 core tests. The patch failed these unit tests: Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/1533//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/1533//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/1533//console This message is automatically generated.
        Hide
        Ted Yu added a comment -

        Latest patch is good.
        Please update Fix Version/s.

        Show
        Ted Yu added a comment - Latest patch is good. Please update Fix Version/s.
        Hide
        stack added a comment -

        I think this is weird '+ this.balancer.setMasterServices(this);' but its not your change.

        +1 on commit.

        Show
        stack added a comment - I think this is weird '+ this.balancer.setMasterServices(this);' but its not your change. +1 on commit.
        Hide
        ramkrishna.s.vasudevan added a comment -

        Can this be an improvement or bug? I think the 'balancer' usage in HMaster and AM was a bug right?

        Show
        ramkrishna.s.vasudevan added a comment - Can this be an improvement or bug? I think the 'balancer' usage in HMaster and AM was a bug right?
        Hide
        stack added a comment -

        @Ram I do not follow. Please rephrase.

        Show
        stack added a comment - @Ram I do not follow. Please rephrase.
        Hide
        ramkrishna.s.vasudevan added a comment - - edited

        @Stack

        -    this.balancer = LoadBalancerFactory.getLoadBalancer(conf);
        +    this.balancer = balancer;
        

        The above change seems to a bug to me. That is why i asked whether it is an improvement or bug?

        Show
        ramkrishna.s.vasudevan added a comment - - edited @Stack - this .balancer = LoadBalancerFactory.getLoadBalancer(conf); + this .balancer = balancer; The above change seems to a bug to me. That is why i asked whether it is an improvement or bug?
        Hide
        stack added a comment -

        The above is from AM? If so, I'm not sure it a bug. At the time, my sense is that the balancer ran w/o keeping context. Whats changed is that you seem to have a LB that is doing this now.

        As to whether a bug or improvement, its your call boss.

        Show
        stack added a comment - The above is from AM? If so, I'm not sure it a bug. At the time, my sense is that the balancer ran w/o keeping context. Whats changed is that you seem to have a LB that is doing this now. As to whether a bug or improvement, its your call boss.
        Hide
        ramkrishna.s.vasudevan added a comment -

        @Stack
        Yes i have a configured LB. But as we provide option to use master services in the LB and now if i try to use the 'balancer' object there, it is a new one. I am updating it to a bug Stack.

        Show
        ramkrishna.s.vasudevan added a comment - @Stack Yes i have a configured LB. But as we provide option to use master services in the LB and now if i try to use the 'balancer' object there, it is a new one. I am updating it to a bug Stack.
        Hide
        ramkrishna.s.vasudevan added a comment -

        Committed to trunk and 0.94.
        Thanks Stack and Ted for your reviews.

        Show
        ramkrishna.s.vasudevan added a comment - Committed to trunk and 0.94. Thanks Stack and Ted for your reviews.
        Hide
        Hudson added a comment -

        Integrated in HBase-0.94 #134 (See https://builds.apache.org/job/HBase-0.94/134/)
        HBASE-5737 Minor Improvements related to balancer. (Ram) (Revision 1328063)

        Result = SUCCESS
        ramkrishna :
        Files :

        • /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java
        • /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/master/HMaster.java
        • /hbase/branches/0.94/src/test/java/org/apache/hadoop/hbase/master/TestAssignmentManager.java
        Show
        Hudson added a comment - Integrated in HBase-0.94 #134 (See https://builds.apache.org/job/HBase-0.94/134/ ) HBASE-5737 Minor Improvements related to balancer. (Ram) (Revision 1328063) Result = SUCCESS ramkrishna : Files : /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/master/HMaster.java /hbase/branches/0.94/src/test/java/org/apache/hadoop/hbase/master/TestAssignmentManager.java
        Hide
        Hudson added a comment -

        Integrated in HBase-TRUNK #2787 (See https://builds.apache.org/job/HBase-TRUNK/2787/)
        HBASE-5737 Minor Improvements related to balancer. (Ram) (Revision 1328057)

        Result = FAILURE
        ramkrishna :
        Files :

        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/master/HMaster.java
        • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/master/TestAssignmentManager.java
        Show
        Hudson added a comment - Integrated in HBase-TRUNK #2787 (See https://builds.apache.org/job/HBase-TRUNK/2787/ ) HBASE-5737 Minor Improvements related to balancer. (Ram) (Revision 1328057) Result = FAILURE ramkrishna : Files : /hbase/trunk/src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/master/HMaster.java /hbase/trunk/src/test/java/org/apache/hadoop/hbase/master/TestAssignmentManager.java
        Hide
        Hudson added a comment -

        Integrated in HBase-0.94-security #17 (See https://builds.apache.org/job/HBase-0.94-security/17/)
        HBASE-5737 Minor Improvements related to balancer. (Ram) (Revision 1328063)

        Result = FAILURE
        ramkrishna :
        Files :

        • /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java
        • /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/master/HMaster.java
        • /hbase/branches/0.94/src/test/java/org/apache/hadoop/hbase/master/TestAssignmentManager.java
        Show
        Hudson added a comment - Integrated in HBase-0.94-security #17 (See https://builds.apache.org/job/HBase-0.94-security/17/ ) HBASE-5737 Minor Improvements related to balancer. (Ram) (Revision 1328063) Result = FAILURE ramkrishna : Files : /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/master/HMaster.java /hbase/branches/0.94/src/test/java/org/apache/hadoop/hbase/master/TestAssignmentManager.java
        Hide
        Hudson added a comment -

        Integrated in HBase-TRUNK-security #176 (See https://builds.apache.org/job/HBase-TRUNK-security/176/)
        HBASE-5737 Minor Improvements related to balancer. (Ram) (Revision 1328057)

        Result = FAILURE
        ramkrishna :
        Files :

        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/master/HMaster.java
        • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/master/TestAssignmentManager.java
        Show
        Hudson added a comment - Integrated in HBase-TRUNK-security #176 (See https://builds.apache.org/job/HBase-TRUNK-security/176/ ) HBASE-5737 Minor Improvements related to balancer. (Ram) (Revision 1328057) Result = FAILURE ramkrishna : Files : /hbase/trunk/src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/master/HMaster.java /hbase/trunk/src/test/java/org/apache/hadoop/hbase/master/TestAssignmentManager.java

          People

          • Assignee:
            ramkrishna.s.vasudevan
            Reporter:
            ramkrishna.s.vasudevan
          • Votes:
            0 Vote for this issue
            Watchers:
            2 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development