HBase
  1. HBase
  2. HBASE-6930

[89-fb] Avoid acquiring the same row lock repeatedly

    Details

    • Type: Bug Bug
    • Status: Open
    • Priority: Major Major
    • Resolution: Unresolved
    • Affects Version/s: None
    • Fix Version/s: None
    • Component/s: None
    • Labels:
      None

      Description

      When processing the multiPut, multiMutations or multiDelete operations, each IPC handler thread tries to acquire a lock for each row key in these batches. If there are duplicated row keys in these batches, previously the IPC handler thread will repeatedly acquire the same row key again and again.

      So the optimization is to sort each batch operation based on the row key in the client side, and skip acquiring the same row lock repeatedly in the server side.

        Activity

        Liyin Tang created issue -
        Hide
        Phabricator added a comment -

        mbautin requested code review of "[jira] HBASE-6930 [89-fb] Fix TestThriftServerLegacy: notifyAll should be inside synchronized block".
        Reviewers: Kannan, Liyin, Karthik, JIRA

        There were a couple of reasons why TestThriftServerLegacy has been failing recently in the HBase 89-fb branch:

        • rHBASEEIGHTNINEFBBRANCH1393468 was calling notifyAll outside a synchronized block
        • rHBASEEIGHTNINEFBBRANCH1391219 changed the meaning of a null expected value passed to checkAndMutate but that was not reflected in the Thrift handler

        TEST PLAN
        Run TestThriftServerLegacy

        REVISION DETAIL
        https://reviews.facebook.net/D5841

        AFFECTED FILES
        src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java

        MANAGE HERALD DIFFERENTIAL RULES
        https://reviews.facebook.net/herald/view/differential/

        WHY DID I GET THIS EMAIL?
        https://reviews.facebook.net/herald/transcript/13833/

        To: Kannan, Liyin, Karthik, JIRA, mbautin

        Show
        Phabricator added a comment - mbautin requested code review of " [jira] HBASE-6930 [89-fb] Fix TestThriftServerLegacy: notifyAll should be inside synchronized block". Reviewers: Kannan, Liyin, Karthik, JIRA There were a couple of reasons why TestThriftServerLegacy has been failing recently in the HBase 89-fb branch: rHBASEEIGHTNINEFBBRANCH1393468 was calling notifyAll outside a synchronized block rHBASEEIGHTNINEFBBRANCH1391219 changed the meaning of a null expected value passed to checkAndMutate but that was not reflected in the Thrift handler TEST PLAN Run TestThriftServerLegacy REVISION DETAIL https://reviews.facebook.net/D5841 AFFECTED FILES src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java MANAGE HERALD DIFFERENTIAL RULES https://reviews.facebook.net/herald/view/differential/ WHY DID I GET THIS EMAIL? https://reviews.facebook.net/herald/transcript/13833/ To: Kannan, Liyin, Karthik, JIRA, mbautin
        Phabricator made changes -
        Field Original Value New Value
        Attachment D5841.1.patch [ 12547610 ]
        Hide
        Phabricator added a comment -

        mbautin updated the revision "[jira] HBASE-6930 [89-fb] Fix TestThriftServerLegacy: notifyAll should be inside synchronized block, and a null checkAndMutate expected value should be handled correctly".
        Reviewers: Kannan, Liyin, Karthik, JIRA

        Adding ThriftServerRunner fixes

        REVISION DETAIL
        https://reviews.facebook.net/D5841

        AFFECTED FILES
        src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
        src/main/java/org/apache/hadoop/hbase/thrift/ThriftServerRunner.java
        src/test/java/org/apache/hadoop/hbase/thrift/TestThriftServerLegacy.java

        To: Kannan, Liyin, Karthik, JIRA, mbautin

        Show
        Phabricator added a comment - mbautin updated the revision " [jira] HBASE-6930 [89-fb] Fix TestThriftServerLegacy: notifyAll should be inside synchronized block, and a null checkAndMutate expected value should be handled correctly". Reviewers: Kannan, Liyin, Karthik, JIRA Adding ThriftServerRunner fixes REVISION DETAIL https://reviews.facebook.net/D5841 AFFECTED FILES src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java src/main/java/org/apache/hadoop/hbase/thrift/ThriftServerRunner.java src/test/java/org/apache/hadoop/hbase/thrift/TestThriftServerLegacy.java To: Kannan, Liyin, Karthik, JIRA, mbautin
        Phabricator made changes -
        Attachment D5841.2.patch [ 12547611 ]
        Hide
        Phabricator added a comment -

        Liyin has accepted the revision "[jira] HBASE-6930 [89-fb] Fix TestThriftServerLegacy: notifyAll should be inside synchronized block, and a null checkAndMutate expected value should be handled correctly".

        Thanks Mikhail !

        REVISION DETAIL
        https://reviews.facebook.net/D5841

        BRANCH
        fix_locked_rows_v2

        To: Kannan, Liyin, Karthik, JIRA, mbautin

        Show
        Phabricator added a comment - Liyin has accepted the revision " [jira] HBASE-6930 [89-fb] Fix TestThriftServerLegacy: notifyAll should be inside synchronized block, and a null checkAndMutate expected value should be handled correctly". Thanks Mikhail ! REVISION DETAIL https://reviews.facebook.net/D5841 BRANCH fix_locked_rows_v2 To: Kannan, Liyin, Karthik, JIRA, mbautin
        Hide
        Phabricator added a comment -

        mbautin has closed the revision "[jira] HBASE-6930 [89-fb] Fix TestThriftServerLegacy: notifyAll should be inside synchronized block, and a null checkAndMutate expected value should be handled correctly".

        CHANGED PRIOR TO COMMIT
        https://reviews.facebook.net/D5841?vs=19251&id=19293#differential-review-toc

        REVISION DETAIL
        https://reviews.facebook.net/D5841

        COMMIT
        https://reviews.facebook.net/rHBASEEIGHTNINEFBBRANCH1393839

        To: Kannan, Liyin, Karthik, JIRA, mbautin

        Show
        Phabricator added a comment - mbautin has closed the revision " [jira] HBASE-6930 [89-fb] Fix TestThriftServerLegacy: notifyAll should be inside synchronized block, and a null checkAndMutate expected value should be handled correctly". CHANGED PRIOR TO COMMIT https://reviews.facebook.net/D5841?vs=19251&id=19293#differential-review-toc REVISION DETAIL https://reviews.facebook.net/D5841 COMMIT https://reviews.facebook.net/rHBASEEIGHTNINEFBBRANCH1393839 To: Kannan, Liyin, Karthik, JIRA, mbautin
        Hide
        Liang Xie added a comment -

        Liyin Tang, seems we need to open an issue for trunk as well, right? I compared "Revision: 1393468" in fb-89 branch and current trunk code in roughly, it looks trunk code need to have the similar optimization as well.

        Show
        Liang Xie added a comment - Liyin Tang , seems we need to open an issue for trunk as well, right? I compared "Revision: 1393468" in fb-89 branch and current trunk code in roughly, it looks trunk code need to have the similar optimization as well.
        Hide
        Lars Hofhansl added a comment -

        The title/description here and the patch (and review) do not seem to mingle...

        Show
        Lars Hofhansl added a comment - The title/description here and the patch (and review) do not seem to mingle...
        Hide
        Dave Latham added a comment -

        Definitely confused here, along with Lars. Did the change describe (avoid acquiring the same row lock) happen somewhere else? Discussing something similar in HBASE-8806.

        Show
        Dave Latham added a comment - Definitely confused here, along with Lars. Did the change describe (avoid acquiring the same row lock) happen somewhere else? Discussing something similar in HBASE-8806 .
        Liyin Tang made changes -
        Attachment D5841.1.patch [ 12547610 ]
        Liyin Tang made changes -
        Attachment D5841.2.patch [ 12547611 ]
        Hide
        Liyin Tang added a comment -

        The patch here seems to be attached to a wrong jira. Sorry about the confusion.
        I have re-attached the path here.

        Show
        Liyin Tang added a comment - The patch here seems to be attached to a wrong jira. Sorry about the confusion. I have re-attached the path here.
        Liyin Tang made changes -
        Attachment HBASE-6930.diff [ 12589942 ]

          People

          • Assignee:
            Unassigned
            Reporter:
            Liyin Tang
          • Votes:
            0 Vote for this issue
            Watchers:
            5 Start watching this issue

            Dates

            • Created:
              Updated:

              Development