Details

    • Hadoop Flags:
      Incompatible change, Reviewed
    • Release Note:
      Changed the coprocessor RegionObserver methods preBatchMutate and postBatchMutate to remove the lock ids from the methods as lock ids are not longer used.
    • Tags:
      0.96notable

      Description

      HBASE-8806 revealed performance problems with batch mutations failing to reacquire the same row locks. It looks like HBASE-8806 will use a less intrusive change for 0.94 to have batch mutations track their own row locks and not attempt to reacquire them. Another approach will be to support reentrant row locks directly. This allows simplifying a great deal of calling code to no longer track and pass around lock ids.

      One affect this change will have is changing the RegionObserver coprocessor's methods preBatchMutate and postBatchMutate from taking a MiniBatchOperationInProgress<Pair<Mutation, Integer>> miniBatchOp to taking a MiniBatchOperationInProgress<Mutation> miniBatchOp. I don't believe CPs should be relying on these lock ids, but that's a potential incompatibility.

      1. HBASE-8877-v7.patch
        49 kB
        Dave Latham
      2. HBASE-8877-v6.patch
        46 kB
        Dave Latham
      3. HBASE-8877-v5.patch
        45 kB
        Dave Latham
      4. hbase-8877-v4-microbenchmark.txt
        4 kB
        Dave Latham
      5. HBASE-8877-v4.patch
        45 kB
        Dave Latham
      6. HBASE-8877-v3.patch
        45 kB
        Dave Latham
      7. HBASE-8877-v2.patch
        45 kB
        Dave Latham
      8. HBASE-8877-refCounts-v5.patch
        69 kB
        Dave Latham
      9. HBASE-8877-refCounts-v4.patch
        69 kB
        Dave Latham
      10. HBASE-8877-refCounts-v3.patch
        71 kB
        Dave Latham
      11. HBASE-8877-refCounts-v2.patch
        71 kB
        Dave Latham
      12. hbase-8877-refCounts-microbenchmarks.txt
        6 kB
        Dave Latham
      13. HBASE-8877-refCounts.patch
        71 kB
        Dave Latham
      14. HBASE-8877-0.94-v2.patch
        8 kB
        Dave Latham
      15. hbase-8877-0.94-microbenchmark.txt
        3 kB
        Dave Latham
      16. HBASE-8877-0.94.patch
        8 kB
        Dave Latham
      17. HBASE-8877.patch
        45 kB
        Dave Latham

        Issue Links

          Activity

          Hide
          Dave Latham added a comment -

          Thanks everyone for your attention, help, and patience, and especially Lars for getting it committed. I added a Release Note to note the incompatible change to the RegionObserver. I'm also curious about the integration failure messages from Jenkins. Particularly the first two where the console doesn't show any test failures.

          Show
          Dave Latham added a comment - Thanks everyone for your attention, help, and patience, and especially Lars for getting it committed. I added a Release Note to note the incompatible change to the RegionObserver. I'm also curious about the integration failure messages from Jenkins. Particularly the first two where the console doesn't show any test failures.
          Hide
          Hudson added a comment -

          SUCCESS: Integrated in HBase-TRUNK-on-Hadoop-2.0.0 #620 (See https://builds.apache.org/job/HBase-TRUNK-on-Hadoop-2.0.0/620/)
          HBASE-8877 Reentrant row locks (Dave Latham) (larsh: rev 1504002)

          • /hbase/trunk/hbase-examples/src/main/java/org/apache/hadoop/hbase/coprocessor/example/BulkDeleteEndpoint.java
          • /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/coprocessor/BaseRegionObserver.java
          • /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/coprocessor/RegionObserver.java
          • /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
          • /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java
          • /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/RegionCoprocessorHost.java
          • /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/util/HBaseFsck.java
          • /hbase/trunk/hbase-server/src/test/java/org/apache/hadoop/hbase/coprocessor/SimpleRegionObserver.java
          • /hbase/trunk/hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestAtomicOperation.java
          • /hbase/trunk/hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestHRegion.java
          • /hbase/trunk/hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestParallelPut.java
          Show
          Hudson added a comment - SUCCESS: Integrated in HBase-TRUNK-on-Hadoop-2.0.0 #620 (See https://builds.apache.org/job/HBase-TRUNK-on-Hadoop-2.0.0/620/ ) HBASE-8877 Reentrant row locks (Dave Latham) (larsh: rev 1504002) /hbase/trunk/hbase-examples/src/main/java/org/apache/hadoop/hbase/coprocessor/example/BulkDeleteEndpoint.java /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/coprocessor/BaseRegionObserver.java /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/coprocessor/RegionObserver.java /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/RegionCoprocessorHost.java /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/util/HBaseFsck.java /hbase/trunk/hbase-server/src/test/java/org/apache/hadoop/hbase/coprocessor/SimpleRegionObserver.java /hbase/trunk/hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestAtomicOperation.java /hbase/trunk/hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestHRegion.java /hbase/trunk/hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestParallelPut.java
          Hide
          Hudson added a comment -

          FAILURE: Integrated in hbase-0.95-on-hadoop2 #182 (See https://builds.apache.org/job/hbase-0.95-on-hadoop2/182/)
          HBASE-8877 Reentrant row locks (Dave Latham) (larsh: rev 1504000)

          • /hbase/branches/0.95/hbase-examples/src/main/java/org/apache/hadoop/hbase/coprocessor/example/BulkDeleteEndpoint.java
          • /hbase/branches/0.95/hbase-server/src/main/java/org/apache/hadoop/hbase/coprocessor/BaseRegionObserver.java
          • /hbase/branches/0.95/hbase-server/src/main/java/org/apache/hadoop/hbase/coprocessor/RegionObserver.java
          • /hbase/branches/0.95/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
          • /hbase/branches/0.95/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java
          • /hbase/branches/0.95/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/RegionCoprocessorHost.java
          • /hbase/branches/0.95/hbase-server/src/main/java/org/apache/hadoop/hbase/util/HBaseFsck.java
          • /hbase/branches/0.95/hbase-server/src/test/java/org/apache/hadoop/hbase/coprocessor/SimpleRegionObserver.java
          • /hbase/branches/0.95/hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestAtomicOperation.java
          • /hbase/branches/0.95/hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestHRegion.java
          • /hbase/branches/0.95/hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestParallelPut.java
          Show
          Hudson added a comment - FAILURE: Integrated in hbase-0.95-on-hadoop2 #182 (See https://builds.apache.org/job/hbase-0.95-on-hadoop2/182/ ) HBASE-8877 Reentrant row locks (Dave Latham) (larsh: rev 1504000) /hbase/branches/0.95/hbase-examples/src/main/java/org/apache/hadoop/hbase/coprocessor/example/BulkDeleteEndpoint.java /hbase/branches/0.95/hbase-server/src/main/java/org/apache/hadoop/hbase/coprocessor/BaseRegionObserver.java /hbase/branches/0.95/hbase-server/src/main/java/org/apache/hadoop/hbase/coprocessor/RegionObserver.java /hbase/branches/0.95/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java /hbase/branches/0.95/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java /hbase/branches/0.95/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/RegionCoprocessorHost.java /hbase/branches/0.95/hbase-server/src/main/java/org/apache/hadoop/hbase/util/HBaseFsck.java /hbase/branches/0.95/hbase-server/src/test/java/org/apache/hadoop/hbase/coprocessor/SimpleRegionObserver.java /hbase/branches/0.95/hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestAtomicOperation.java /hbase/branches/0.95/hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestHRegion.java /hbase/branches/0.95/hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestParallelPut.java
          Hide
          Hudson added a comment -

          FAILURE: Integrated in HBase-TRUNK #4260 (See https://builds.apache.org/job/HBase-TRUNK/4260/)
          HBASE-8877 Reentrant row locks (Dave Latham) (larsh: rev 1504002)

          • /hbase/trunk/hbase-examples/src/main/java/org/apache/hadoop/hbase/coprocessor/example/BulkDeleteEndpoint.java
          • /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/coprocessor/BaseRegionObserver.java
          • /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/coprocessor/RegionObserver.java
          • /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
          • /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java
          • /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/RegionCoprocessorHost.java
          • /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/util/HBaseFsck.java
          • /hbase/trunk/hbase-server/src/test/java/org/apache/hadoop/hbase/coprocessor/SimpleRegionObserver.java
          • /hbase/trunk/hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestAtomicOperation.java
          • /hbase/trunk/hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestHRegion.java
          • /hbase/trunk/hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestParallelPut.java
          Show
          Hudson added a comment - FAILURE: Integrated in HBase-TRUNK #4260 (See https://builds.apache.org/job/HBase-TRUNK/4260/ ) HBASE-8877 Reentrant row locks (Dave Latham) (larsh: rev 1504002) /hbase/trunk/hbase-examples/src/main/java/org/apache/hadoop/hbase/coprocessor/example/BulkDeleteEndpoint.java /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/coprocessor/BaseRegionObserver.java /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/coprocessor/RegionObserver.java /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/RegionCoprocessorHost.java /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/util/HBaseFsck.java /hbase/trunk/hbase-server/src/test/java/org/apache/hadoop/hbase/coprocessor/SimpleRegionObserver.java /hbase/trunk/hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestAtomicOperation.java /hbase/trunk/hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestHRegion.java /hbase/trunk/hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestParallelPut.java
          Hide
          Hudson added a comment -

          FAILURE: Integrated in hbase-0.95 #331 (See https://builds.apache.org/job/hbase-0.95/331/)
          HBASE-8877 Reentrant row locks (Dave Latham) (larsh: rev 1504000)

          • /hbase/branches/0.95/hbase-examples/src/main/java/org/apache/hadoop/hbase/coprocessor/example/BulkDeleteEndpoint.java
          • /hbase/branches/0.95/hbase-server/src/main/java/org/apache/hadoop/hbase/coprocessor/BaseRegionObserver.java
          • /hbase/branches/0.95/hbase-server/src/main/java/org/apache/hadoop/hbase/coprocessor/RegionObserver.java
          • /hbase/branches/0.95/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
          • /hbase/branches/0.95/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java
          • /hbase/branches/0.95/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/RegionCoprocessorHost.java
          • /hbase/branches/0.95/hbase-server/src/main/java/org/apache/hadoop/hbase/util/HBaseFsck.java
          • /hbase/branches/0.95/hbase-server/src/test/java/org/apache/hadoop/hbase/coprocessor/SimpleRegionObserver.java
          • /hbase/branches/0.95/hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestAtomicOperation.java
          • /hbase/branches/0.95/hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestHRegion.java
          • /hbase/branches/0.95/hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestParallelPut.java
          Show
          Hudson added a comment - FAILURE: Integrated in hbase-0.95 #331 (See https://builds.apache.org/job/hbase-0.95/331/ ) HBASE-8877 Reentrant row locks (Dave Latham) (larsh: rev 1504000) /hbase/branches/0.95/hbase-examples/src/main/java/org/apache/hadoop/hbase/coprocessor/example/BulkDeleteEndpoint.java /hbase/branches/0.95/hbase-server/src/main/java/org/apache/hadoop/hbase/coprocessor/BaseRegionObserver.java /hbase/branches/0.95/hbase-server/src/main/java/org/apache/hadoop/hbase/coprocessor/RegionObserver.java /hbase/branches/0.95/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java /hbase/branches/0.95/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java /hbase/branches/0.95/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/RegionCoprocessorHost.java /hbase/branches/0.95/hbase-server/src/main/java/org/apache/hadoop/hbase/util/HBaseFsck.java /hbase/branches/0.95/hbase-server/src/test/java/org/apache/hadoop/hbase/coprocessor/SimpleRegionObserver.java /hbase/branches/0.95/hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestAtomicOperation.java /hbase/branches/0.95/hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestHRegion.java /hbase/branches/0.95/hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestParallelPut.java
          Hide
          Lars Hofhansl added a comment -

          Committed to 0.95 and trunk. Thanks for the patch Dave!

          Show
          Lars Hofhansl added a comment - Committed to 0.95 and trunk. Thanks for the patch Dave!
          Hide
          Ted Yu added a comment -

          +1

          Show
          Ted Yu added a comment - +1
          Hide
          Lars Hofhansl added a comment -

          Actually. Can I get another +1 for refCounts-v5?

          Show
          Lars Hofhansl added a comment - Actually. Can I get another +1 for refCounts-v5?
          Hide
          Lars Hofhansl added a comment -

          Will commit in a bit, unless I hear objections or somebody beats me to it.

          Show
          Lars Hofhansl added a comment - Will commit in a bit, unless I hear objections or somebody beats me to it.
          Hide
          Dave Latham added a comment -

          Looks good to commit to me. Hadoop QA says that org.apache.hadoop.hbase.mapreduce.TestHFileOutputFormat failed, but that area of the code is untouched by this change, the test passed in the previous very similar patch, and the test passes for me locally.

          If no one objects, perhaps Lars Hofhansl or stack could do the honors and commit to trunk and 0.95 later today or tomorrow? (Looks like HBASE-8806 will take care of 0.94 which sounds good to me.)

          Show
          Dave Latham added a comment - Looks good to commit to me. Hadoop QA says that org.apache.hadoop.hbase.mapreduce.TestHFileOutputFormat failed, but that area of the code is untouched by this change, the test passed in the previous very similar patch, and the test passes for me locally. If no one objects, perhaps Lars Hofhansl or stack could do the honors and commit to trunk and 0.95 later today or tomorrow? (Looks like HBASE-8806 will take care of 0.94 which sounds good to me.)
          Hide
          Hadoop QA added a comment -

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

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

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

          +1 hadoop1.0. The patch compiles against the hadoop 1.0 profile.

          +1 hadoop2.0. The patch compiles against the hadoop 2.0 profile.

          +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 lineLengths. The patch does not introduce lines longer than 100

          +1 site. The mvn site goal succeeds with this patch.

          -1 core tests. The patch failed these unit tests:
          org.apache.hadoop.hbase.mapreduce.TestHFileOutputFormat

          Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/6357//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/6357//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-prefix-tree.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/6357//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-client.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/6357//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-common.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/6357//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-protocol.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/6357//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-server.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/6357//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop1-compat.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/6357//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-examples.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/6357//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop-compat.html
          Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/6357//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/12592549/HBASE-8877-refCounts-v5.patch against trunk revision . +1 @author . The patch does not contain any @author tags. +1 tests included . The patch appears to include 12 new or modified tests. +1 hadoop1.0 . The patch compiles against the hadoop 1.0 profile. +1 hadoop2.0 . The patch compiles against the hadoop 2.0 profile. +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 lineLengths . The patch does not introduce lines longer than 100 +1 site . The mvn site goal succeeds with this patch. -1 core tests . The patch failed these unit tests: org.apache.hadoop.hbase.mapreduce.TestHFileOutputFormat Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/6357//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/6357//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-prefix-tree.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/6357//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-client.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/6357//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-common.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/6357//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-protocol.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/6357//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-server.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/6357//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop1-compat.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/6357//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-examples.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/6357//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop-compat.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/6357//console This message is automatically generated.
          Hide
          Dave Latham added a comment -

          Attaching HBASE-8877-refCounts-v5.patch
          This one's for all the marbles.

          • Reformatted the too-long line (thanks Hadoop QA)
          • improved a log message and an exception message
          • reduced scope of RowLockContext methods from public to package

          Here are the changes from HBASE-8877-refCounts-v4.patch to HBASE-8877-refCounts-v5.patch

          476,477c476,477
          < +  public RowLock getRowLock(final byte[] row, boolean waitForLock)
          <    throws IOException {
          ---
          > -  throws IOException {
          > +  public RowLock getRowLock(byte[] row, boolean waitForLock) throws IOException {
          499c499
          < +          // Row is already locked by some other thread
          ---
          > +          // Row is already locked by some other thread, give up or wait for it
          505a506,507
          > -              throw new IOException("Timed out on getting lock for row="
          > -                  + Bytes.toStringBinary(row));
          > +              throw new IOException("Timed out waiting for lock for row: " + rowKey);
          510c511,516
          > -            LOG.warn("internalObtainRowLock interrupted for row=" + Bytes.toStringBinary(row));
          > +            LOG.warn("Thread interrupted waiting for lock on row: " + rowKey);
          1144c1150
          < +    public RowLockContext(HashedBytes row) {
          ---
          > +    RowLockContext(HashedBytes row) {
          1149c1155
          < +    public boolean ownedByCurrentThread() {
          ---
          > +    boolean ownedByCurrentThread() {
          1153c1159
          < +    public RowLock newLock() {
          ---
          > +    RowLock newLock() {
          1168c1174,1175
          < +          throw new RuntimeException("Internal row lock state inconsistent, should not happen, row: " + row);
          ---
          > +          throw new RuntimeException(
          > +              "Internal row lock state inconsistent, should not happen, row: " + row);
          
          Show
          Dave Latham added a comment - Attaching HBASE-8877 -refCounts-v5.patch This one's for all the marbles. Reformatted the too-long line (thanks Hadoop QA) improved a log message and an exception message reduced scope of RowLockContext methods from public to package Here are the changes from HBASE-8877 -refCounts-v4.patch to HBASE-8877 -refCounts-v5.patch 476,477c476,477 < + public RowLock getRowLock(final byte[] row, boolean waitForLock) < throws IOException { --- > - throws IOException { > + public RowLock getRowLock(byte[] row, boolean waitForLock) throws IOException { 499c499 < + // Row is already locked by some other thread --- > + // Row is already locked by some other thread, give up or wait for it 505a506,507 > - throw new IOException("Timed out on getting lock for row=" > - + Bytes.toStringBinary(row)); > + throw new IOException("Timed out waiting for lock for row: " + rowKey); 510c511,516 > - LOG.warn("internalObtainRowLock interrupted for row=" + Bytes.toStringBinary(row)); > + LOG.warn("Thread interrupted waiting for lock on row: " + rowKey); 1144c1150 < + public RowLockContext(HashedBytes row) { --- > + RowLockContext(HashedBytes row) { 1149c1155 < + public boolean ownedByCurrentThread() { --- > + boolean ownedByCurrentThread() { 1153c1159 < + public RowLock newLock() { --- > + RowLock newLock() { 1168c1174,1175 < + throw new RuntimeException("Internal row lock state inconsistent, should not happen, row: " + row); --- > + throw new RuntimeException( > + "Internal row lock state inconsistent, should not happen, row: " + row);
          Hide
          Hadoop QA added a comment -

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

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

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

          +1 hadoop1.0. The patch compiles against the hadoop 1.0 profile.

          +1 hadoop2.0. The patch compiles against the hadoop 2.0 profile.

          +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 lineLengths. The patch introduces lines longer than 100

          +1 site. The mvn site goal succeeds with this patch.

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

          Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/6355//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/6355//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-protocol.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/6355//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-client.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/6355//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-examples.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/6355//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop1-compat.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/6355//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-prefix-tree.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/6355//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-common.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/6355//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-server.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/6355//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop-compat.html
          Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/6355//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/12592477/HBASE-8877-refCounts-v4.patch against trunk revision . +1 @author . The patch does not contain any @author tags. +1 tests included . The patch appears to include 12 new or modified tests. +1 hadoop1.0 . The patch compiles against the hadoop 1.0 profile. +1 hadoop2.0 . The patch compiles against the hadoop 2.0 profile. +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 lineLengths . The patch introduces lines longer than 100 +1 site . The mvn site goal succeeds with this patch. +1 core tests . The patch passed unit tests in . Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/6355//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/6355//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-protocol.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/6355//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-client.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/6355//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-examples.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/6355//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop1-compat.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/6355//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-prefix-tree.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/6355//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-common.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/6355//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-server.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/6355//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop-compat.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/6355//console This message is automatically generated.
          Hide
          Lars Hofhansl added a comment -

          Let's do that. +1 from me.

          Show
          Lars Hofhansl added a comment - Let's do that. +1 from me.
          Hide
          Dave Latham added a comment -

          Now that it's done, I'd prefer refCounts. I think it's less likely for someone to get it wrong in the future, because there's no worry about nested methods releasing a lock you acquired.

          Show
          Dave Latham added a comment - Now that it's done, I'd prefer refCounts. I think it's less likely for someone to get it wrong in the future, because there's no worry about nested methods releasing a lock you acquired.
          Hide
          Lars Hofhansl added a comment -

          Patch looks good.
          In the context of doMiniBatchMutation the previous version that could release all "my" locks was nicer. Hmm.

          +1 on either v7 or refCounts-v4.
          Any opinions? v7 makes life easy for doMiniBatchMutation, refcounts-v4 provides for a more general API in the future that we may or may not use.

          Show
          Lars Hofhansl added a comment - Patch looks good. In the context of doMiniBatchMutation the previous version that could release all "my" locks was nicer. Hmm. +1 on either v7 or refCounts-v4. Any opinions? v7 makes life easy for doMiniBatchMutation, refcounts-v4 provides for a more general API in the future that we may or may not use.
          Hide
          Dave Latham added a comment -

          And here's the diff up on review board if that makes it easier for anyone.
          https://reviews.apache.org/r/12573/diff/#index_header

          Show
          Dave Latham added a comment - And here's the diff up on review board if that makes it easier for anyone. https://reviews.apache.org/r/12573/diff/#index_header
          Hide
          Dave Latham added a comment -

          Another try rebased off current tip of trunk.
          HBASE-8877-refCounts-v4

          Show
          Dave Latham added a comment - Another try rebased off current tip of trunk. HBASE-8877 -refCounts-v4
          Hide
          Hadoop QA added a comment -

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

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

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

          -1 patch. The patch command could not apply the patch.

          Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/6354//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/12592445/HBASE-8877-refCounts-v3.patch against trunk revision . +1 @author . The patch does not contain any @author tags. +1 tests included . The patch appears to include 12 new or modified tests. -1 patch . The patch command could not apply the patch. Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/6354//console This message is automatically generated.
          Hide
          Lars Hofhansl added a comment -

          Awesome. Thanks for bearing with our (or at least my) bickering

          Patch looks good. (You even made the lock count a simple int, since it is only ever changed by the same thread). The change is big, because of the new try/catch blocks, wonder whether I should put it up on RB.

          Show
          Lars Hofhansl added a comment - Awesome. Thanks for bearing with our (or at least my) bickering Patch looks good. (You even made the lock count a simple int, since it is only ever changed by the same thread). The change is big, because of the new try/catch blocks, wonder whether I should put it up on RB.
          Hide
          Hadoop QA added a comment -

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

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

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

          -1 patch. The patch command could not apply the patch.

          Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/6353//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/12592445/HBASE-8877-refCounts-v3.patch against trunk revision . +1 @author . The patch does not contain any @author tags. +1 tests included . The patch appears to include 12 new or modified tests. -1 patch . The patch command could not apply the patch. Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/6353//console This message is automatically generated.
          Hide
          Dave Latham added a comment -

          One more patch, fixes a typo and moves the enforcement of current thread from RowLock.release into RowLockContext.releaseLock for consistency.

          Apologies for the patch spam.

          Show
          Dave Latham added a comment - One more patch, fixes a typo and moves the enforcement of current thread from RowLock.release into RowLockContext.releaseLock for consistency. Apologies for the patch spam.
          Hide
          Dave Latham added a comment -

          Thanks, Ted, for taking a look. Sorry for changing the same file while you were looking.

          Should an assertion (or exception) be placed in the if block above ?

          The same behavior is there in the previous patch (v7) (and nearly the same in trunk as well) so it's not really a change. That said, this state cannot happen as I read the code, but if it did then the row locks may be stuck, so an exception sounds good to me so that it would be louder.

          Can RowLock class be package private ?

          It should be the same scope as the getRowLock method (needed for clients to keep references and release them) which I believe we decided to be public (for CPs for example).

          I've attached HBASE-8877-refCounts-v2.patch with the change to throw an exception on inconsistent row lock state.

          Show
          Dave Latham added a comment - Thanks, Ted, for taking a look. Sorry for changing the same file while you were looking. Should an assertion (or exception) be placed in the if block above ? The same behavior is there in the previous patch (v7) (and nearly the same in trunk as well) so it's not really a change. That said, this state cannot happen as I read the code, but if it did then the row locks may be stuck, so an exception sounds good to me so that it would be louder. Can RowLock class be package private ? It should be the same scope as the getRowLock method (needed for clients to keep references and release them) which I believe we decided to be public (for CPs for example). I've attached HBASE-8877 -refCounts-v2.patch with the change to throw an exception on inconsistent row lock state.
          Hide
          Dave Latham added a comment -

          Reattaching refCounts patch with check to enforce that only the same thread may release a lock that acquired it and so that Hadoop QA checks this file rather than the benchmark results.

          Show
          Dave Latham added a comment - Reattaching refCounts patch with check to enforce that only the same thread may release a lock that acquired it and so that Hadoop QA checks this file rather than the benchmark results.
          Hide
          Dave Latham added a comment -

          microbenchmark comparing latest patch (HBASE-8877-refCounts.patch) to trunk and HBASE-8877-v7.patch.

          Shows that HBASE-8877-refCounts is about the same as HBASE-8877-v7 and both faster than trunk.

          Show
          Dave Latham added a comment - microbenchmark comparing latest patch ( HBASE-8877 -refCounts.patch) to trunk and HBASE-8877 -v7.patch. Shows that HBASE-8877 -refCounts is about the same as HBASE-8877 -v7 and both faster than trunk.
          Hide
          Ted Yu added a comment -
          +        if (existingContext != this) {
          +          LOG.error("Internal row lock state inconsistent, should not happen, row: " + row);
          

          Should an assertion (or exception) be placed in the if block above ?

          +  public class RowLock {
          

          Can RowLock class be package private ?

          Show
          Ted Yu added a comment - + if (existingContext != this ) { + LOG.error( "Internal row lock state inconsistent, should not happen, row: " + row); Should an assertion (or exception) be placed in the if block above ? + public class RowLock { Can RowLock class be package private ?
          Hide
          Dave Latham added a comment -

          Alright, here's a patch with reentrant row locks and reference counts.

          This patch updates the row lock interface on HRegion to the following methods:

          • RowLock HRegion.getRowLock(final byte[] row, boolean waitForLock): acquire lock, optionally wait for lock
          • RowLock HRegion.getRowLock(final byte[] row): acquire lock, always waits for lock
          • RowLock.release(): releases the row lock

          The same thread can acquire multiple locks on the same row, but must release them all. doMiniBatchMutate now keeps a List of row locks (one for each row) and releases them all.

          While reviewing the clients I noticed that HRegion.increment and the duplicated code in HRegion.append both had possible paths to fail to release their row locks so I added a try/finally block to ensure they always release their locks.

          All the tests pass locally. I will run the microbenchmark too and post the results.

          Show
          Dave Latham added a comment - Alright, here's a patch with reentrant row locks and reference counts. This patch updates the row lock interface on HRegion to the following methods: RowLock HRegion.getRowLock(final byte[] row, boolean waitForLock): acquire lock, optionally wait for lock RowLock HRegion.getRowLock(final byte[] row): acquire lock, always waits for lock RowLock.release(): releases the row lock The same thread can acquire multiple locks on the same row, but must release them all. doMiniBatchMutate now keeps a List of row locks (one for each row) and releases them all. While reviewing the clients I noticed that HRegion.increment and the duplicated code in HRegion.append both had possible paths to fail to release their row locks so I added a try/finally block to ensure they always release their locks. All the tests pass locally. I will run the microbenchmark too and post the results.
          Hide
          stack added a comment -

          ...what if in the future a method does its own locking...

          You have a use case in mind Lars Hofhansl?

          Show
          stack added a comment - ...what if in the future a method does its own locking... You have a use case in mind Lars Hofhansl ?
          Hide
          Dave Latham added a comment -

          Lars Hofhansl Agreed that it's a bit fragile and that exactly was why I was initially concerned about checkAndMutate before I convinced myself it was safe and added the comment indicating that the nested release may have already released the locks.

          Reference counts would change the contract (from current patch) and require clients like doMiniBatchMutation to track the number of times it acquires the lock on each row and then release them the same number of times as opposed to a simple releaseAll. I can give it a shot to see how it turns out if you want to see it.

          Show
          Dave Latham added a comment - Lars Hofhansl Agreed that it's a bit fragile and that exactly was why I was initially concerned about checkAndMutate before I convinced myself it was safe and added the comment indicating that the nested release may have already released the locks. Reference counts would change the contract (from current patch) and require clients like doMiniBatchMutation to track the number of times it acquires the lock on each row and then release them the same number of times as opposed to a simple releaseAll. I can give it a shot to see how it turns out if you want to see it.
          Hide
          Lars Hofhansl added a comment -

          Do we need to do reference counting on the locks?

          Technically the patch is correct, since an HBase lock does not outlive an RPC request, but it still seems a tad fragile.
          For example what if in the future a method does its own locking: It acquires a lock on a row, does some logic, releases the lock. If a method higher up in the call stack had the row locked, it is now unlocked from under it.

          Show
          Lars Hofhansl added a comment - Do we need to do reference counting on the locks? Technically the patch is correct, since an HBase lock does not outlive an RPC request, but it still seems a tad fragile. For example what if in the future a method does its own locking: It acquires a lock on a row, does some logic, releases the lock. If a method higher up in the call stack had the row locked, it is now unlocked from under it.
          Hide
          Hadoop QA added a comment -

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

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

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

          +1 hadoop1.0. The patch compiles against the hadoop 1.0 profile.

          +1 hadoop2.0. The patch compiles against the hadoop 2.0 profile.

          +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 lineLengths. The patch does not introduce lines longer than 100

          +1 site. The mvn site goal succeeds with this patch.

          -1 core tests. The patch failed these unit tests:
          org.apache.hadoop.hbase.TestFullLogReconstruction
          org.apache.hadoop.hbase.security.access.TestAccessController

          Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/6343//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/6343//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-prefix-tree.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/6343//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-client.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/6343//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-common.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/6343//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-protocol.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/6343//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-server.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/6343//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop1-compat.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/6343//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-examples.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/6343//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop-compat.html
          Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/6343//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/12592355/HBASE-8877-v7.patch against trunk revision . +1 @author . The patch does not contain any @author tags. +1 tests included . The patch appears to include 12 new or modified tests. +1 hadoop1.0 . The patch compiles against the hadoop 1.0 profile. +1 hadoop2.0 . The patch compiles against the hadoop 2.0 profile. +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 lineLengths . The patch does not introduce lines longer than 100 +1 site . The mvn site goal succeeds with this patch. -1 core tests . The patch failed these unit tests: org.apache.hadoop.hbase.TestFullLogReconstruction org.apache.hadoop.hbase.security.access.TestAccessController Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/6343//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/6343//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-prefix-tree.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/6343//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-client.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/6343//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-common.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/6343//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-protocol.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/6343//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-server.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/6343//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop1-compat.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/6343//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-examples.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/6343//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop-compat.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/6343//console This message is automatically generated.
          Hide
          stack added a comment -

          +1 on patch. Will commit to trunk in few hours unless objection.

          No harm adding warning on 0.94 patch.

          Show
          stack added a comment - +1 on patch. Will commit to trunk in few hours unless objection. No harm adding warning on 0.94 patch.
          Hide
          Dave Latham added a comment -

          3 +1's on the dev mailing list for the removal of lock ids from the coprocessor, in addition to Andrew's +1 here. No -1's.

          One thought - should we commit to 0.94 a javadoc method at least warning that the locks are going to disappear from the coprocessor?

          Show
          Dave Latham added a comment - 3 +1's on the dev mailing list for the removal of lock ids from the coprocessor, in addition to Andrew's +1 here. No -1's. One thought - should we commit to 0.94 a javadoc method at least warning that the locks are going to disappear from the coprocessor?
          Hide
          Dave Latham added a comment -

          Thanks, Stack, for the review. Judging by your comments I believe you're addressing HBASE-8877-v6.patch (rather than HBASE-8877-0.94-v2.patch).

          A bit of doc on this would help: ... rowLocksHeldByThread

          Done.

          hould we deprecate stuff like this, public OperationStatus[] put(Put[] puts) throws IOException {, the methods that you have made into pass-throughs? (Fine in another issue)

          That's the only method that became a pass-through. I considered just removing it - we're already changing the HRegion public methods from row locks - I assumed that was OK to do in the singularity (not to mention the CP change). It's only used by HBaseFsck and 3 tests. I've now inlined it directly. Let me know if you think it needs to be deprecated for a release first.

          When we fall out here because we failed to acquire a lock, what happens? We apply all mutations for which we did get a lock? And then return the client reporting as failed those we did not get a lock on?

          If we did get any locks then we apply those mutations for which we did get locks, clean them up, return out to batchMutate which will continue again. If we failed to get any locks at all, then it means the timeout expired (default 30 seconds) and by my reading it still goes through all the cleanup methods then tries again. None of that logic was touched for these issues.

          Is this racey?

          Strictly rated G

          Could another thread come in and add to lockedRows before get to add this rowKey to the thread local?

          Nope, lockedRows.putIfAbsent gives atomicity so the next thread would get back a non null existingContext.

          This should be finals

          Agreed. Done.

          Attached HBASE-8877-v7.patch

          Show
          Dave Latham added a comment - Thanks, Stack, for the review. Judging by your comments I believe you're addressing HBASE-8877 -v6.patch (rather than HBASE-8877 -0.94-v2.patch). A bit of doc on this would help: ... rowLocksHeldByThread Done. hould we deprecate stuff like this, public OperationStatus[] put(Put[] puts) throws IOException {, the methods that you have made into pass-throughs? (Fine in another issue) That's the only method that became a pass-through. I considered just removing it - we're already changing the HRegion public methods from row locks - I assumed that was OK to do in the singularity (not to mention the CP change). It's only used by HBaseFsck and 3 tests. I've now inlined it directly. Let me know if you think it needs to be deprecated for a release first. When we fall out here because we failed to acquire a lock, what happens? We apply all mutations for which we did get a lock? And then return the client reporting as failed those we did not get a lock on? If we did get any locks then we apply those mutations for which we did get locks, clean them up, return out to batchMutate which will continue again. If we failed to get any locks at all, then it means the timeout expired (default 30 seconds) and by my reading it still goes through all the cleanup methods then tries again. None of that logic was touched for these issues. Is this racey? Strictly rated G Could another thread come in and add to lockedRows before get to add this rowKey to the thread local? Nope, lockedRows.putIfAbsent gives atomicity so the next thread would get back a non null existingContext. This should be finals Agreed. Done. Attached HBASE-8877 -v7.patch
          Hide
          stack added a comment -

          Very nice patch. Nice cleanup.

          A bit of doc on this would help:

          rowLocksHeldByThread

          It is a nice trick. It is worth talking up!

          Should we deprecate stuff like this, public OperationStatus[] put(Put[] puts) throws IOException {, the methods that you have made into pass-throughs? (Fine in another issue)

          When we fall out here because we failed to acquire a lock, what happens? We apply all mutations for which we did get a lock? And then return the client reporting as failed those we did not get a lock on?

          assert !shouldBlock : "Should never fail to get lock when blocking";
          break; // stop acquiring more rows for this batch

          Is this racey?

          +        RowLockContext existingContext = lockedRows.putIfAbsent(rowKey, rowLockContext);
          +        if (existingContext == null) {
          +          // Row is not already locked by any thread, add it to this thread's list
          +          rowLocksHeldByThread.get().add(rowKey);
          

          Could another thread come in and add to lockedRows before get to add this rowKey to the thread local?

          This should be finals

          + private CountDownLatch latch;
          + private Thread thread;

          Otherwise +1 on commit.

          Show
          stack added a comment - Very nice patch. Nice cleanup. A bit of doc on this would help: rowLocksHeldByThread It is a nice trick. It is worth talking up! Should we deprecate stuff like this, public OperationStatus[] put(Put[] puts) throws IOException {, the methods that you have made into pass-throughs? (Fine in another issue) When we fall out here because we failed to acquire a lock, what happens? We apply all mutations for which we did get a lock? And then return the client reporting as failed those we did not get a lock on? assert !shouldBlock : "Should never fail to get lock when blocking"; break; // stop acquiring more rows for this batch Is this racey? + RowLockContext existingContext = lockedRows.putIfAbsent(rowKey, rowLockContext); + if (existingContext == null ) { + // Row is not already locked by any thread, add it to this thread's list + rowLocksHeldByThread.get().add(rowKey); Could another thread come in and add to lockedRows before get to add this rowKey to the thread local? This should be finals + private CountDownLatch latch; + private Thread thread; Otherwise +1 on commit.
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12592195/HBASE-8877-0.94-v2.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 patch. The patch command could not apply the patch.

          Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/6329//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/12592195/HBASE-8877-0.94-v2.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 patch . The patch command could not apply the patch. Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/6329//console This message is automatically generated.
          Hide
          Dave Latham added a comment -

          Thanks, Ted and Lars for the reviews.

          Here's a new patch, HBASE-8877-0.94-v2.patch which fixes the typo in the log messages "Releases" -> "Released".

          Show
          Dave Latham added a comment - Thanks, Ted and Lars for the reviews. Here's a new patch, HBASE-8877 -0.94-v2.patch which fixes the typo in the log messages "Releases" -> "Released".
          Hide
          Lars Hofhansl added a comment -

          0.94 patch looks good. I played through scenarios where the client provides a lock id, it should work find in all cases (as long as obtainRowLock does not acquire reentrant locks, which is the case).

          This worries me a little bit:

          -      LOG.warn("Release unknown lockId: " + lockId);
          +      // Row not locked.  Likely released by the same thread already.
                 return;
          

          We're loosing a double check here. Not a deal breaker, though.

          Also, while we're at it, let's fix the typo here:

          +    if (rowLockContext == null) {
                 LOG.error("Releases row not locked, lockId: " + lockId + " row: "
                     + rowKey);
          

          "Releases" -> "Released".

          Show
          Lars Hofhansl added a comment - 0.94 patch looks good. I played through scenarios where the client provides a lock id, it should work find in all cases (as long as obtainRowLock does not acquire reentrant locks, which is the case). This worries me a little bit: - LOG.warn( "Release unknown lockId: " + lockId); + // Row not locked. Likely released by the same thread already. return ; We're loosing a double check here. Not a deal breaker, though. Also, while we're at it, let's fix the typo here: + if (rowLockContext == null ) { LOG.error( "Releases row not locked, lockId: " + lockId + " row: " + rowKey); "Releases" -> "Released".
          Hide
          Ted Yu added a comment -

          +1 on patch v6.

          Show
          Ted Yu added a comment - +1 on patch v6.
          Hide
          Hadoop QA added a comment -

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

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

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

          +1 hadoop1.0. The patch compiles against the hadoop 1.0 profile.

          +1 hadoop2.0. The patch compiles against the hadoop 2.0 profile.

          +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 lineLengths. The patch does not introduce lines longer than 100

          +1 site. The mvn site goal succeeds with this patch.

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

          Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/6326//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/6326//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-prefix-tree.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/6326//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-client.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/6326//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-common.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/6326//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-protocol.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/6326//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-server.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/6326//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop1-compat.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/6326//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-examples.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/6326//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop-compat.html
          Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/6326//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/12592154/HBASE-8877-v6.patch against trunk revision . +1 @author . The patch does not contain any @author tags. +1 tests included . The patch appears to include 9 new or modified tests. +1 hadoop1.0 . The patch compiles against the hadoop 1.0 profile. +1 hadoop2.0 . The patch compiles against the hadoop 2.0 profile. +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 lineLengths . The patch does not introduce lines longer than 100 +1 site . The mvn site goal succeeds with this patch. +1 core tests . The patch passed unit tests in . Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/6326//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/6326//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-prefix-tree.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/6326//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-client.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/6326//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-common.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/6326//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-protocol.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/6326//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-server.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/6326//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop1-compat.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/6326//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-examples.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/6326//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop-compat.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/6326//console This message is automatically generated.
          Hide
          Dave Latham added a comment -

          Note the above comment is referencing the HRegion class.

          Show
          Dave Latham added a comment - Note the above comment is referencing the HRegion class.
          Hide
          Dave Latham added a comment -

          Last patch failed on TestHeapSize. This surprised me since it had passed for me locally. Turns out locally I was on a 64-bit VM and Jenkins ran it out a 32-bit. It also turns out the current trunk had the FIXED_OVERHEAD off by +1 long, -1 reference and -2 booleans which just happened to pass the tests due to alignment but after this change began to fail. Digging further it's obvious the DEEP_OVERHEAD is missing many fields as is the heapSize() method. Looks like we may not be even trying any more. Has there already been any discussion about this?

          Attaching v6 patch. In this patch I've corrected FIXED_OVERHEAD which gets the test passing. I also added a couple missing AtomicIntegers to DEEP_OVERHEAD as well as a comment specifying all the other missing things which I think is too great a scope for me to try to fix in this patch, but at least serves as a warning to others.

          Show
          Dave Latham added a comment - Last patch failed on TestHeapSize. This surprised me since it had passed for me locally. Turns out locally I was on a 64-bit VM and Jenkins ran it out a 32-bit. It also turns out the current trunk had the FIXED_OVERHEAD off by +1 long, -1 reference and -2 booleans which just happened to pass the tests due to alignment but after this change began to fail. Digging further it's obvious the DEEP_OVERHEAD is missing many fields as is the heapSize() method. Looks like we may not be even trying any more. Has there already been any discussion about this? Attaching v6 patch. In this patch I've corrected FIXED_OVERHEAD which gets the test passing. I also added a couple missing AtomicIntegers to DEEP_OVERHEAD as well as a comment specifying all the other missing things which I think is too great a scope for me to try to fix in this patch, but at least serves as a warning to others.
          Hide
          Hadoop QA added a comment -

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

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

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

          +1 hadoop1.0. The patch compiles against the hadoop 1.0 profile.

          +1 hadoop2.0. The patch compiles against the hadoop 2.0 profile.

          +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 lineLengths. The patch does not introduce lines longer than 100

          +1 site. The mvn site goal succeeds with this patch.

          -1 core tests. The patch failed these unit tests:
          org.apache.hadoop.hbase.io.TestHeapSize

          Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/6325//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/6325//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-protocol.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/6325//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-client.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/6325//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-examples.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/6325//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop1-compat.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/6325//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-prefix-tree.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/6325//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-common.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/6325//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-server.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/6325//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop-compat.html
          Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/6325//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/12592151/HBASE-8877-v5.patch against trunk revision . +1 @author . The patch does not contain any @author tags. +1 tests included . The patch appears to include 9 new or modified tests. +1 hadoop1.0 . The patch compiles against the hadoop 1.0 profile. +1 hadoop2.0 . The patch compiles against the hadoop 2.0 profile. +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 lineLengths . The patch does not introduce lines longer than 100 +1 site . The mvn site goal succeeds with this patch. -1 core tests . The patch failed these unit tests: org.apache.hadoop.hbase.io.TestHeapSize Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/6325//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/6325//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-protocol.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/6325//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-client.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/6325//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-examples.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/6325//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop1-compat.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/6325//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-prefix-tree.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/6325//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-common.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/6325//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-server.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/6325//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop-compat.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/6325//console This message is automatically generated.
          Hide
          Dave Latham added a comment -

          Here's a v5 patch which merges in changes from trunk since v4 could no longer apply.

          Show
          Dave Latham added a comment - Here's a v5 patch which merges in changes from trunk since v4 could no longer apply.
          Hide
          Hadoop QA added a comment -

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

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

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

          -1 patch. The patch command could not apply the patch.

          Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/6324//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/12592150/HBASE-8877-v4.patch against trunk revision . +1 @author . The patch does not contain any @author tags. +1 tests included . The patch appears to include 9 new or modified tests. -1 patch . The patch command could not apply the patch. Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/6324//console This message is automatically generated.
          Hide
          Dave Latham added a comment -

          Reattached HBASE-8877-v4.patch for Hadoop QA to notice.

          Show
          Dave Latham added a comment - Reattached HBASE-8877 -v4.patch for Hadoop QA to notice.
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12592108/hbase-8877-v4-microbenchmark.txt
          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 patch. The patch command could not apply the patch.

          Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/6319//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/12592108/hbase-8877-v4-microbenchmark.txt 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 patch . The patch command could not apply the patch. Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/6319//console This message is automatically generated.
          Hide
          Dave Latham added a comment -

          Here's the patch for some minimal reentrant locks for the 0.94 branch. It is similar to Anoop's patch in HBASE-8806. Also attached are the perf benchmark results showing it is as fast or faster than current 0.94.

          Show
          Dave Latham added a comment - Here's the patch for some minimal reentrant locks for the 0.94 branch. It is similar to Anoop's patch in HBASE-8806 . Also attached are the perf benchmark results showing it is as fast or faster than current 0.94.
          Hide
          Dave Latham added a comment -

          What was the unit for min/max/mean values ?

          Each trial measures the number of milliseconds to execute a batchMutate of 25000 puts.

          Show
          Dave Latham added a comment - What was the unit for min/max/mean values ? Each trial measures the number of milliseconds to execute a batchMutate of 25000 puts.
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12592082/hbase-8807-v4-microbenchmark.txt
          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 patch. The patch command could not apply the patch.

          Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/6317//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/12592082/hbase-8807-v4-microbenchmark.txt 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 patch . The patch command could not apply the patch. Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/6317//console This message is automatically generated.
          Hide
          Ted Yu added a comment -

          Nice results.

          What was the unit for min/max/mean values ?

          Show
          Ted Yu added a comment - Nice results. What was the unit for min/max/mean values ?
          Hide
          Dave Latham added a comment -

          churro morales ran the patch v3 through his microbenchmark on mini batch puts. It revealed an issue in the patch that the list of locks from the same thread held duplicates resulting in erroneous errors being logged. Here's a v4 patch fixing that issue. Here are also results for the microbenchmark on this patch that show it as fast or faster than the existing trunk.

          If this looks good, It think it's ready for commit to trunk and 0.96.

          Should also have a patch soon for 0.94 that is similar to Anoop's patch on HBASE-8806 but only uses reentrancy for doMiniBatchMutation and mutateRowsWithLocks to avoid problems with client side row locks. Verifying tests and perf for that one.

          Show
          Dave Latham added a comment - churro morales ran the patch v3 through his microbenchmark on mini batch puts. It revealed an issue in the patch that the list of locks from the same thread held duplicates resulting in erroneous errors being logged. Here's a v4 patch fixing that issue. Here are also results for the microbenchmark on this patch that show it as fast or faster than the existing trunk. If this looks good, It think it's ready for commit to trunk and 0.96. Should also have a patch soon for 0.94 that is similar to Anoop's patch on HBASE-8806 but only uses reentrancy for doMiniBatchMutation and mutateRowsWithLocks to avoid problems with client side row locks. Verifying tests and perf for that one.
          Hide
          Ted Yu added a comment -

          +1 on patch v3.

          Show
          Ted Yu added a comment - +1 on patch v3.
          Hide
          Hadoop QA added a comment -

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

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

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

          +1 hadoop1.0. The patch compiles against the hadoop 1.0 profile.

          +1 hadoop2.0. The patch compiles against the hadoop 2.0 profile.

          -1 javadoc. The javadoc tool appears to have generated 2 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 lineLengths. The patch does not introduce lines longer than 100

          +1 site. The mvn site goal succeeds with this patch.

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

          Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/6237//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/6237//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-prefix-tree.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/6237//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-client.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/6237//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-common.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/6237//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-protocol.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/6237//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-server.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/6237//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop1-compat.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/6237//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-examples.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/6237//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop-compat.html
          Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/6237//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/12591212/HBASE-8877-v3.patch against trunk revision . +1 @author . The patch does not contain any @author tags. +1 tests included . The patch appears to include 9 new or modified tests. +1 hadoop1.0 . The patch compiles against the hadoop 1.0 profile. +1 hadoop2.0 . The patch compiles against the hadoop 2.0 profile. -1 javadoc . The javadoc tool appears to have generated 2 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 lineLengths . The patch does not introduce lines longer than 100 +1 site . The mvn site goal succeeds with this patch. +1 core tests . The patch passed unit tests in . Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/6237//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/6237//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-prefix-tree.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/6237//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-client.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/6237//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-common.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/6237//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-protocol.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/6237//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-server.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/6237//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop1-compat.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/6237//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-examples.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/6237//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop-compat.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/6237//console This message is automatically generated.
          Hide
          Dave Latham added a comment -

          Ah, ok I've reverted them in this patch (v3) to public so that CPs can acquire and release locks if desired. Thanks, Ted, Anoop, Ramkrishna.

          Show
          Dave Latham added a comment - Ah, ok I've reverted them in this patch (v3) to public so that CPs can acquire and release locks if desired. Thanks, Ted, Anoop, Ramkrishna.
          Hide
          ramkrishna.s.vasudevan added a comment -

          Should we allow acquire and release locks from CPs?

          Is this in lieu with having some cross region locks using CPs? May be we need to make the tryRowLock and releaseRowLocks to be public in case we are giving CPs the option to acquire and release the locks. Do correct me if am wrong.

          Show
          ramkrishna.s.vasudevan added a comment - Should we allow acquire and release locks from CPs? Is this in lieu with having some cross region locks using CPs? May be we need to make the tryRowLock and releaseRowLocks to be public in case we are giving CPs the option to acquire and release the locks. Do correct me if am wrong.
          Hide
          Anoop Sam John added a comment -

          Should we allow acquire and release locks from CPs?

          Show
          Anoop Sam John added a comment - Should we allow acquire and release locks from CPs?
          Hide
          Dave Latham added a comment -

          Looks like the following methods should be public:

          Why do you think they should be public methods?

          Show
          Dave Latham added a comment - Looks like the following methods should be public: Why do you think they should be public methods?
          Hide
          Ted Yu added a comment -
          -  public Integer getLock(Integer lockid, byte [] row, boolean waitForLock)
          

          Looks like the following methods should be public:

          +  @VisibleForTesting boolean tryRowLock(byte[] row) {
          +  @VisibleForTesting void getRowLock(byte[] row) throws IOException {
          
          Show
          Ted Yu added a comment - - public Integer getLock( Integer lockid, byte [] row, boolean waitForLock) Looks like the following methods should be public: + @VisibleForTesting boolean tryRowLock( byte [] row) { + @VisibleForTesting void getRowLock( byte [] row) throws IOException {
          Hide
          Hadoop QA added a comment -

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

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

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

          +1 hadoop1.0. The patch compiles against the hadoop 1.0 profile.

          +1 hadoop2.0. The patch compiles against the hadoop 2.0 profile.

          -1 javadoc. The javadoc tool appears to have generated 2 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 lineLengths. The patch does not introduce lines longer than 100

          +1 site. The mvn site goal succeeds with this patch.

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

          Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/6232//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/6232//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-prefix-tree.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/6232//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-client.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/6232//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-common.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/6232//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-protocol.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/6232//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-server.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/6232//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop1-compat.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/6232//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-examples.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/6232//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop-compat.html
          Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/6232//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/12591094/HBASE-8877-v2.patch against trunk revision . +1 @author . The patch does not contain any @author tags. +1 tests included . The patch appears to include 9 new or modified tests. +1 hadoop1.0 . The patch compiles against the hadoop 1.0 profile. +1 hadoop2.0 . The patch compiles against the hadoop 2.0 profile. -1 javadoc . The javadoc tool appears to have generated 2 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 lineLengths . The patch does not introduce lines longer than 100 +1 site . The mvn site goal succeeds with this patch. -1 core tests . The patch failed these unit tests: org.apache.hadoop.hbase.TestIOFencing Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/6232//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/6232//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-prefix-tree.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/6232//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-client.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/6232//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-common.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/6232//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-protocol.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/6232//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-server.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/6232//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop1-compat.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/6232//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-examples.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/6232//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop-compat.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/6232//console This message is automatically generated.
          Hide
          Dave Latham added a comment -

          +1 on the CP interface change. There might (in theory - doubt in practice) have been some utility before for passing through lock ids but if this issue is committed obviously no longer. This proposal is only for >= 0.95?

          Thanks, Andrew. Yes, this proposal is for 0.95+ only.

          Show
          Dave Latham added a comment - +1 on the CP interface change. There might (in theory - doubt in practice) have been some utility before for passing through lock ids but if this issue is committed obviously no longer. This proposal is only for >= 0.95? Thanks, Andrew. Yes, this proposal is for 0.95+ only.
          Hide
          Dave Latham added a comment -

          Thanks, Ted, for the review. I sent out a note to the dev list for input on the removal of lock ids. Here's a new patch in which I fixed javadoc on tryRowLock and made RowLockContext private.

          Show
          Dave Latham added a comment - Thanks, Ted, for the review. I sent out a note to the dev list for input on the removal of lock ids. Here's a new patch in which I fixed javadoc on tryRowLock and made RowLockContext private.
          Hide
          Andrew Purtell added a comment - - edited

          +1 on the CP interface change. There might (in theory - doubt in practice) have been some utility before for passing through lock ids but if this issue is committed obviously no longer. This proposal is only for >= 0.95?

          Edit: fix JIRA formatting.

          Show
          Andrew Purtell added a comment - - edited +1 on the CP interface change. There might (in theory - doubt in practice) have been some utility before for passing through lock ids but if this issue is committed obviously no longer. This proposal is only for >= 0.95? Edit: fix JIRA formatting.
          Hide
          Hadoop QA added a comment -

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

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

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

          +1 hadoop1.0. The patch compiles against the hadoop 1.0 profile.

          +1 hadoop2.0. The patch compiles against the hadoop 2.0 profile.

          -1 javadoc. The javadoc tool appears to have generated 2 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 lineLengths. The patch does not introduce lines longer than 100

          +1 site. The mvn site goal succeeds with this patch.

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

          Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/6224//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/6224//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-protocol.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/6224//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-client.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/6224//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-examples.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/6224//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop1-compat.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/6224//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-prefix-tree.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/6224//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-common.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/6224//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-server.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/6224//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop-compat.html
          Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/6224//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/12591013/HBASE-8877.patch against trunk revision . +1 @author . The patch does not contain any @author tags. +1 tests included . The patch appears to include 9 new or modified tests. +1 hadoop1.0 . The patch compiles against the hadoop 1.0 profile. +1 hadoop2.0 . The patch compiles against the hadoop 2.0 profile. -1 javadoc . The javadoc tool appears to have generated 2 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 lineLengths . The patch does not introduce lines longer than 100 +1 site . The mvn site goal succeeds with this patch. +1 core tests . The patch passed unit tests in . Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/6224//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/6224//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-protocol.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/6224//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-client.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/6224//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-examples.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/6224//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop1-compat.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/6224//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-prefix-tree.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/6224//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-common.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/6224//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-server.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/6224//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop-compat.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/6224//console This message is automatically generated.
          Hide
          Ted Yu added a comment -
          -  public OperationStatus[] batchMutate(
          -      Pair<Mutation, Integer>[] mutationsAndLocks) throws IOException {
          -    return batchMutate(mutationsAndLocks, false);
          +  public OperationStatus[] batchMutate(Mutation[] mutations) throws IOException {
          

          Should dev@hbase be polled for the removal of lock Id ?

             /**
          -   * Returns existing row lock if found, otherwise
          -   * obtains a new row lock and returns it.
          -   * @param lockid requested by the user, or null if the user didn't already hold lock
          -   * @param row the row to lock
          -   * @param waitForLock if true, will block until the lock is available, otherwise will
          -   * simply return null if it could not acquire the lock.
          -   * @return lockid or null if waitForLock is false and the lock was unavailable.
          +   * public void getRowLock(byte [] row) throws IOException {
          +   * internalObtainRowLock(row, true);
              */
          

          The change in javadoc seems unnecessary.

          +  static class RowLockContext {
          

          RowLockContext can be private, right ?

          Show
          Ted Yu added a comment - - public OperationStatus[] batchMutate( - Pair<Mutation, Integer >[] mutationsAndLocks) throws IOException { - return batchMutate(mutationsAndLocks, false ); + public OperationStatus[] batchMutate(Mutation[] mutations) throws IOException { Should dev@hbase be polled for the removal of lock Id ? /** - * Returns existing row lock if found, otherwise - * obtains a new row lock and returns it. - * @param lockid requested by the user, or null if the user didn't already hold lock - * @param row the row to lock - * @param waitForLock if true , will block until the lock is available, otherwise will - * simply return null if it could not acquire the lock. - * @ return lockid or null if waitForLock is false and the lock was unavailable. + * public void getRowLock( byte [] row) throws IOException { + * internalObtainRowLock(row, true ); */ The change in javadoc seems unnecessary. + static class RowLockContext { RowLockContext can be private, right ?
          Hide
          Dave Latham added a comment -

          A few changes in this patch compared to the latest similar patch on HBASE-8806:

          • fixed the heap size logic to match the new fields
          • wrapped a couple lines that were longer than 100 chars
          • changed releaseMyRowLocks method to free the list of locks from the thread local so that each thread doesn't wind up having very long empty lists hanging around
          • changed the row lock methods from public to package private as they are only used inside HRegion (and by some tests)
          • added some comments to checkAndMutate to make clear row it's reusing locks
          • renamed new thread local field to rowLocksHeldByThread
          Show
          Dave Latham added a comment - A few changes in this patch compared to the latest similar patch on HBASE-8806 : fixed the heap size logic to match the new fields wrapped a couple lines that were longer than 100 chars changed releaseMyRowLocks method to free the list of locks from the thread local so that each thread doesn't wind up having very long empty lists hanging around changed the row lock methods from public to package private as they are only used inside HRegion (and by some tests) added some comments to checkAndMutate to make clear row it's reusing locks renamed new thread local field to rowLocksHeldByThread
          Hide
          Dave Latham added a comment -

          Here's a patch implementing reentrant row locks. It changes the locking interface to these 3 methods:

          • void getRowLock(byte[] row) throws IOException (acquire, throw if unable to acqurie before timeout)
          • boolean tryRowLock(byte[] row) (acquire without wait and return true iff successful)
          • void releaseMyRowLocks() (release all row locks held by the current thread)
          Show
          Dave Latham added a comment - Here's a patch implementing reentrant row locks. It changes the locking interface to these 3 methods: void getRowLock(byte[] row) throws IOException (acquire, throw if unable to acqurie before timeout) boolean tryRowLock(byte[] row) (acquire without wait and return true iff successful) void releaseMyRowLocks() (release all row locks held by the current thread)

            People

            • Assignee:
              Dave Latham
              Reporter:
              Dave Latham
            • Votes:
              0 Vote for this issue
              Watchers:
              12 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development