HBase
  1. HBase
  2. HBASE-9949

Fix the race condition between Compaction and StoreScanner.init

    Details

    • Type: Bug Bug
    • Status: Resolved
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 0.89-fb
    • Fix Version/s: 0.89-fb, 0.98.0
    • Component/s: Scanners
    • Labels:
      None
    • Hadoop Flags:
      Reviewed

      Description

      The StoreScanner constructor has multiple stages and there can be a race betwwen an ongoing compaction and the StoreScanner constructor where we might get the list of scanners before a compaction and seek on those scanners after the compaction.

      1. 9949.addendum
        16 kB
        Ted Yu
      2. 9949-0.96.addendum
        0.7 kB
        Ted Yu
      3. 9949-trunk-v3.txt
        18 kB
        Ted Yu
      4. 9949-trunk-v2.txt
        18 kB
        Ted Yu
      5. 9949-trunk-v1.txt
        16 kB
        Ted Yu

        Activity

        Hide
        Ted Yu added a comment -

        Patch for trunk, based on patch for 89-fb branch.

        Show
        Ted Yu added a comment - Patch for trunk, based on patch for 89-fb branch.
        Hide
        Ted Yu added a comment -

        TestStoreScanner#testCompactionRaceCondition passes locally.

        Show
        Ted Yu added a comment - TestStoreScanner#testCompactionRaceCondition passes locally.
        Hide
        Sergey Shelukhin added a comment -

        can you create an RB? Many on the injectionhandler events appear to be unused. Overall, I wonder if there could be better approach... I guess it's better than some things we have, like not completing compaction on config.

        Show
        Sergey Shelukhin added a comment - can you create an RB? Many on the injectionhandler events appear to be unused. Overall, I wonder if there could be better approach... I guess it's better than some things we have, like not completing compaction on config.
        Hide
        Ted Yu added a comment -

        I put the patch on https://reviews.apache.org/r/15465/

        The unused events would be dropped in next patch.

        Show
        Ted Yu added a comment - I put the patch on https://reviews.apache.org/r/15465/ The unused events would be dropped in next patch.
        Hide
        Manukranth Kolloju added a comment -

        Ted Yu We use the other event InjectionEvents in testing parts of master etc. So, they might not apply to trunk.

        Show
        Manukranth Kolloju added a comment - Ted Yu We use the other event InjectionEvents in testing parts of master etc. So, they might not apply to trunk.
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12613482/9949-trunk-v1.txt
        against trunk revision .

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

        +1 tests included. The patch appears to include 3 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 1 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 patch appears to cause mvn site goal to fail.

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

        Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/7832//testReport/
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/7832//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-prefix-tree.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/7832//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-client.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/7832//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-common.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/7832//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-protocol.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/7832//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-server.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/7832//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop1-compat.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/7832//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-examples.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/7832//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-thrift.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/7832//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop-compat.html
        Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/7832//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/12613482/9949-trunk-v1.txt against trunk revision . +1 @author . The patch does not contain any @author tags. +1 tests included . The patch appears to include 3 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 1 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 patch appears to cause mvn site goal to fail. +1 core tests . The patch passed unit tests in . Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/7832//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/7832//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-prefix-tree.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/7832//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-client.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/7832//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-common.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/7832//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-protocol.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/7832//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-server.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/7832//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop1-compat.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/7832//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-examples.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/7832//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-thrift.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/7832//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop-compat.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/7832//console This message is automatically generated.
        Hide
        Sergey Shelukhin added a comment -

        some code feedback on RB. It would be nice if one more person would look at general approach

        Show
        Sergey Shelukhin added a comment - some code feedback on RB. It would be nice if one more person would look at general approach
        Hide
        Ted Yu added a comment -

        Patch v2 addresses review comments by introducing enum: StoreScannerCompactionRace

        Also added more comments for the new test.

        Show
        Ted Yu added a comment - Patch v2 addresses review comments by introducing enum: StoreScannerCompactionRace Also added more comments for the new test.
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12613721/9949-trunk-v2.txt
        against trunk revision .

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

        +1 tests included. The patch appears to include 3 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 patch appears to cause mvn site goal to fail.

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

        Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/7852//testReport/
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/7852//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-protocol.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/7852//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-thrift.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/7852//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-client.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/7852//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-examples.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/7852//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop1-compat.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/7852//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-prefix-tree.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/7852//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-common.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/7852//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-server.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/7852//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop-compat.html
        Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/7852//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/12613721/9949-trunk-v2.txt against trunk revision . +1 @author . The patch does not contain any @author tags. +1 tests included . The patch appears to include 3 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 patch appears to cause mvn site goal to fail. -1 core tests . The patch failed these unit tests: org.apache.hadoop.hbase.coprocessor.TestRegionServerCoprocessorExceptionWithAbort Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/7852//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/7852//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-protocol.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/7852//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-thrift.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/7852//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-client.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/7852//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-examples.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/7852//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop1-compat.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/7852//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-prefix-tree.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/7852//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-common.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/7852//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-server.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/7852//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop-compat.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/7852//console This message is automatically generated.
        Hide
        Sergey Shelukhin added a comment -

        minor nit on rb, can be fixed on commit; I'm +0.9, noticed that Jonathan Hsieh was also leery of the approach on RB.
        I think it may warrant more discussion - how we do this kind of stuff (since presumably this injection will start being used all over the place if committed), but if there's general agreement then I'm +1

        Show
        Sergey Shelukhin added a comment - minor nit on rb, can be fixed on commit; I'm +0.9, noticed that Jonathan Hsieh was also leery of the approach on RB. I think it may warrant more discussion - how we do this kind of stuff (since presumably this injection will start being used all over the place if committed), but if there's general agreement then I'm +1
        Hide
        Ted Yu added a comment -

        By default InjectionHandler#processEvent() is no-op. There is no impact on performance.
        The test uses specialized InjectionHandler:

        +    StoreScannerCompactionRaceCondition ih =
        +        new StoreScannerCompactionRaceCondition(s, 5);
        +    InjectionHandler.set(ih);
        
        Show
        Ted Yu added a comment - By default InjectionHandler#processEvent() is no-op. There is no impact on performance. The test uses specialized InjectionHandler: + StoreScannerCompactionRaceCondition ih = + new StoreScannerCompactionRaceCondition(s, 5); + InjectionHandler.set(ih);
        Hide
        Manukranth Kolloju added a comment -

        Sergey Shelukhin, in prod, this operation should have absolutely low impact. The only concern if any should be about the readability of the code going forward.

        Show
        Manukranth Kolloju added a comment - Sergey Shelukhin , in prod, this operation should have absolutely low impact. The only concern if any should be about the readability of the code going forward.
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12614001/9949-trunk-v3.txt
        against trunk revision .

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

        +1 tests included. The patch appears to include 3 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 patch appears to cause mvn site goal to fail.

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

        Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/7873//testReport/
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/7873//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-protocol.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/7873//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-thrift.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/7873//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-client.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/7873//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-examples.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/7873//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop1-compat.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/7873//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-prefix-tree.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/7873//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-common.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/7873//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-server.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/7873//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop-compat.html
        Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/7873//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/12614001/9949-trunk-v3.txt against trunk revision . +1 @author . The patch does not contain any @author tags. +1 tests included . The patch appears to include 3 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 patch appears to cause mvn site goal to fail. +1 core tests . The patch passed unit tests in . Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/7873//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/7873//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-protocol.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/7873//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-thrift.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/7873//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-client.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/7873//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-examples.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/7873//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop1-compat.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/7873//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-prefix-tree.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/7873//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-common.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/7873//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-server.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/7873//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop-compat.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/7873//console This message is automatically generated.
        Hide
        Ted Yu added a comment -

        Planning to integrate over the weekend, if there is no further comment.

        Show
        Ted Yu added a comment - Planning to integrate over the weekend, if there is no further comment.
        Hide
        Ted Yu added a comment -

        Integrated to 0.96 and trunk.

        Thanks for the reviews, Jon and Sergey.

        Show
        Ted Yu added a comment - Integrated to 0.96 and trunk. Thanks for the reviews, Jon and Sergey.
        Hide
        Hudson added a comment -

        FAILURE: Integrated in hbase-0.96 #190 (See https://builds.apache.org/job/hbase-0.96/190/)
        HBASE-9949 Fix the race condition between Compaction and StoreScanner.init (tedyu: rev 1542518)

        • /hbase/branches/0.96/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HStore.java
        • /hbase/branches/0.96/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/StoreScanner.java
        • /hbase/branches/0.96/hbase-server/src/main/java/org/apache/hadoop/hbase/util/InjectionEvent.java
        • /hbase/branches/0.96/hbase-server/src/main/java/org/apache/hadoop/hbase/util/InjectionHandler.java
        • /hbase/branches/0.96/hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestStoreScanner.java
        Show
        Hudson added a comment - FAILURE: Integrated in hbase-0.96 #190 (See https://builds.apache.org/job/hbase-0.96/190/ ) HBASE-9949 Fix the race condition between Compaction and StoreScanner.init (tedyu: rev 1542518) /hbase/branches/0.96/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HStore.java /hbase/branches/0.96/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/StoreScanner.java /hbase/branches/0.96/hbase-server/src/main/java/org/apache/hadoop/hbase/util/InjectionEvent.java /hbase/branches/0.96/hbase-server/src/main/java/org/apache/hadoop/hbase/util/InjectionHandler.java /hbase/branches/0.96/hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestStoreScanner.java
        Hide
        Hudson added a comment -

        FAILURE: Integrated in hbase-0.96-hadoop2 #120 (See https://builds.apache.org/job/hbase-0.96-hadoop2/120/)
        HBASE-9949 Addendum removes readpoint parameter for getScanner() (tedyu: rev 1542528)

        • /hbase/branches/0.96/hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestStoreScanner.java
          HBASE-9949 Fix the race condition between Compaction and StoreScanner.init (tedyu: rev 1542518)
        • /hbase/branches/0.96/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HStore.java
        • /hbase/branches/0.96/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/StoreScanner.java
        • /hbase/branches/0.96/hbase-server/src/main/java/org/apache/hadoop/hbase/util/InjectionEvent.java
        • /hbase/branches/0.96/hbase-server/src/main/java/org/apache/hadoop/hbase/util/InjectionHandler.java
        • /hbase/branches/0.96/hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestStoreScanner.java
        Show
        Hudson added a comment - FAILURE: Integrated in hbase-0.96-hadoop2 #120 (See https://builds.apache.org/job/hbase-0.96-hadoop2/120/ ) HBASE-9949 Addendum removes readpoint parameter for getScanner() (tedyu: rev 1542528) /hbase/branches/0.96/hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestStoreScanner.java HBASE-9949 Fix the race condition between Compaction and StoreScanner.init (tedyu: rev 1542518) /hbase/branches/0.96/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HStore.java /hbase/branches/0.96/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/StoreScanner.java /hbase/branches/0.96/hbase-server/src/main/java/org/apache/hadoop/hbase/util/InjectionEvent.java /hbase/branches/0.96/hbase-server/src/main/java/org/apache/hadoop/hbase/util/InjectionHandler.java /hbase/branches/0.96/hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestStoreScanner.java
        Hide
        Hudson added a comment -

        SUCCESS: Integrated in HBase-TRUNK-on-Hadoop-2.0.0 #839 (See https://builds.apache.org/job/HBase-TRUNK-on-Hadoop-2.0.0/839/)
        HBASE-9949 Fix the race condition between Compaction and StoreScanner.init (tedyu: rev 1542519)

        • /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HStore.java
        • /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/StoreScanner.java
        • /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/util/InjectionEvent.java
        • /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/util/InjectionHandler.java
        • /hbase/trunk/hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestStoreScanner.java
        Show
        Hudson added a comment - SUCCESS: Integrated in HBase-TRUNK-on-Hadoop-2.0.0 #839 (See https://builds.apache.org/job/HBase-TRUNK-on-Hadoop-2.0.0/839/ ) HBASE-9949 Fix the race condition between Compaction and StoreScanner.init (tedyu: rev 1542519) /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HStore.java /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/StoreScanner.java /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/util/InjectionEvent.java /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/util/InjectionHandler.java /hbase/trunk/hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestStoreScanner.java
        Hide
        Hudson added a comment -

        SUCCESS: Integrated in hbase-0.96 #191 (See https://builds.apache.org/job/hbase-0.96/191/)
        HBASE-9949 Revert due to TestHRegion failure (tedyu: rev 1542569)

        • /hbase/branches/0.96/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HStore.java
        • /hbase/branches/0.96/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/StoreScanner.java
        • /hbase/branches/0.96/hbase-server/src/main/java/org/apache/hadoop/hbase/util/InjectionEvent.java
        • /hbase/branches/0.96/hbase-server/src/main/java/org/apache/hadoop/hbase/util/InjectionHandler.java
        • /hbase/branches/0.96/hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestStoreScanner.java
          HBASE-9949 Addendum removes readpoint parameter for getScanner() (tedyu: rev 1542528)
        • /hbase/branches/0.96/hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestStoreScanner.java
        Show
        Hudson added a comment - SUCCESS: Integrated in hbase-0.96 #191 (See https://builds.apache.org/job/hbase-0.96/191/ ) HBASE-9949 Revert due to TestHRegion failure (tedyu: rev 1542569) /hbase/branches/0.96/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HStore.java /hbase/branches/0.96/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/StoreScanner.java /hbase/branches/0.96/hbase-server/src/main/java/org/apache/hadoop/hbase/util/InjectionEvent.java /hbase/branches/0.96/hbase-server/src/main/java/org/apache/hadoop/hbase/util/InjectionHandler.java /hbase/branches/0.96/hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestStoreScanner.java HBASE-9949 Addendum removes readpoint parameter for getScanner() (tedyu: rev 1542528) /hbase/branches/0.96/hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestStoreScanner.java
        Hide
        Hudson added a comment -

        SUCCESS: Integrated in HBase-TRUNK #4682 (See https://builds.apache.org/job/HBase-TRUNK/4682/)
        HBASE-9949 Fix the race condition between Compaction and StoreScanner.init (tedyu: rev 1542519)

        • /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HStore.java
        • /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/StoreScanner.java
        • /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/util/InjectionEvent.java
        • /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/util/InjectionHandler.java
        • /hbase/trunk/hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestStoreScanner.java
        Show
        Hudson added a comment - SUCCESS: Integrated in HBase-TRUNK #4682 (See https://builds.apache.org/job/HBase-TRUNK/4682/ ) HBASE-9949 Fix the race condition between Compaction and StoreScanner.init (tedyu: rev 1542519) /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HStore.java /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/StoreScanner.java /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/util/InjectionEvent.java /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/util/InjectionHandler.java /hbase/trunk/hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestStoreScanner.java
        Hide
        Hudson added a comment -

        FAILURE: Integrated in hbase-0.96-hadoop2 #121 (See https://builds.apache.org/job/hbase-0.96-hadoop2/121/)
        HBASE-9949 Revert due to TestHRegion failure (tedyu: rev 1542569)

        • /hbase/branches/0.96/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HStore.java
        • /hbase/branches/0.96/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/StoreScanner.java
        • /hbase/branches/0.96/hbase-server/src/main/java/org/apache/hadoop/hbase/util/InjectionEvent.java
        • /hbase/branches/0.96/hbase-server/src/main/java/org/apache/hadoop/hbase/util/InjectionHandler.java
        • /hbase/branches/0.96/hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestStoreScanner.java
        Show
        Hudson added a comment - FAILURE: Integrated in hbase-0.96-hadoop2 #121 (See https://builds.apache.org/job/hbase-0.96-hadoop2/121/ ) HBASE-9949 Revert due to TestHRegion failure (tedyu: rev 1542569) /hbase/branches/0.96/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HStore.java /hbase/branches/0.96/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/StoreScanner.java /hbase/branches/0.96/hbase-server/src/main/java/org/apache/hadoop/hbase/util/InjectionEvent.java /hbase/branches/0.96/hbase-server/src/main/java/org/apache/hadoop/hbase/util/InjectionHandler.java /hbase/branches/0.96/hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestStoreScanner.java
        Hide
        Jonathan Hsieh added a comment -

        Hey Ted Yu commented on Friday or Thursday that I'm concerned about this infrastructure creeping in throughout the code. Specifically the in the main comment i mentioned that "This was not addressed" and then you committed without addressing the concern I had with the code in the review and there were no +1's in review board. (Though sergey had a conditional +1 in jira).

        I'm assuming this was a an oversight.

        To be clear, I'm basically fine with the fix – I'm mostly concerned about the new framework.

        It seems like yet another infrastructure and it is one that I'm not particularly fond of because of it seems cumbersome and has the potential to perf impact in other areas if extended. This will take more work but it can be done in a way that makes the code more readable and maintainable and I'd rather we move in that direction instead of adding yet more one of infrastructures. Can we instead make use a factories patterns + mocks to do this injection? Happy to move this discussion to the mailing list.

        Show
        Jonathan Hsieh added a comment - Hey Ted Yu commented on Friday or Thursday that I'm concerned about this infrastructure creeping in throughout the code. Specifically the in the main comment i mentioned that "This was not addressed" and then you committed without addressing the concern I had with the code in the review and there were no +1's in review board. (Though sergey had a conditional +1 in jira). I'm assuming this was a an oversight. To be clear, I'm basically fine with the fix – I'm mostly concerned about the new framework. It seems like yet another infrastructure and it is one that I'm not particularly fond of because of it seems cumbersome and has the potential to perf impact in other areas if extended. This will take more work but it can be done in a way that makes the code more readable and maintainable and I'd rather we move in that direction instead of adding yet more one of infrastructures. Can we instead make use a factories patterns + mocks to do this injection? Happy to move this discussion to the mailing list.
        Hide
        Ted Yu added a comment -

        How about I take out the new infrastructure through an addendum and keep the fix (since you're fine with it).

        That would give us more time in streamlining proper test infrastructure for this ?

        Show
        Ted Yu added a comment - How about I take out the new infrastructure through an addendum and keep the fix (since you're fine with it). That would give us more time in streamlining proper test infrastructure for this ?
        Hide
        Jonathan Hsieh added a comment -

        Sounds great. Thanks! Want to start the thread on dev list?

        Show
        Jonathan Hsieh added a comment - Sounds great. Thanks! Want to start the thread on dev list?
        Hide
        Ted Yu added a comment -

        Checked in addendum to trunk.

        Show
        Ted Yu added a comment - Checked in addendum to trunk.
        Hide
        Hudson added a comment -

        SUCCESS: Integrated in HBase-TRUNK #4687 (See https://builds.apache.org/job/HBase-TRUNK/4687/)
        HBASE-9949 Addendum takes out test infrastructure and new test (tedyu: rev 1543305)

        • /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HStore.java
        • /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/StoreScanner.java
        • /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/util/InjectionEvent.java
        • /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/util/InjectionHandler.java
        • /hbase/trunk/hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestStoreScanner.java
        Show
        Hudson added a comment - SUCCESS: Integrated in HBase-TRUNK #4687 (See https://builds.apache.org/job/HBase-TRUNK/4687/ ) HBASE-9949 Addendum takes out test infrastructure and new test (tedyu: rev 1543305) /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HStore.java /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/StoreScanner.java /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/util/InjectionEvent.java /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/util/InjectionHandler.java /hbase/trunk/hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestStoreScanner.java
        Hide
        Ted Yu added a comment -

        @Jon:
        I was thinking about the following approach for testing :
        1. Introduce config parameter for StoreScanner implementation which would be used by HStore for creating scanner
        2. In TestStoreScanner, add StoreScanner implementation which extends StoreScanner and set the above config parameter to this class.
        3. Register custom ChangedReadersObserver through the following API of HStore :

          public void addChangedReaderObserver(ChangedReadersObserver o) {
        

        The BEFORE_SEEK hook would be activated before Store.getScanner() is called.
        The AFTER_SEEK hook can be activated at the end of ctor of StoreScanner wrapper created in #2
        The custom ChangedReadersObserver would activate the COMPACT_COMPLETE hook.

        What do you think ?

        Show
        Ted Yu added a comment - @Jon: I was thinking about the following approach for testing : 1. Introduce config parameter for StoreScanner implementation which would be used by HStore for creating scanner 2. In TestStoreScanner, add StoreScanner implementation which extends StoreScanner and set the above config parameter to this class. 3. Register custom ChangedReadersObserver through the following API of HStore : public void addChangedReaderObserver(ChangedReadersObserver o) { The BEFORE_SEEK hook would be activated before Store.getScanner() is called. The AFTER_SEEK hook can be activated at the end of ctor of StoreScanner wrapper created in #2 The custom ChangedReadersObserver would activate the COMPACT_COMPLETE hook. What do you think ?
        Hide
        Hudson added a comment -

        SUCCESS: Integrated in HBase-TRUNK-on-Hadoop-2.0.0 #844 (See https://builds.apache.org/job/HBase-TRUNK-on-Hadoop-2.0.0/844/)
        HBASE-9949 Addendum takes out test infrastructure and new test (tedyu: rev 1543305)

        • /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HStore.java
        • /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/StoreScanner.java
        • /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/util/InjectionEvent.java
        • /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/util/InjectionHandler.java
        • /hbase/trunk/hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestStoreScanner.java
        Show
        Hudson added a comment - SUCCESS: Integrated in HBase-TRUNK-on-Hadoop-2.0.0 #844 (See https://builds.apache.org/job/HBase-TRUNK-on-Hadoop-2.0.0/844/ ) HBASE-9949 Addendum takes out test infrastructure and new test (tedyu: rev 1543305) /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HStore.java /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/StoreScanner.java /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/util/InjectionEvent.java /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/util/InjectionHandler.java /hbase/trunk/hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestStoreScanner.java
        Hide
        Andrew Purtell added a comment -

        ​Shall we revert the HBASE-9949 commit to trunk until we have consensus on the testing part? Seems both Jon and I, and probably Stack also given his comment on dev@, have an issue with the testing bits that were committed. (Sorry I didn't come around to this issue before it went it.)​

        Show
        Andrew Purtell added a comment - ​Shall we revert the HBASE-9949 commit to trunk until we have consensus on the testing part? Seems both Jon and I, and probably Stack also given his comment on dev@, have an issue with the testing bits that were committed. (Sorry I didn't come around to this issue before it went it.)​
        Hide
        Ted Yu added a comment -

        The testing bits were taken out through 9949.addendum

        Show
        Ted Yu added a comment - The testing bits were taken out through 9949.addendum
        Hide
        Andrew Purtell added a comment -

        The testing bits were taken out through 9949.addendum

        Great, thanks Ted.

        Show
        Andrew Purtell added a comment - The testing bits were taken out through 9949.addendum Great, thanks Ted.
        Hide
        Ted Yu added a comment -

        Test would be added in separate JIRA once approach to testing is agreed upon.

        Show
        Ted Yu added a comment - Test would be added in separate JIRA once approach to testing is agreed upon.
        Hide
        Lars Hofhansl added a comment -

        Adding 0.96 bit, as this as also committed to 0.96. (not sure whether this is in 0.96.1 or 0.96.2, though).
        Now, this seems important to fix in 0.94 as well - if I read this right we can lose data...?

        The updateReaders bit is a bit scary. I am in this code a bit these days. Another option would be to block reader updates until after he compaction/flush is finished, that would be relatively easy to do by just locking the StoreScanner in question for the entire duration of the compaction (updateReaders would then block until the StoreScanner is released).

        Show
        Lars Hofhansl added a comment - Adding 0.96 bit, as this as also committed to 0.96. (not sure whether this is in 0.96.1 or 0.96.2, though). Now, this seems important to fix in 0.94 as well - if I read this right we can lose data...? The updateReaders bit is a bit scary. I am in this code a bit these days. Another option would be to block reader updates until after he compaction/flush is finished, that would be relatively easy to do by just locking the StoreScanner in question for the entire duration of the compaction (updateReaders would then block until the StoreScanner is released).
        Hide
        Ted Yu added a comment -

        The fix was not in 0.96

        Show
        Ted Yu added a comment - The fix was not in 0.96
        Hide
        Lars Hofhansl added a comment -

        Yes, was reverted.

        Show
        Lars Hofhansl added a comment - Yes, was reverted.

          People

          • Assignee:
            Manukranth Kolloju
            Reporter:
            Manukranth Kolloju
          • Votes:
            0 Vote for this issue
            Watchers:
            14 Start watching this issue

            Dates

            • Due:
              Created:
              Updated:
              Resolved:

              Time Tracking

              Estimated:
              Original Estimate - 48h
              48h
              Remaining:
              Remaining Estimate - 48h
              48h
              Logged:
              Time Spent - Not Specified
              Not Specified

                Development