Uploaded image for project: 'HBase'
  1. HBase
  2. HBASE-3681

Check the sloppiness of the region load before balancing

    Details

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

      Description

      Per our discussion at the hackathon today, it seems that it would be more helpful to add a sloppiness check before doing the normal balancing.

      The current situation is that the balancer always tries to get the region load even, meaning that there can be some very frequent regions movement.

      Setting the balancer to run less often (like every 4 hours) isn't much better since the load could get out of whack easily.

      This is why running the normal balancer frequently, but first checking for some sloppiness in the region load across the RS, seems like a more viable option.

      1. hbase-3681.txt
        8 kB
        Ted Yu
      2. hbase-3681-v2.txt
        5 kB
        Ted Yu
      3. hbase-3681-v3.txt
        5 kB
        Ted Yu

        Activity

        Hide
        lars_francke Lars Francke added a comment -

        This issue was closed as part of a bulk closing operation on 2015-11-20. All issues that have been resolved and where all fixVersions have been released have been closed (following discussions on the mailing list).

        Show
        lars_francke Lars Francke added a comment - This issue was closed as part of a bulk closing operation on 2015-11-20. All issues that have been resolved and where all fixVersions have been released have been closed (following discussions on the mailing list).
        Hide
        hudson Hudson added a comment -

        Integrated in HBase-TRUNK #1814 (See https://hudson.apache.org/hudson/job/HBase-TRUNK/1814/)

        Show
        hudson Hudson added a comment - Integrated in HBase-TRUNK #1814 (See https://hudson.apache.org/hudson/job/HBase-TRUNK/1814/ )
        Hide
        jdcryans Jean-Daniel Cryans added a comment -

        Committed to branch (slop=0) and trunk (slop=0.2), thanks for the good work Ted!

        Show
        jdcryans Jean-Daniel Cryans added a comment - Committed to branch (slop=0) and trunk (slop=0.2), thanks for the good work Ted!
        Hide
        yuzhihong@gmail.com Ted Yu added a comment -

        Thanks for review J-D

        Show
        yuzhihong@gmail.com Ted Yu added a comment - Thanks for review J-D
        Hide
        jdcryans Jean-Daniel Cryans added a comment -

        +1 I like this new version, I'm going to fix the following nits on commit if you don't mind:

        • "balance if regionserver has average + (average * slop) regions" should actually refer to all region servers (it sounds like it is on a per RS basis)
        • the default value in the code isn't the same as in hbase-default.xml, I'm going to put a different value both branches anyways
        Show
        jdcryans Jean-Daniel Cryans added a comment - +1 I like this new version, I'm going to fix the following nits on commit if you don't mind: "balance if regionserver has average + (average * slop) regions" should actually refer to all region servers (it sounds like it is on a per RS basis) the default value in the code isn't the same as in hbase-default.xml, I'm going to put a different value both branches anyways
        Hide
        yuzhihong@gmail.com Ted Yu added a comment -

        Made changes according to review comments.

        Show
        yuzhihong@gmail.com Ted Yu added a comment - Made changes according to review comments.
        Hide
        jdcryans Jean-Daniel Cryans added a comment -

        Some comments:

        • I think the ifs in the new constructor aren't written consistently with the rest of the code. Usually we either do brackets or one-line ifs and this is neither.
        • I wouldn't reuse the min/max since they don't have the same meaning.
        • The comment "so that behavior is consistent with that of 0.90.1." is true but I don't think it belongs in there, since it will go stale very fast. Just saying that here is enough IMO.
        • I think for 0.92 we could set the slop to something like 0.1 or 0.2, but keep it at 0 for 0.90.1. Regarding that, Stack moved this issue to 0.92 but he agreed we can move it back into 0.90.2 since 1) we need it at SU and 2) it's going to be disabled by default.
        Show
        jdcryans Jean-Daniel Cryans added a comment - Some comments: I think the ifs in the new constructor aren't written consistently with the rest of the code. Usually we either do brackets or one-line ifs and this is neither. I wouldn't reuse the min/max since they don't have the same meaning. The comment "so that behavior is consistent with that of 0.90.1." is true but I don't think it belongs in there, since it will go stale very fast. Just saying that here is enough IMO. I think for 0.92 we could set the slop to something like 0.1 or 0.2, but keep it at 0 for 0.90.1. Regarding that, Stack moved this issue to 0.92 but he agreed we can move it back into 0.90.2 since 1) we need it at SU and 2) it's going to be disabled by default.
        Hide
        stack stack added a comment -

        Marking patch available

        Show
        stack stack added a comment - Marking patch available
        Hide
        yuzhihong@gmail.com Ted Yu added a comment -

        Second version.
        TestLoadBalancer, TestRegionRebalancing, TestMultiParallel and TestMasterObserver pass.
        Changed default slop to 0 so that behavior is consistent with 0.90.1

        Show
        yuzhihong@gmail.com Ted Yu added a comment - Second version. TestLoadBalancer, TestRegionRebalancing, TestMultiParallel and TestMasterObserver pass. Changed default slop to 0 so that behavior is consistent with 0.90.1
        Hide
        streamy Jonathan Gray added a comment -

        For unit tests which expect the old behavior, they should set the sloppiness configuration to 0 (and we might make that the default value as well, since this change in behavior could be seen as less optimal in some cases).

        Also, why do you do an additional -1 for the min? If it was left as floor, then setting sloppiness to 0 would yield exactly the same behavior as we have today.

        Show
        streamy Jonathan Gray added a comment - For unit tests which expect the old behavior, they should set the sloppiness configuration to 0 (and we might make that the default value as well, since this change in behavior could be seen as less optimal in some cases). Also, why do you do an additional -1 for the min? If it was left as floor, then setting sloppiness to 0 would yield exactly the same behavior as we have today.
        Hide
        jdcryans Jean-Daniel Cryans added a comment -

        This is the behavior we're trying to get to. The current problem is that every time the master balances you'll have some disruption. You could instead set the balancer to run less often, and have one big batch of regions moving at the same time while some region server may get really unbalanced. Since the time to rebalance 5 regions can easily be the same as rebalancing 50 regions, the disruption ends up being almost the same.

        To optimize that situation, we want to keep running the balancer often but first check against some sloppiness.

        Show
        jdcryans Jean-Daniel Cryans added a comment - This is the behavior we're trying to get to. The current problem is that every time the master balances you'll have some disruption. You could instead set the balancer to run less often, and have one big batch of regions moving at the same time while some region server may get really unbalanced. Since the time to rebalance 5 regions can easily be the same as rebalancing 50 regions, the disruption ends up being almost the same. To optimize that situation, we want to keep running the balancer often but first check against some sloppiness.
        Hide
        yuzhihong@gmail.com Ted Yu added a comment -

        The above approach may surprise some users who would expect sloppiness envelope to be respected after balancing is done.
        It also leads to burst of region re-assignment from one balancing to the next.
        It also makes unit testing a little complicated.

        Show
        yuzhihong@gmail.com Ted Yu added a comment - The above approach may surprise some users who would expect sloppiness envelope to be respected after balancing is done. It also leads to burst of region re-assignment from one balancing to the next. It also makes unit testing a little complicated.
        Hide
        jdcryans Jean-Daniel Cryans added a comment -

        Ah I see, then I think it would be better to create a constructor for LoadBalancer and pass the HBC.

        Show
        jdcryans Jean-Daniel Cryans added a comment - Ah I see, then I think it would be better to create a constructor for LoadBalancer and pass the HBC.
        Hide
        yuzhihong@gmail.com Ted Yu added a comment -

        LoadBalancer currently doesn't have access to Configuration. That's why slop config is passed from HMaster.

        I will submit another patch which checks against sloppiness envelope. If any of the servers is outside the envelope, the current balancing code will run. See following:

            float average = (float)numRegions / numServers; // for logging
            int min = (int) Math.floor(average * (1 - slop));
            if (min > 0) min -= 1;
            int max = (int) Math.ceil(average * (1 + slop));
            
            if(serversByLoad.lastKey().getLoad().getNumberOfRegions() <= max &&
               serversByLoad.firstKey().getLoad().getNumberOfRegions() >= min) {
              // Skipped because no server outside (min,max) range
              LOG.info("Skipping load balancing.  servers=" + numServers + " " +
                  "regions=" + numRegions + " average=" + average + " " +
                  "mostloaded=" + serversByLoad.lastKey().getLoad().getNumberOfRegions() +
                  " leastloaded=" + serversByLoad.firstKey().getLoad().getNumberOfRegions());
              return null;
            }
            min = numRegions / numServers;
            max = numRegions % numServers == 0 ? min : min + 1;
        
        Show
        yuzhihong@gmail.com Ted Yu added a comment - LoadBalancer currently doesn't have access to Configuration. That's why slop config is passed from HMaster. I will submit another patch which checks against sloppiness envelope. If any of the servers is outside the envelope, the current balancing code will run. See following: float average = ( float )numRegions / numServers; // for logging int min = ( int ) Math .floor(average * (1 - slop)); if (min > 0) min -= 1; int max = ( int ) Math .ceil(average * (1 + slop)); if (serversByLoad.lastKey().getLoad().getNumberOfRegions() <= max && serversByLoad.firstKey().getLoad().getNumberOfRegions() >= min) { // Skipped because no server outside (min,max) range LOG.info( "Skipping load balancing. servers=" + numServers + " " + "regions=" + numRegions + " average=" + average + " " + "mostloaded=" + serversByLoad.lastKey().getLoad().getNumberOfRegions() + " leastloaded=" + serversByLoad.firstKey().getLoad().getNumberOfRegions()); return null ; } min = numRegions / numServers; max = numRegions % numServers == 0 ? min : min + 1;
        Hide
        jdcryans Jean-Daniel Cryans added a comment -

        I think there's a small misunderstanding of what the intention of this jira is.

        The Balancer should only run if any of the servers are outside of the sloppiness envelope, it's not on a per region server basis else we would just be delaying the problem. So, this patch really only needs to add a check before everything else to see where each region server is with regards to the slop factor, then make the decision of doing the balance to even out everything.

        Also I think the slop should be configured in AssignmentManager, since HMaster in your patch is really just passing it down.

        Show
        jdcryans Jean-Daniel Cryans added a comment - I think there's a small misunderstanding of what the intention of this jira is. The Balancer should only run if any of the servers are outside of the sloppiness envelope, it's not on a per region server basis else we would just be delaying the problem. So, this patch really only needs to add a check before everything else to see where each region server is with regards to the slop factor, then make the decision of doing the balance to even out everything. Also I think the slop should be configured in AssignmentManager, since HMaster in your patch is really just passing it down.
        Hide
        yuzhihong@gmail.com Ted Yu added a comment -

        Initial attempt.
        If slop is greater than 0, some tests fail.
        e.g.

        2011-03-21 22:18:44,886 INFO  [main] master.TestLoadBalancer(186): Mock Cluster : { 1 , 1 , 1123 , 133 , 138 , 12 , 1444 , 0 , 0 , 144 , 1 , 1 } [srvr=12 rgns=2998 avg=249.83333 max=250 min=249]
        2011-03-21 22:18:44,887 WARN  [main] master.LoadBalancer(322): regionidx=2027, regionsToMove=2027, numServers=12, serversOverloaded=2, serversUnderloaded=10
        2011-03-21 22:18:44,887 WARN  [main] master.LoadBalancer(332): Input 127.0.0.1,14173,-4503661418262151144 1, 127.0.0.1,16709,5660298165359772741 1, 127.0.0.1,16753,6033378928668609711 1123, 127.0.0.1,21997,-5339821095735763805 133, 127.0.0.1,25701,-2236437849271081337 138, 127.0.0.1,2735,5768301853710328270 12, 127.0.0.1,28795,1395900819118748885 1444, 127.0.0.1,41820,-7080037238717148768 0, 127.0.0.1,42315,-7850691226193331732 0, 127.0.0.1,55335,-5765170823713071701 144, 127.0.0.1,55580,-8175369880198350855 1, 127.0.0.1,8395,850167680932222284 1
        2011-03-21 22:18:44,887 INFO  [main] master.LoadBalancer(336): Calculated a load balance in 1ms. Moving 2027 regions off of 2 overloaded servers onto 10 less loaded servers
        2011-03-21 22:18:44,889 INFO  [main] master.TestLoadBalancer(189): Mock Balance : { 248 , 248 , 274 , 248 , 248 , 248 , 274 , 249 , 249 , 216 , 248 , 248 }
        
        Show
        yuzhihong@gmail.com Ted Yu added a comment - Initial attempt. If slop is greater than 0, some tests fail. e.g. 2011-03-21 22:18:44,886 INFO [main] master.TestLoadBalancer(186): Mock Cluster : { 1 , 1 , 1123 , 133 , 138 , 12 , 1444 , 0 , 0 , 144 , 1 , 1 } [srvr=12 rgns=2998 avg=249.83333 max=250 min=249] 2011-03-21 22:18:44,887 WARN [main] master.LoadBalancer(322): regionidx=2027, regionsToMove=2027, numServers=12, serversOverloaded=2, serversUnderloaded=10 2011-03-21 22:18:44,887 WARN [main] master.LoadBalancer(332): Input 127.0.0.1,14173,-4503661418262151144 1, 127.0.0.1,16709,5660298165359772741 1, 127.0.0.1,16753,6033378928668609711 1123, 127.0.0.1,21997,-5339821095735763805 133, 127.0.0.1,25701,-2236437849271081337 138, 127.0.0.1,2735,5768301853710328270 12, 127.0.0.1,28795,1395900819118748885 1444, 127.0.0.1,41820,-7080037238717148768 0, 127.0.0.1,42315,-7850691226193331732 0, 127.0.0.1,55335,-5765170823713071701 144, 127.0.0.1,55580,-8175369880198350855 1, 127.0.0.1,8395,850167680932222284 1 2011-03-21 22:18:44,887 INFO [main] master.LoadBalancer(336): Calculated a load balance in 1ms. Moving 2027 regions off of 2 overloaded servers onto 10 less loaded servers 2011-03-21 22:18:44,889 INFO [main] master.TestLoadBalancer(189): Mock Balance : { 248 , 248 , 274 , 248 , 248 , 248 , 274 , 249 , 249 , 216 , 248 , 248 }
        Hide
        jdcryans Jean-Daniel Cryans added a comment -

        Sounds good Ted.

        Show
        jdcryans Jean-Daniel Cryans added a comment - Sounds good Ted.
        Hide
        yuzhihong@gmail.com Ted Yu added a comment -

        I plan to put in this change:

            min = (int) Math.floor(average * (1 - slop)) - 1;
            max = (int) Math.ceil(average * (1 + slop));
        
        Show
        yuzhihong@gmail.com Ted Yu added a comment - I plan to put in this change: min = ( int ) Math .floor(average * (1 - slop)) - 1; max = ( int ) Math .ceil(average * (1 + slop));

          People

          • Assignee:
            yuzhihong@gmail.com Ted Yu
            Reporter:
            jdcryans Jean-Daniel Cryans
          • Votes:
            0 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development