Details

    • Hadoop Flags:
      Reviewed

      Description

      Path-1 add another mertic for HRegion to count request made to region.

      Path-2 add another command to hbase shell to grab all regions, sort by requests from Path-1 and move in round-robin algorithm to servers

      1. hbase-3507-high-scale.txt
        10 kB
        Ted Yu
      2. hbase-3507-v4.txt
        12 kB
        stack
      3. hbase-3507-split-merge.txt
        13 kB
        Ted Yu
      4. hbase-3507-splitTxn.txt
        16 kB
        Ted Yu
      5. hbase-requestsCount-v2.patch
        22 kB
        Sebastian Bauer
      6. hbase-requestsCount-2.patch
        3 kB
        Sebastian Bauer
      7. hbase-requestsCount.patch
        8 kB
        Sebastian Bauer

        Issue Links

          Activity

          Hide
          anih Sebastian Bauer added a comment -

          Path-1

          Show
          anih Sebastian Bauer added a comment - Path-1
          Hide
          anih Sebastian Bauer added a comment -

          Path-2

          Show
          anih Sebastian Bauer added a comment - Path-2
          Hide
          yuzhihong@gmail.com Ted Yu added a comment -

          In rebalance(), regions_table maps request count to region. However, we should consider regions with same number of requests - hash collision.
          Also, we should consider the current region assignment before calling admin.move()
          e.g. consider the top two most heavily read regions A (on RS1) and B (on RS2)
          If later requests for B get higher than requests for A, current patch would shuffle region A (to RS2) with region B - an unnecessary action.
          Using more complex logic, we should balance the sum of requests landed on each region server.

          Show
          yuzhihong@gmail.com Ted Yu added a comment - In rebalance(), regions_table maps request count to region. However, we should consider regions with same number of requests - hash collision. Also, we should consider the current region assignment before calling admin.move() e.g. consider the top two most heavily read regions A (on RS1) and B (on RS2) If later requests for B get higher than requests for A, current patch would shuffle region A (to RS2) with region B - an unnecessary action. Using more complex logic, we should balance the sum of requests landed on each region server.
          Hide
          ryanobjc ryan rawson added a comment -

          how frequently would you move regions? Moving a highly loaded region under load might be more disruptive than leaving where it is... Using a more sophisticated algorithm that uses a decaying moving average over some period of time seems like that might have a better impact. For example do you move a region because it gets hot for 30 seconds? 1 minute? 5 minutes? 10 minutes even? I'm not sure where the line is, but it seems like the goal should be to move regions for persistent high long term load, not transient spikes.

          Thoughts?

          Show
          ryanobjc ryan rawson added a comment - how frequently would you move regions? Moving a highly loaded region under load might be more disruptive than leaving where it is... Using a more sophisticated algorithm that uses a decaying moving average over some period of time seems like that might have a better impact. For example do you move a region because it gets hot for 30 seconds? 1 minute? 5 minutes? 10 minutes even? I'm not sure where the line is, but it seems like the goal should be to move regions for persistent high long term load, not transient spikes. Thoughts?
          Hide
          anih Sebastian Bauer added a comment -

          @Ted Yu: maybe average from all regionservers requests counters and if regionserver diverge from this average(+/- some percent) we move region that leverage this?

          @rayan rawson: this was needed only once after hadoop big crash when hbase cannot reassign regions to the same regionservers. In this form rebalance is not for frequently use

          Show
          anih Sebastian Bauer added a comment - @Ted Yu: maybe average from all regionservers requests counters and if regionserver diverge from this average(+/- some percent) we move region that leverage this? @rayan rawson: this was needed only once after hadoop big crash when hbase cannot reassign regions to the same regionservers. In this form rebalance is not for frequently use
          Hide
          yuzhihong@gmail.com Ted Yu added a comment -

          I agree.
          We may also provide number of read requests metric along side total number of requests.
          Write requests are currently get balanced in the form of split regions.
          We should establish different load balancing policies favoring:
          1. read requests
          2. write requests
          3. read and write requests

          Show
          yuzhihong@gmail.com Ted Yu added a comment - I agree. We may also provide number of read requests metric along side total number of requests. Write requests are currently get balanced in the form of split regions. We should establish different load balancing policies favoring: 1. read requests 2. write requests 3. read and write requests
          Hide
          anih Sebastian Bauer added a comment -

          how writes balancing properly? i think childrens from split can land in one regionserver? and what with increaments, this is special case because this type of writes doesn't trigger splits so often

          Show
          anih Sebastian Bauer added a comment - how writes balancing properly? i think childrens from split can land in one regionserver? and what with increaments, this is special case because this type of writes doesn't trigger splits so often
          Hide
          yuzhihong@gmail.com Ted Yu added a comment -

          What I did in HBASE-3373 is to rebalance newly split regions onto other region servers.

          Show
          yuzhihong@gmail.com Ted Yu added a comment - What I did in HBASE-3373 is to rebalance newly split regions onto other region servers.
          Hide
          yuzhihong@gmail.com Ted Yu added a comment -

          TestHeapSize would fail without the following change in HRegion:

            public static final long FIXED_OVERHEAD = ClassSize.align(
                (5 * Bytes.SIZEOF_LONG) +
          
          Show
          yuzhihong@gmail.com Ted Yu added a comment - TestHeapSize would fail without the following change in HRegion: public static final long FIXED_OVERHEAD = ClassSize.align( (5 * Bytes.SIZEOF_LONG) +
          Hide
          yuzhihong@gmail.com Ted Yu added a comment -

          In table.jsp, we should provide option to sort regions by request count.

          Show
          yuzhihong@gmail.com Ted Yu added a comment - In table.jsp, we should provide option to sort regions by request count.
          Hide
          anih Sebastian Bauer added a comment -

          what is a difference between FIXED_OVERHEAD and DEEP_OVERHEAD? because memstoreSize is in DEEP_OVERHEAD

          Show
          anih Sebastian Bauer added a comment - what is a difference between FIXED_OVERHEAD and DEEP_OVERHEAD? because memstoreSize is in DEEP_OVERHEAD
          Hide
          anih Sebastian Bauer added a comment -

          new patch have long requestsCount and changed FIXED_OVERHEAD to pass TestHeapSize

          Show
          anih Sebastian Bauer added a comment - new patch have long requestsCount and changed FIXED_OVERHEAD to pass TestHeapSize
          Hide
          yuzhihong@gmail.com Ted Yu added a comment -

          In preparation for HBASE-3616, I think we need to enhance this method in SplitTransaction:

           HRegion createDaughterRegion(final HRegionInfo hri,
               final FlushRequester flusher)
          

          requestsCount is initially 0 for daughter regions which doesn't reflect the fact that parent region has been heavily accessed.
          Maybe we can assign half of requestsCount of parent region to each daughter region ?

          Show
          yuzhihong@gmail.com Ted Yu added a comment - In preparation for HBASE-3616 , I think we need to enhance this method in SplitTransaction: HRegion createDaughterRegion( final HRegionInfo hri, final FlushRequester flusher) requestsCount is initially 0 for daughter regions which doesn't reflect the fact that parent region has been heavily accessed. Maybe we can assign half of requestsCount of parent region to each daughter region ?
          Hide
          yuzhihong@gmail.com Ted Yu added a comment -

          Here is the patch that removes balancing portion and adds handling of region split/merge.

          Show
          yuzhihong@gmail.com Ted Yu added a comment - Here is the patch that removes balancing portion and adds handling of region split/merge.
          Hide
          stack stack added a comment -

          @Ted How does your patch relate to Sebastians?

          Show
          stack stack added a comment - @Ted How does your patch relate to Sebastians?
          Hide
          stack stack added a comment -

          @Ted On splitting requests between daughters, that sounds good.

          Show
          stack stack added a comment - @Ted On splitting requests between daughters, that sounds good.
          Hide
          yuzhihong@gmail.com Ted Yu added a comment -

          My patch is based on Sebastian's patch
          I removed the balancing script and added handling of region split and merge.

          Show
          yuzhihong@gmail.com Ted Yu added a comment - My patch is based on Sebastian's patch I removed the balancing script and added handling of region split and merge.
          Hide
          stack stack added a comment -

          Seems odd that on construction you'd pass in requests given as you are creating the region; how can a region have 'requests' if its only being created. Can't you use a setRequests function? If you didn't take requests in the constructor, it looks like your patch would be half the size.

          Otherwise patch looks good Ted.

          Show
          stack stack added a comment - Seems odd that on construction you'd pass in requests given as you are creating the region; how can a region have 'requests' if its only being created. Can't you use a setRequests function? If you didn't take requests in the constructor, it looks like your patch would be half the size. Otherwise patch looks good Ted.
          Hide
          yuzhihong@gmail.com Ted Yu added a comment -

          I thought of adding setRequests() but didn't go that route.
          There're two reasons:
          1. Request for region comes in one by one normally. The setter isn't used.
          2. When region splits, the request count goes half/half to daughter regions. It is intuitive to assign the count in ctor.
          I will upload another version which removes unnecessary changes in unit tests.

          BTW unit tests pass on my Mac.

          Show
          yuzhihong@gmail.com Ted Yu added a comment - I thought of adding setRequests() but didn't go that route. There're two reasons: 1. Request for region comes in one by one normally. The setter isn't used. 2. When region splits, the request count goes half/half to daughter regions. It is intuitive to assign the count in ctor. I will upload another version which removes unnecessary changes in unit tests. BTW unit tests pass on my Mac.
          Hide
          yuzhihong@gmail.com Ted Yu added a comment -

          Removed trivial changes in unit tests.

          Show
          yuzhihong@gmail.com Ted Yu added a comment - Removed trivial changes in unit tests.
          Hide
          stack stack added a comment -

          Hey Ted, a package private setRequests seems to be the way to go rather than pass in on construction just for the case of passing a request count on daughter split. I can do it on commit if you want?

          Show
          stack stack added a comment - Hey Ted, a package private setRequests seems to be the way to go rather than pass in on construction just for the case of passing a request count on daughter split. I can do it on commit if you want?
          Hide
          yuzhihong@gmail.com Ted Yu added a comment -

          Go ahead.
          Thanks

          Show
          yuzhihong@gmail.com Ted Yu added a comment - Go ahead. Thanks
          Hide
          stack stack added a comment -

          Here is a version of the patch that has the HRegion#requestsCounter package private so it can be set by the SplitTransaction. It removes the passing of requests count into the HRegion constructor.

          There seem to a few issues w/ the patch though. First, I see that we only increment the new HRegion#requestsCounter on region open – is that right? Second, we're already doing counts up in HRegionServer. Is the intent to count twice but this time at the HRegions' scope?

          Show
          stack stack added a comment - Here is a version of the patch that has the HRegion#requestsCounter package private so it can be set by the SplitTransaction. It removes the passing of requests count into the HRegion constructor. There seem to a few issues w/ the patch though. First, I see that we only increment the new HRegion#requestsCounter on region open – is that right? Second, we're already doing counts up in HRegionServer. Is the intent to count twice but this time at the HRegions' scope?
          Hide
          yuzhihong@gmail.com Ted Yu added a comment -

          HRegion.startRegionOperation() is called in many methods, such as checkAndMutate(), delete(Delete delete, Integer lockid, boolean writeToWAL), increment().

          For item 2, the intent is to record the count at HRegion level so that we have more accurate measure for region server load.

          Show
          yuzhihong@gmail.com Ted Yu added a comment - HRegion.startRegionOperation() is called in many methods, such as checkAndMutate(), delete(Delete delete, Integer lockid, boolean writeToWAL), increment(). For item 2, the intent is to record the count at HRegion level so that we have more accurate measure for region server load.
          Hide
          stack stack added a comment -

          Ok on the startRegionOperation.

          So we are double-counting then? Once at RS level and then again at the Region level?

          Should we check in this to do our counting? http://sourceforge.net/projects/high-scale-lib/files/high-scale-lib/high-scale-lib-v1.1.1/

          Show
          stack stack added a comment - Ok on the startRegionOperation. So we are double-counting then? Once at RS level and then again at the Region level? Should we check in this to do our counting? http://sourceforge.net/projects/high-scale-lib/files/high-scale-lib/high-scale-lib-v1.1.1/
          Hide
          ryanobjc ryan rawson added a comment -

          in my profiling runs, the AtomicInteger/Longs in our stats were
          showing up as small hot spots. It wasnt a huge deal so I didn't do all
          the surgery to switch in high scale lib counters, but we should
          consider it at some point.

          Show
          ryanobjc ryan rawson added a comment - in my profiling runs, the AtomicInteger/Longs in our stats were showing up as small hot spots. It wasnt a huge deal so I didn't do all the surgery to switch in high scale lib counters, but we should consider it at some point.
          Hide
          yuzhihong@gmail.com Ted Yu added a comment -

          I think we can try using ConcurrentAutoTable

          From https://issues.apache.org/jira/browse/CASSANDRA-1888:
          I committed 1.1.1 from (http://repo1.maven.org/maven2/com/github/stephenc/high-scale-lib/high-scale-lib/1.1.1/high-scale-lib-1.1.1.jar)

          How should I specify dependency in pom.xml ?

          Show
          yuzhihong@gmail.com Ted Yu added a comment - I think we can try using ConcurrentAutoTable From https://issues.apache.org/jira/browse/CASSANDRA-1888: I committed 1.1.1 from ( http://repo1.maven.org/maven2/com/github/stephenc/high-scale-lib/high-scale-lib/1.1.1/high-scale-lib-1.1.1.jar ) How should I specify dependency in pom.xml ?
          Hide
          yuzhihong@gmail.com Ted Yu added a comment -

          Changed requestsCount to high-scale-lib's Counter class.
          Also added the summation of region requestsCount's in HRegion.merge
          This patch is based on latest trunk code.

          Show
          yuzhihong@gmail.com Ted Yu added a comment - Changed requestsCount to high-scale-lib's Counter class. Also added the summation of region requestsCount's in HRegion.merge This patch is based on latest trunk code.
          Hide
          stack stack added a comment -

          I took a quick look at this. It looks great Ted. I want to look again before commit. Thats a nice find where you figured a maven repository location for the cliff click jar. Good stuff Ted.

          Show
          stack stack added a comment - I took a quick look at this. It looks great Ted. I want to look again before commit. Thats a nice find where you figured a maven repository location for the cliff click jar. Good stuff Ted.
          Hide
          stack stack added a comment -

          Making major rather than trivial and filing against 0.92.

          Show
          stack stack added a comment - Making major rather than trivial and filing against 0.92.
          Hide
          stack stack added a comment -

          I just committed part 1 of this patch. Thanks Ted and Sebastian for persisting.

          Regards part 2, it looks a bit wonky to me. You are balancing based off requests though the balancer in place is balancing off the count of regions (you are running the balance yourself manually, external to hbase). Thats kinda cool but I'm not sure I should commit it. For example, the name of the shell command is 'rebalance' when it probably should be 'rebalance_based_off_region_hits_but_native_balancer_must_be_disabled'?

          Thanks lads.

          Show
          stack stack added a comment - I just committed part 1 of this patch. Thanks Ted and Sebastian for persisting. Regards part 2, it looks a bit wonky to me. You are balancing based off requests though the balancer in place is balancing off the count of regions (you are running the balance yourself manually, external to hbase). Thats kinda cool but I'm not sure I should commit it. For example, the name of the shell command is 'rebalance' when it probably should be 'rebalance_based_off_region_hits_but_native_balancer_must_be_disabled'? Thanks lads.
          Hide
          hudson Hudson added a comment -

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

          Show
          hudson Hudson added a comment - Integrated in HBase-TRUNK #1792 (See https://hudson.apache.org/hudson/job/HBase-TRUNK/1792/ )
          Hide
          stack stack added a comment -

          Moving out of 0.92.0. Pull it back in if you think different.

          Show
          stack stack added a comment - Moving out of 0.92.0. Pull it back in if you think different.
          Hide
          stack stack added a comment -

          Bringing back in; give it another review before punting.

          Show
          stack stack added a comment - Bringing back in; give it another review before punting.
          Hide
          stack stack added a comment -

          Changed the title to leave out the rebalance aspect. Will open new issue for rebalance command (since we committed first part of this two-part patch against this issue already)

          Show
          stack stack added a comment - Changed the title to leave out the rebalance aspect. Will open new issue for rebalance command (since we committed first part of this two-part patch against this issue already)
          Hide
          stack stack added a comment -

          Part 1 was committed a good while ago. Thanks Ted and Sebastian. I opened HBASE-4431 to add the part two patch added above. Lets do the part two work over in the new issue.

          Show
          stack stack added a comment - Part 1 was committed a good while ago. Thanks Ted and Sebastian. I opened HBASE-4431 to add the part two patch added above. Lets do the part two work over in the new issue.
          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).

            People

            • Assignee:
              yuzhihong@gmail.com Ted Yu
              Reporter:
              anih Sebastian Bauer
            • Votes:
              1 Vote for this issue
              Watchers:
              6 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development