HBase
  1. HBase
  2. HBASE-2295

Row locks may deadlock with themselves

    Details

    • Type: Bug Bug
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 0.90.0
    • Component/s: regionserver
    • Labels:
      None
    • Hadoop Flags:
      Reviewed

      Description

      Row locks in HRegion are keyed by a int-sized hash of the row key. It's perfectly possible for two rows to hash to the same key. So, if any client tries to lock both rows, it will deadlock with itself. Switching to a 64-bit hash is an improvement but still sketchy.

      1. rowLockDeadlock.txt
        4 kB
        dhruba borthakur
      2. rowLockDeadlock2.txt
        4 kB
        dhruba borthakur

        Issue Links

          Activity

          Todd Lipcon created issue -
          Hide
          dhruba borthakur added a comment -

          are you talking about the multiPut where multiple rows could be updated atomically by an app?

          Show
          dhruba borthakur added a comment - are you talking about the multiPut where multiple rows could be updated atomically by an app?
          Hide
          ryan rawson added a comment -

          The multiput does not promise nor do multi row atomic puts. It actually just
          calls the array put API.

          On Mar 7, 2010 11:11 AM, "dhruba borthakur (JIRA)" <jira@apache.org> wrote:

          [
          https://issues.apache.org/jira/browse/HBASE-2295?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12842465#action_12842465]

          dhruba borthakur commented on HBASE-2295:
          -----------------------------------------

          are you talking about the multiPut where multiple rows could be updated
          atomically by an app?

          perfectly possible for two rows to hash to the same key. So, if any client
          tries to lock both rows, it will deadlock with itself. Switching to a 64-bit
          hash is an improvement but still sketchy.


          This message is automatically generated by JIRA.
          -
          You can reply to this email to add a comment to the issue online.

          Show
          ryan rawson added a comment - The multiput does not promise nor do multi row atomic puts. It actually just calls the array put API. On Mar 7, 2010 11:11 AM, "dhruba borthakur (JIRA)" <jira@apache.org> wrote: [ https://issues.apache.org/jira/browse/HBASE-2295?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12842465#action_12842465 ] dhruba borthakur commented on HBASE-2295 : ----------------------------------------- are you talking about the multiPut where multiple rows could be updated atomically by an app? perfectly possible for two rows to hash to the same key. So, if any client tries to lock both rows, it will deadlock with itself. Switching to a 64-bit hash is an improvement but still sketchy. – This message is automatically generated by JIRA. - You can reply to this email to add a comment to the issue online.
          Hide
          dhruba borthakur added a comment -

          1. Changed locksToRows to be a Set (instead of a Map)
          2. Generate sequential lockids, and in the case of collision use a Random number to start from a new origin

          Show
          dhruba borthakur added a comment - 1. Changed locksToRows to be a Set (instead of a Map) 2. Generate sequential lockids, and in the case of collision use a Random number to start from a new origin
          dhruba borthakur made changes -
          Field Original Value New Value
          Attachment rowLockDeadlock.txt [ 12438164 ]
          Hide
          dhruba borthakur added a comment -

          The patch I uploaded is for hbase 0.20.

          BTW, when I downloaded the hbase trunk, I do not see any build.xml files in the source repo at all. Do I need to use something other than "ant test"?

          Show
          dhruba borthakur added a comment - The patch I uploaded is for hbase 0.20. BTW, when I downloaded the hbase trunk, I do not see any build.xml files in the source repo at all. Do I need to use something other than "ant test"?
          Hide
          Todd Lipcon added a comment -

          Hey Dhruba,

          I don't think this patch works right. You can't have a HashSet<byte[]> since byte[]'s hashcode is identity based, not content, right?

          Show
          Todd Lipcon added a comment - Hey Dhruba, I don't think this patch works right. You can't have a HashSet<byte[]> since byte[]'s hashcode is identity based, not content, right?
          Hide
          dhruba borthakur added a comment -

          I agree, that is the case for all native items like byte[]. I will change it.

          Show
          dhruba borthakur added a comment - I agree, that is the case for all native items like byte[]. I will change it.
          Hide
          stack added a comment -

          @Dhruba

          If you want to use a SortedSet and pass a comparator instead, so you can retain byte [] elements, there is a bytes comparator here: http://hadoop.apache.org/hbase/docs/r0.20.3/api/org/apache/hadoop/hbase/util/Bytes.html#BYTES_COMPARATOR Otherwise, patch looks good.

          Regards TRUNK, its been mavenized so to build it, you need maven – see http://wiki.apache.org/hadoop/Hbase/MavenPrimer – and then to build and run tests do something like mvn install

          Show
          stack added a comment - @Dhruba If you want to use a SortedSet and pass a comparator instead, so you can retain byte [] elements, there is a bytes comparator here: http://hadoop.apache.org/hbase/docs/r0.20.3/api/org/apache/hadoop/hbase/util/Bytes.html#BYTES_COMPARATOR Otherwise, patch looks good. Regards TRUNK, its been mavenized so to build it, you need maven – see http://wiki.apache.org/hadoop/Hbase/MavenPrimer – and then to build and run tests do something like mvn install
          Hide
          dhruba borthakur added a comment -

          This patch incorporates Todd's and Stack's comments. And is for hbase trunk.

          Show
          dhruba borthakur added a comment - This patch incorporates Todd's and Stack's comments. And is for hbase trunk.
          dhruba borthakur made changes -
          Attachment rowLockDeadlock2.txt [ 12438269 ]
          Hide
          stack added a comment -

          Committed to trunk and branch. Thanks for patch Dhruba.

          Show
          stack added a comment - Committed to trunk and branch. Thanks for patch Dhruba.
          stack made changes -
          Status Open [ 1 ] Resolved [ 5 ]
          Hadoop Flags [Reviewed]
          Fix Version/s 0.20.4 [ 12314496 ]
          Fix Version/s 0.21.0 [ 12313607 ]
          Resolution Fixed [ 1 ]
          stack made changes -
          Assignee dhruba borthakur [ dhruba ]
          Hide
          Todd Lipcon added a comment -

          This already went in, but one quick note on the final patch: rather than checking locksToRows.size() > 0, it's probably faster to check !locksToRows.isEmpty()

          Does this need to be a followup JIRA?

          Show
          Todd Lipcon added a comment - This already went in, but one quick note on the final patch: rather than checking locksToRows.size() > 0, it's probably faster to check !locksToRows.isEmpty() Does this need to be a followup JIRA?
          Hide
          stack added a comment -

          I agree isEmpty is better than size... I'll just put fix in under this issue. Thanks Todd.

          I changed it to do this instead on trunk and branch:

                while (!this.lockedRows.isEmpty()) {
                  if (LOG.isDebugEnabled()) {
                    LOG.debug("Waiting on " + this.lockedRows.size() + " row locks");
                  }
          ...
          
          Show
          stack added a comment - I agree isEmpty is better than size... I'll just put fix in under this issue. Thanks Todd. I changed it to do this instead on trunk and branch: while (! this .lockedRows.isEmpty()) { if (LOG.isDebugEnabled()) { LOG.debug( "Waiting on " + this .lockedRows.size() + " row locks" ); } ...
          stack made changes -
          Fix Version/s 0.20.5 [ 12314800 ]
          Fix Version/s 0.20.4 [ 12314496 ]
          Hide
          stack added a comment -

          Marking these as fixed against 0.21.0 rather than against 0.20.5.

          Show
          stack added a comment - Marking these as fixed against 0.21.0 rather than against 0.20.5.
          stack made changes -
          Fix Version/s 0.20.5 [ 12314800 ]
          Dave Latham made changes -
          Link This issue relates to HBASE-3894 [ HBASE-3894 ]
          Transition Time In Source Status Execution Times Last Executer Last Execution Date
          Open Open Resolved Resolved
          3d 17h 39m 1 stack 09/Mar/10 17:11

            People

            • Assignee:
              dhruba borthakur
              Reporter:
              Todd Lipcon
            • Votes:
              0 Vote for this issue
              Watchers:
              5 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development