Details

    • Bug
    • Status: Closed
    • Major
    • Resolution: Fixed
    • 0.20.2, 0.20.3
    • 0.90.0
    • None
    • None

    Description

      Right now with large region count tables, the write buffer is not efficient. This is because we issue potentially N RPCs, where N is the # of regions in the table. When N gets large (lets say 1200+) things become sloowwwww.

      Instead if we batch things up using a different RPC and use thread pools, we could see higher performance!

      This requires a RPC change...

      Attachments

        1. TestBatchPut.java
          2 kB
          Cosmin Lehene
        2. HBASE-2066-v2.patch
          25 kB
          ryan rawson
        3. HBASE-2066-branch.patch
          22 kB
          ryan rawson
        4. HBASE-2066-3.patch
          31 kB
          ryan rawson
        5. HBASE-2066-20-branch.txt
          32 kB
          ryan rawson

        Issue Links

          Activity

            ryanobjc ryan rawson added a comment -

            include rpc version bump

            ryanobjc ryan rawson added a comment - include rpc version bump
            stack Michael Stack added a comment -

            Patch looks great.

            We can't do a version bump in 0.20 branch. Adding a new method to the interface w/o version bumping doesn't work I suppose. How about a version in 0.20 that doesn't pass ExcecutorService and a timeout and whose method is named processBatchOfRows rather processBatchOfPuts?

            Any chance of some tests?

            Will this fix help in 0.20 branch?

            @@ -845,8 +855,9 @@ public class HConnectionManager implements HConstants {
             
                       // by nature of the map, we know that the start key has to be < 
                       // otherwise it wouldn't be in the headMap. 
            -          if (KeyValue.getRowComparator(tableName).compareRows(endKey, 0, endKey.length,
            -              row, 0, row.length) <= 0) {
            +          if (Bytes.equals(endKey, HConstants.EMPTY_END_ROW) ||
            +              KeyValue.getRowComparator(tableName).compareRows(endKey, 0, endKey.length,
            +              row, 0, row.length) > 0) {
                         // delete any matching entry
                         HRegionLocation rl =
                           tableLocations.remove(matchingRegions.lastKey());
            

            Do you want to change these:

            +            LOG.debug("Failed all from " + request.address + " due to ExecutionException");
            

            .. so the are instead:

            +            LOG.debug("Failed all from " + request.address, e);
            

            Is this done once, getCurrentNrHRS, in the HTable constructor?

            looks really good

            stack Michael Stack added a comment - Patch looks great. We can't do a version bump in 0.20 branch. Adding a new method to the interface w/o version bumping doesn't work I suppose. How about a version in 0.20 that doesn't pass ExcecutorService and a timeout and whose method is named processBatchOfRows rather processBatchOfPuts? Any chance of some tests? Will this fix help in 0.20 branch? @@ -845,8 +855,9 @@ public class HConnectionManager implements HConstants { // by nature of the map, we know that the start key has to be < // otherwise it wouldn't be in the headMap. - if (KeyValue.getRowComparator(tableName).compareRows(endKey, 0, endKey.length, - row, 0, row.length) <= 0) { + if (Bytes.equals(endKey, HConstants.EMPTY_END_ROW) || + KeyValue.getRowComparator(tableName).compareRows(endKey, 0, endKey.length, + row, 0, row.length) > 0) { // delete any matching entry HRegionLocation rl = tableLocations.remove(matchingRegions.lastKey()); Do you want to change these: + LOG.debug( "Failed all from " + request.address + " due to ExecutionException" ); .. so the are instead: + LOG.debug( "Failed all from " + request.address, e); Is this done once, getCurrentNrHRS, in the HTable constructor? looks really good

            How does this relate to HBASE-1845?

            hammer Jeff Hammerbacher added a comment - How does this relate to HBASE-1845 ?
            ryanobjc ryan rawson added a comment -

            This is much less ambitious than HBASE-1845 and seeks to optimize the Put case only.

            One of the problems with the original HBASE-1845 patch is that it requires a new API to take advantage of it, thus requires porting code. Furthermore there is HTable handy things like write buffering, write buffer size settings, etc, etc. I started with the 1845 patch, and realized we also needed a way to parallelize puts in the normal API. This is much simpler than 1845 because we don't have to line up return codes (there are no return codes for puts, just exceptions due to temporary issues).

            Short: this is a drop in replacement and makes things go fast now. HBASE-1845 requires a new API.

            ryanobjc ryan rawson added a comment - This is much less ambitious than HBASE-1845 and seeks to optimize the Put case only. One of the problems with the original HBASE-1845 patch is that it requires a new API to take advantage of it, thus requires porting code. Furthermore there is HTable handy things like write buffering, write buffer size settings, etc, etc. I started with the 1845 patch, and realized we also needed a way to parallelize puts in the normal API. This is much simpler than 1845 because we don't have to line up return codes (there are no return codes for puts, just exceptions due to temporary issues). Short: this is a drop in replacement and makes things go fast now. HBASE-1845 requires a new API.
            ryanobjc ryan rawson added a comment -

            this is a trunk version with test.

            ryanobjc ryan rawson added a comment - this is a trunk version with test.
            stack Michael Stack added a comment -

            Patch looks good. Make sure all licenses are 2010 on commit and add some class comment to new classes saying what they do on commit. You don't up the RPC version? Otherwise it looks great RR.

            stack Michael Stack added a comment - Patch looks good. Make sure all licenses are 2010 on commit and add some class comment to new classes saying what they do on commit. You don't up the RPC version? Otherwise it looks great RR.
            stack Michael Stack added a comment -

            Hey man, commit already!

            stack Michael Stack added a comment - Hey man, commit already!
            clehene Cosmin Lehene added a comment -

            Patch fails to apply on trunk.
            After manually applying chunks I got these while doing puts

            EXCEPTION 1

            java.lang.NullPointerException
            at org.apache.hadoop.hbase.client.HConnectionManager$TableServers.deleteCachedLocation(HConnectionManager.java:889)
            at org.apache.hadoop.hbase.client.HConnectionManager$TableServers.processBatchOfPuts(HConnectionManager.java:1413)
            at org.apache.hadoop.hbase.client.HTable.flushCommits(HTable.java:586)
            at org.apache.hadoop.hbase.client.HTable.put(HTable.java:471)
            at TestBatchPut$MyThread.run(TestBatchPut.java:65)

            EXCEPTION 2

            java.lang.NullPointerException
            at java.util.TreeMap.rotateRight(TreeMap.java:2057)
            at java.util.TreeMap.fixAfterDeletion(TreeMap.java:2217)
            at java.util.TreeMap.deleteEntry(TreeMap.java:2151)
            at java.util.TreeMap.remove(TreeMap.java:585)
            at org.apache.hadoop.hbase.util.SoftValueSortedMap.remove(SoftValueSortedMap.java:104)
            at org.apache.hadoop.hbase.client.HConnectionManager$TableServers.deleteCachedLocation(HConnectionManager.java:897)
            at org.apache.hadoop.hbase.client.HConnectionManager$TableServers.processBatchOfPuts(HConnectionManager.java:1413)
            at org.apache.hadoop.hbase.client.HTable.flushCommits(HTable.java:586)
            at org.apache.hadoop.hbase.client.HTable.put(HTable.java:471)
            at TestBatchPut$MyThread.run(TestBatchPut.java:65)

            Also the throughput went down and the max seconds for a put went up (could be also from the hbase restart).

            I'll attach the piece of code I'm using to benchmark it

            clehene Cosmin Lehene added a comment - Patch fails to apply on trunk. After manually applying chunks I got these while doing puts EXCEPTION 1 java.lang.NullPointerException at org.apache.hadoop.hbase.client.HConnectionManager$TableServers.deleteCachedLocation(HConnectionManager.java:889) at org.apache.hadoop.hbase.client.HConnectionManager$TableServers.processBatchOfPuts(HConnectionManager.java:1413) at org.apache.hadoop.hbase.client.HTable.flushCommits(HTable.java:586) at org.apache.hadoop.hbase.client.HTable.put(HTable.java:471) at TestBatchPut$MyThread.run(TestBatchPut.java:65) EXCEPTION 2 java.lang.NullPointerException at java.util.TreeMap.rotateRight(TreeMap.java:2057) at java.util.TreeMap.fixAfterDeletion(TreeMap.java:2217) at java.util.TreeMap.deleteEntry(TreeMap.java:2151) at java.util.TreeMap.remove(TreeMap.java:585) at org.apache.hadoop.hbase.util.SoftValueSortedMap.remove(SoftValueSortedMap.java:104) at org.apache.hadoop.hbase.client.HConnectionManager$TableServers.deleteCachedLocation(HConnectionManager.java:897) at org.apache.hadoop.hbase.client.HConnectionManager$TableServers.processBatchOfPuts(HConnectionManager.java:1413) at org.apache.hadoop.hbase.client.HTable.flushCommits(HTable.java:586) at org.apache.hadoop.hbase.client.HTable.put(HTable.java:471) at TestBatchPut$MyThread.run(TestBatchPut.java:65) Also the throughput went down and the max seconds for a put went up (could be also from the hbase restart). I'll attach the piece of code I'm using to benchmark it
            clehene Cosmin Lehene added a comment -

            run TestBatchPut nr_of_threads nr_of_puts_per_call

            clehene Cosmin Lehene added a comment - run TestBatchPut nr_of_threads nr_of_puts_per_call
            ryanobjc ryan rawson added a comment -

            looks like a basic thread concurrency problem here.

            Now to the performance issues, the current code uses ONE threadpool for everyone, which is currently set to 10 threads static. The original code used a thread pool per HTable and sized it to the number of regionservers - that is impossible to do in HCM because of chicken-and-egg bootstrap problems (the call we'd use calls HCM.<init> which calls ...).

            Maybe the threadpool should move back into HTable to support parallelism better? With 10 worker threads for way more than 10 client threads, yeah put performance is going to nosedive.

            ryanobjc ryan rawson added a comment - looks like a basic thread concurrency problem here. Now to the performance issues, the current code uses ONE threadpool for everyone, which is currently set to 10 threads static. The original code used a thread pool per HTable and sized it to the number of regionservers - that is impossible to do in HCM because of chicken-and-egg bootstrap problems (the call we'd use calls HCM.<init> which calls ...). Maybe the threadpool should move back into HTable to support parallelism better? With 10 worker threads for way more than 10 client threads, yeah put performance is going to nosedive.
            ryanobjc ryan rawson added a comment -

            here is the much awaited new version. i'll run some tests on it and then commit if things look good.

            ryanobjc ryan rawson added a comment - here is the much awaited new version. i'll run some tests on it and then commit if things look good.
            ryanobjc ryan rawson added a comment -

            i ran TestBatchPut for a while and inserted 3.3GB of data w/o problems. Ended up with like 4 table splits. No more concurrent exceptions, no major slowdown... the threads got slower as my machine bogged down, but it wasnt some crazy kind of exponential slowdown originally reported.

            if there is no complaints, i'm going to commit this as-is.

            ryanobjc ryan rawson added a comment - i ran TestBatchPut for a while and inserted 3.3GB of data w/o problems. Ended up with like 4 table splits. No more concurrent exceptions, no major slowdown... the threads got slower as my machine bogged down, but it wasnt some crazy kind of exponential slowdown originally reported. if there is no complaints, i'm going to commit this as-is.
            ryanobjc ryan rawson added a comment -

            commited to trunk

            ryanobjc ryan rawson added a comment - commited to trunk
            ryanobjc ryan rawson added a comment -

            this will go into 0.20 branch since now we have HBASE-2219 in there

            ryanobjc ryan rawson added a comment - this will go into 0.20 branch since now we have HBASE-2219 in there
            ryanobjc ryan rawson added a comment -

            adding to 0.20.4

            ryanobjc ryan rawson added a comment - adding to 0.20.4
            ryanobjc ryan rawson added a comment -

            for the branch

            ryanobjc ryan rawson added a comment - for the branch

            Since HBASE-2066 was committed on 0.20 branch o.a.h.h.client.TestGetRowVersions is hanging.

            Before:

            test-core:
                [mkdir] Created dir: /home/apurtell/src/Hadoop/hbase.git/build/test/logs
                [junit] Running org.apache.hadoop.hbase.client.TestGetRowVersions
                [junit] Tests run: 1, Failures: 0, Errors: 0, Time elapsed: 36.048 sec
            

            Now:

            test-core:
                [mkdir] Created dir: /home/apurtell/src/Hadoop/hbase.git/build/test/logs
                [junit] Running org.apache.hadoop.hbase.client.TestGetRowVersions
                [junit] Tests run: 1, Failures: 0, Errors: 1, Time elapsed: 0 sec
                [junit] Test org.apache.hadoop.hbase.client.TestGetRowVersions FAILED (timeout)
            

            TestGetRowVersions shuts down and restarts the minicluster mid test. Maybe it could just force flush instead?

            Prior to 2066 this test would exit, but I think only by luck. Now, according to jstack main() is joined to the regionserver thread, which is trying again and again to report for duty to a master thread that has gone away. Neither main nor the regionserver threads are daemon threads, so the JVM does not exit.

            apurtell Andrew Kyle Purtell added a comment - Since HBASE-2066 was committed on 0.20 branch o.a.h.h.client.TestGetRowVersions is hanging. Before: test-core: [mkdir] Created dir: /home/apurtell/src/Hadoop/hbase.git/build/test/logs [junit] Running org.apache.hadoop.hbase.client.TestGetRowVersions [junit] Tests run: 1, Failures: 0, Errors: 0, Time elapsed: 36.048 sec Now: test-core: [mkdir] Created dir: /home/apurtell/src/Hadoop/hbase.git/build/test/logs [junit] Running org.apache.hadoop.hbase.client.TestGetRowVersions [junit] Tests run: 1, Failures: 0, Errors: 1, Time elapsed: 0 sec [junit] Test org.apache.hadoop.hbase.client.TestGetRowVersions FAILED (timeout) TestGetRowVersions shuts down and restarts the minicluster mid test. Maybe it could just force flush instead? Prior to 2066 this test would exit, but I think only by luck. Now, according to jstack main() is joined to the regionserver thread, which is trying again and again to report for duty to a master thread that has gone away. Neither main nor the regionserver threads are daemon threads, so the JVM does not exit.

            Ryan committed a fix to SVN which makes the testcase work again.

            apurtell Andrew Kyle Purtell added a comment - Ryan committed a fix to SVN which makes the testcase work again.
            ryanobjc ryan rawson added a comment -

            commited to branch now

            ryanobjc ryan rawson added a comment - commited to branch now
            stack Michael Stack added a comment -

            Marking these as fixed against 0.21.0 rather than against 0.20.5.

            stack Michael Stack added a comment - Marking these as fixed against 0.21.0 rather than against 0.20.5.
            larsfrancke 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).

            larsfrancke 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

              ryanobjc ryan rawson
              ryanobjc ryan rawson
              Votes:
              0 Vote for this issue
              Watchers:
              5 Start watching this issue

              Dates

                Created:
                Updated:
                Resolved: