HBase
  1. HBase
  2. HBASE-5510

Pass region info in LoadBalancer.randomAssignment(List<ServerName> servers)

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 0.92.0
    • Fix Version/s: 0.95.0
    • Component/s: None
    • Labels:
      None
    • Hadoop Flags:
      Reviewed

      Description

      In LB there is randomAssignment(List<ServerName servers>) API which will be used by AM to assign
      a region from a down RS. [This will be also used in other cases like call to assign() API from client]
      I feel it would be better to pass the HRegionInfo also into this method. When the LB making a choice for a region
      assignment, when one RS is down, it would be nice that the LB knows for which region it is doing this server selection.

      Scenario
      While one RS down, we wanted the regions to get moved to other RSs but a set of regions stay together. We are having custom load balancer but with the current way of LB interface this is not possible. Another way is I can allow a random assignment of the regions at the RS down time. Later with a cluster balance I can balance the regions as I need. But this might make regions assign 1st to one RS and then again move to another. Also for some time period my business use case can not get satisfied.

      Also I have seen some issue in JIRA which speaks about making sure that Root and META regions always sit in some specific RSs. With the current LB API this wont be possible in future.

      1. HBase-5010_3.patch
        2 kB
        Anoop Sam John
      2. HBase-5510_2.patch
        2 kB
        Anoop Sam John
      3. HBase-5510.patch
        2 kB
        Anoop Sam John

        Activity

        Anoop Sam John created issue -
        ramkrishna.s.vasudevan made changes -
        Field Original Value New Value
        Assignee ramkrishna.s.vasudevan [ ram_krish ]
        Hide
        ramkrishna.s.vasudevan added a comment -

        @Ted

        If interface change is ok, I can make a patch for it.

        Show
        ramkrishna.s.vasudevan added a comment - @Ted If interface change is ok, I can make a patch for it.
        Hide
        Ted Yu added a comment -

        For getRegionPlan(), we already provide server to exclude:

          RegionPlan getRegionPlan(final RegionState state,
              final ServerName serverToExclude, final boolean forceNewPlan) {
        

        Does the above not serve the purpose of migrating region away ?

        but a set of regions stay together

        Can I get some explanation for the rationale here ?
        Load balancer should honor block locality when moving regions. Why should these regions stay together ?

        Changing interface is fine for TRUNK.

        But I want to understand the use case more.

        Show
        Ted Yu added a comment - For getRegionPlan(), we already provide server to exclude: RegionPlan getRegionPlan( final RegionState state, final ServerName serverToExclude, final boolean forceNewPlan) { Does the above not serve the purpose of migrating region away ? but a set of regions stay together Can I get some explanation for the rationale here ? Load balancer should honor block locality when moving regions. Why should these regions stay together ? Changing interface is fine for TRUNK. But I want to understand the use case more.
        Hide
        ramkrishna.s.vasudevan added a comment -

        @Ted
        The rationale is like,
        I have set of regions S1...Sn and R1...Rn...In this i expect S1-R1, S2-R2 ...to be colocated in the same RS.

        S1...Sn are balanced by one LB.
        There is a custom LB which will balance R1..Rn. Now this LB should get the assignment done by first LB and based on that it will assign R1..Rn. Finally ensuring the colocation.
        Am i clear Ted?

        Show
        ramkrishna.s.vasudevan added a comment - @Ted The rationale is like, I have set of regions S1...Sn and R1...Rn...In this i expect S1-R1, S2-R2 ...to be colocated in the same RS. S1...Sn are balanced by one LB. There is a custom LB which will balance R1..Rn. Now this LB should get the assignment done by first LB and based on that it will assign R1..Rn. Finally ensuring the colocation. Am i clear Ted?
        Hide
        Ted Yu added a comment -

        Should colocation be satisfied by randomAssignment() ?

        By the time of assigning regions R1 to Rn, one of more of S1 to Sn may have been moved to new location (manual operation, server downtime, etc). How do we address this scenario ?

        The HRegionInfo Anoop suggested passing to randomAssignment() is that of the region to be assigned, how do we retrieve its buddy region's information ?

        Thanks

        Show
        Ted Yu added a comment - Should colocation be satisfied by randomAssignment() ? By the time of assigning regions R1 to Rn, one of more of S1 to Sn may have been moved to new location (manual operation, server downtime, etc). How do we address this scenario ? The HRegionInfo Anoop suggested passing to randomAssignment() is that of the region to be assigned, how do we retrieve its buddy region's information ? Thanks
        Hide
        Anoop Sam John added a comment -

        @Ted
        Thanks for your reply. R1-S1 mapping can be found out using the start and end keys of both these regions. Our design will be so that whenever we see S1 when will be able to judge which R1 is its buddy.. am I clear to you? But now the issue is randomAssignment() will not reveal the region for which the selection is happening now. If we can get this we will be able to do our business scenario using our LB..

        @Ram : Thanks for the explanation comment.

        Thanks

        Show
        Anoop Sam John added a comment - @Ted Thanks for your reply. R1-S1 mapping can be found out using the start and end keys of both these regions. Our design will be so that whenever we see S1 when will be able to judge which R1 is its buddy.. am I clear to you? But now the issue is randomAssignment() will not reveal the region for which the selection is happening now. If we can get this we will be able to do our business scenario using our LB.. @Ram : Thanks for the explanation comment. Thanks
        Hide
        Ted Yu added a comment -

        Can I see the patch ?

        Show
        Ted Yu added a comment - Can I see the patch ?
        Anoop Sam John made changes -
        Attachment HBase-5510.patch [ 12516934 ]
        Hide
        Anoop Sam John added a comment -

        Pls see the patch.

        Show
        Anoop Sam John added a comment - Pls see the patch.
        Hide
        Ted Yu added a comment -

        Looking at other methods in LoadBalancer, the regions are passed as first parameter.
        I think randomAssignment() should follow this convention.

        Please wrap long lines in the patch.

        Currently LoadBalancer is marked @InterfaceAudience.Private
        I think the use case presented in this JIRA clearly means it shouldn't be private.
        Please change it to @InterfaceAudience.Public in the next patch.

        Show
        Ted Yu added a comment - Looking at other methods in LoadBalancer, the regions are passed as first parameter. I think randomAssignment() should follow this convention. Please wrap long lines in the patch. Currently LoadBalancer is marked @InterfaceAudience.Private I think the use case presented in this JIRA clearly means it shouldn't be private. Please change it to @InterfaceAudience.Public in the next patch.
        Anoop Sam John made changes -
        Attachment HBase-5510_2.patch [ 12516978 ]
        Hide
        Anoop Sam John added a comment -

        @Ted
        Addressed your comments in the new patch.
        Thanks

        Show
        Anoop Sam John added a comment - @Ted Addressed your comments in the new patch. Thanks
        Hide
        ramkrishna.s.vasudevan added a comment -

        The patch looks good to me.
        @Ted
        Do you want to take a look at the updated patch?

        Show
        ramkrishna.s.vasudevan added a comment - The patch looks good to me. @Ted Do you want to take a look at the updated patch?
        Ted Yu made changes -
        Hadoop Flags Reviewed [ 10343 ]
        Fix Version/s 0.96.0 [ 12320040 ]
        Ted Yu made changes -
        Status Open [ 1 ] Patch Available [ 10002 ]
        Hide
        Ted Yu added a comment -

        Patch v2 looks good.

        Minor comments:

        +      balancer.randomAssignment(state.getRegion(),servers));
        

        A space is needed between comma and servers.

        +  public ServerName randomAssignment(HRegionInfo regionInfo,List<ServerName> servers);
        

        Wrap long line, please.

        Show
        Ted Yu added a comment - Patch v2 looks good. Minor comments: + balancer.randomAssignment(state.getRegion(),servers)); A space is needed between comma and servers. + public ServerName randomAssignment(HRegionInfo regionInfo,List<ServerName> servers); Wrap long line, please.
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12516978/HBase-5510_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 appears to have generated -129 warning messages.

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

        -1 findbugs. The patch appears to introduce 154 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.mapreduce.TestImportTsv
        org.apache.hadoop.hbase.mapred.TestTableMapReduce
        org.apache.hadoop.hbase.mapreduce.TestHFileOutputFormat

        Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/1089//testReport/
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/1089//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html
        Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/1089//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/12516978/HBase-5510_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 appears to have generated -129 warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. -1 findbugs. The patch appears to introduce 154 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.mapreduce.TestImportTsv org.apache.hadoop.hbase.mapred.TestTableMapReduce org.apache.hadoop.hbase.mapreduce.TestHFileOutputFormat Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/1089//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/1089//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/1089//console This message is automatically generated.
        Anoop Sam John made changes -
        Attachment HBase-5010_3.patch [ 12517004 ]
        Hide
        Ted Yu added a comment -

        +1 on patch v3

        I will integrate to TRUNK Monday afternoon if there is no objection.

        Show
        Ted Yu added a comment - +1 on patch v3 I will integrate to TRUNK Monday afternoon if there is no objection.
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12517004/HBase-5010_3.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 appears to have generated -129 warning messages.

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

        -1 findbugs. The patch appears to introduce 154 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.mapreduce.TestImportTsv
        org.apache.hadoop.hbase.mapred.TestTableMapReduce
        org.apache.hadoop.hbase.mapreduce.TestHFileOutputFormat

        Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/1092//testReport/
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/1092//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html
        Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/1092//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/12517004/HBase-5010_3.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 appears to have generated -129 warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. -1 findbugs. The patch appears to introduce 154 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.mapreduce.TestImportTsv org.apache.hadoop.hbase.mapred.TestTableMapReduce org.apache.hadoop.hbase.mapreduce.TestHFileOutputFormat Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/1092//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/1092//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/1092//console This message is automatically generated.
        Ted Yu made changes -
        Summary Change in LB.randomAssignment(List<ServerName> servers) API Pass region info in LoadBalancer.randomAssignment(List<ServerName> servers)
        Hide
        Ted Yu added a comment -

        Integrated to TRUNK.

        Thanks for the patch Anoop.

        Show
        Ted Yu added a comment - Integrated to TRUNK. Thanks for the patch Anoop.
        Ted Yu made changes -
        Assignee ramkrishna.s.vasudevan [ ram_krish ] Anoop Sam John [ anoopsamjohn ]
        ramkrishna.s.vasudevan made changes -
        Status Patch Available [ 10002 ] Resolved [ 5 ]
        Resolution Fixed [ 1 ]
        stack made changes -
        Fix Version/s 0.95.0 [ 12324094 ]
        Fix Version/s 0.96.0 [ 12320040 ]
        stack made changes -
        Fix Version/s 0.98.0 [ 12323143 ]
        stack made changes -
        Fix Version/s 0.98.0 [ 12323143 ]
        Hide
        stack added a comment -

        Marking closed.

        Show
        stack added a comment - Marking closed.
        stack made changes -
        Status Resolved [ 5 ] Closed [ 6 ]

          People

          • Assignee:
            Anoop Sam John
            Reporter:
            Anoop Sam John
          • Votes:
            0 Vote for this issue
            Watchers:
            4 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development