HBase
  1. HBase
  2. HBASE-5922

HalfStoreFileReader seekBefore causes StackOverflowError

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Critical Critical
    • Resolution: Fixed
    • Affects Version/s: 0.90.0
    • Fix Version/s: 0.94.1, 0.95.0
    • Component/s: Client, io
    • Labels:
      None
    • Environment:

      HBase 0.90.4

    • Hadoop Flags:
      Reviewed

      Description

      Calling HRegionServer.getClosestRowBefore() can cause a stack overflow if the underlying store file is a reference and the row key is in the bottom.

      java.io.IOException: java.io.IOException: java.lang.StackOverflowError
      at org.apache.hadoop.hbase.regionserver.HRegionServer.convertThrowableToIOE(HRegionServer.java:990)
      at org.apache.hadoop.hbase.regionserver.HRegionServer.convertThrowableToIOE(HRegionServer.java:978)
      at org.apache.hadoop.hbase.regionserver.HRegionServer.getClosestRowBefore(HRegionServer.java:1651)
      at sun.reflect.GeneratedMethodAccessor174.invoke(Unknown Source)
      at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:25)
      at java.lang.reflect.Method.invoke(Method.java:597)
      at org.apache.hadoop.hbase.ipc.HBaseRPC$Server.call(HBaseRPC.java:570)
      at org.apache.hadoop.hbase.ipc.HBaseServer$Handler.run(HBaseServer.java:1039)
      Caused by: java.lang.StackOverflowError
      at org.apache.hadoop.hbase.io.HalfStoreFileReader$1.seekBefore(HalfStoreFileReader.java:147)
      at org.apache.hadoop.hbase.io.HalfStoreFileReader$1.seekBefore(HalfStoreFileReader.java:149)
      at org.apache.hadoop.hbase.io.HalfStoreFileReader$1.seekBefore(HalfStoreFileReader.java:149)
      at org.apache.hadoop.hbase.io.HalfStoreFileReader$1.seekBefore(HalfStoreFileReader.java:149)
      at org.apache.hadoop.hbase.io.HalfStoreFileReader$1.seekBefore(HalfStoreFileReader.java:149)

      1. 5922.092.txt
        0.8 kB
        stack
      2. HBASE-5922.v4.patch
        5 kB
        Nate Putnam
      3. HBASE-5922.v3.patch
        5 kB
        Nate Putnam
      4. HBASE-5922.v2.patch
        5 kB
        Nate Putnam
      5. HBASE-5922.patch
        4 kB
        stack
      6. HBASE-5922.patch
        4 kB
        Nate Putnam

        Issue Links

          Activity

          Hide
          Hudson added a comment -

          Integrated in HBase-0.94-security-on-Hadoop-23 #8 (See https://builds.apache.org/job/HBase-0.94-security-on-Hadoop-23/8/)
          HBASE-5997 Fix concerns raised in HBASE-5922 related to HalfStoreFileReader (Revision 1383792)

          Result = FAILURE
          stack :
          Files :

          • /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/io/HalfStoreFileReader.java
          • /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/regionserver/Store.java
          • /hbase/branches/0.94/src/test/java/org/apache/hadoop/hbase/io/TestHalfStoreFileReader.java
          Show
          Hudson added a comment - Integrated in HBase-0.94-security-on-Hadoop-23 #8 (See https://builds.apache.org/job/HBase-0.94-security-on-Hadoop-23/8/ ) HBASE-5997 Fix concerns raised in HBASE-5922 related to HalfStoreFileReader (Revision 1383792) Result = FAILURE stack : Files : /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/io/HalfStoreFileReader.java /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/regionserver/Store.java /hbase/branches/0.94/src/test/java/org/apache/hadoop/hbase/io/TestHalfStoreFileReader.java
          Hide
          Hudson added a comment -

          Integrated in HBase-0.94-security #52 (See https://builds.apache.org/job/HBase-0.94-security/52/)
          HBASE-5997 Fix concerns raised in HBASE-5922 related to HalfStoreFileReader (Revision 1383792)

          Result = SUCCESS
          stack :
          Files :

          • /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/io/HalfStoreFileReader.java
          • /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/regionserver/Store.java
          • /hbase/branches/0.94/src/test/java/org/apache/hadoop/hbase/io/TestHalfStoreFileReader.java
          Show
          Hudson added a comment - Integrated in HBase-0.94-security #52 (See https://builds.apache.org/job/HBase-0.94-security/52/ ) HBASE-5997 Fix concerns raised in HBASE-5922 related to HalfStoreFileReader (Revision 1383792) Result = SUCCESS stack : Files : /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/io/HalfStoreFileReader.java /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/regionserver/Store.java /hbase/branches/0.94/src/test/java/org/apache/hadoop/hbase/io/TestHalfStoreFileReader.java
          Hide
          Hudson added a comment -

          Integrated in HBase-TRUNK-on-Hadoop-2.0.0 #169 (See https://builds.apache.org/job/HBase-TRUNK-on-Hadoop-2.0.0/169/)
          HBASE-5997 Fix concerns raised in HBASE-5922 related to HalfStoreFileReader (Revision 1383788)

          Result = FAILURE
          stack :
          Files :

          • /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/io/HalfStoreFileReader.java
          • /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HStore.java
          • /hbase/trunk/hbase-server/src/test/java/org/apache/hadoop/hbase/io/TestHalfStoreFileReader.java
          Show
          Hudson added a comment - Integrated in HBase-TRUNK-on-Hadoop-2.0.0 #169 (See https://builds.apache.org/job/HBase-TRUNK-on-Hadoop-2.0.0/169/ ) HBASE-5997 Fix concerns raised in HBASE-5922 related to HalfStoreFileReader (Revision 1383788) Result = FAILURE stack : Files : /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/io/HalfStoreFileReader.java /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HStore.java /hbase/trunk/hbase-server/src/test/java/org/apache/hadoop/hbase/io/TestHalfStoreFileReader.java
          Hide
          Hudson added a comment -

          Integrated in HBase-0.94 #464 (See https://builds.apache.org/job/HBase-0.94/464/)
          HBASE-5997 Fix concerns raised in HBASE-5922 related to HalfStoreFileReader (Revision 1383792)

          Result = FAILURE
          stack :
          Files :

          • /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/io/HalfStoreFileReader.java
          • /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/regionserver/Store.java
          • /hbase/branches/0.94/src/test/java/org/apache/hadoop/hbase/io/TestHalfStoreFileReader.java
          Show
          Hudson added a comment - Integrated in HBase-0.94 #464 (See https://builds.apache.org/job/HBase-0.94/464/ ) HBASE-5997 Fix concerns raised in HBASE-5922 related to HalfStoreFileReader (Revision 1383792) Result = FAILURE stack : Files : /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/io/HalfStoreFileReader.java /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/regionserver/Store.java /hbase/branches/0.94/src/test/java/org/apache/hadoop/hbase/io/TestHalfStoreFileReader.java
          Hide
          Hudson added a comment -

          Integrated in HBase-TRUNK #3322 (See https://builds.apache.org/job/HBase-TRUNK/3322/)
          HBASE-5997 Fix concerns raised in HBASE-5922 related to HalfStoreFileReader (Revision 1383788)

          Result = FAILURE
          stack :
          Files :

          • /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/io/HalfStoreFileReader.java
          • /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HStore.java
          • /hbase/trunk/hbase-server/src/test/java/org/apache/hadoop/hbase/io/TestHalfStoreFileReader.java
          Show
          Hudson added a comment - Integrated in HBase-TRUNK #3322 (See https://builds.apache.org/job/HBase-TRUNK/3322/ ) HBASE-5997 Fix concerns raised in HBASE-5922 related to HalfStoreFileReader (Revision 1383788) Result = FAILURE stack : Files : /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/io/HalfStoreFileReader.java /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HStore.java /hbase/trunk/hbase-server/src/test/java/org/apache/hadoop/hbase/io/TestHalfStoreFileReader.java
          Hide
          Hudson added a comment -

          Integrated in HBase-TRUNK-on-Hadoop-2.0.0 #150 (See https://builds.apache.org/job/HBase-TRUNK-on-Hadoop-2.0.0/150/)
          HBASE-5997 Fix concerns raised in HBASE-5922 related to HalfStoreFileReader; REVERT (Revision 1377483)

          Result = FAILURE
          stack :
          Files :

          • /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/io/HalfStoreFileReader.java
          • /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HStore.java
          • /hbase/trunk/hbase-server/src/test/java/org/apache/hadoop/hbase/io/TestHalfStoreFileReader.java
          Show
          Hudson added a comment - Integrated in HBase-TRUNK-on-Hadoop-2.0.0 #150 (See https://builds.apache.org/job/HBase-TRUNK-on-Hadoop-2.0.0/150/ ) HBASE-5997 Fix concerns raised in HBASE-5922 related to HalfStoreFileReader; REVERT (Revision 1377483) Result = FAILURE stack : Files : /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/io/HalfStoreFileReader.java /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HStore.java /hbase/trunk/hbase-server/src/test/java/org/apache/hadoop/hbase/io/TestHalfStoreFileReader.java
          Hide
          Hudson added a comment -

          Integrated in HBase-TRUNK #3281 (See https://builds.apache.org/job/HBase-TRUNK/3281/)
          HBASE-5997 Fix concerns raised in HBASE-5922 related to HalfStoreFileReader; REVERT (Revision 1377483)

          Result = FAILURE
          stack :
          Files :

          • /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/io/HalfStoreFileReader.java
          • /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HStore.java
          • /hbase/trunk/hbase-server/src/test/java/org/apache/hadoop/hbase/io/TestHalfStoreFileReader.java
          Show
          Hudson added a comment - Integrated in HBase-TRUNK #3281 (See https://builds.apache.org/job/HBase-TRUNK/3281/ ) HBASE-5997 Fix concerns raised in HBASE-5922 related to HalfStoreFileReader; REVERT (Revision 1377483) Result = FAILURE stack : Files : /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/io/HalfStoreFileReader.java /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HStore.java /hbase/trunk/hbase-server/src/test/java/org/apache/hadoop/hbase/io/TestHalfStoreFileReader.java
          Hide
          Hudson added a comment -

          Integrated in HBase-TRUNK-on-Hadoop-2.0.0 #149 (See https://builds.apache.org/job/HBase-TRUNK-on-Hadoop-2.0.0/149/)
          HBASE-5997 Fix concerns raised in HBASE-5922 related to HalfStoreFileReader (Revision 1377366)

          Result = FAILURE
          stack :
          Files :

          • /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/io/HalfStoreFileReader.java
          • /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HStore.java
          • /hbase/trunk/hbase-server/src/test/java/org/apache/hadoop/hbase/io/TestHalfStoreFileReader.java
          Show
          Hudson added a comment - Integrated in HBase-TRUNK-on-Hadoop-2.0.0 #149 (See https://builds.apache.org/job/HBase-TRUNK-on-Hadoop-2.0.0/149/ ) HBASE-5997 Fix concerns raised in HBASE-5922 related to HalfStoreFileReader (Revision 1377366) Result = FAILURE stack : Files : /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/io/HalfStoreFileReader.java /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HStore.java /hbase/trunk/hbase-server/src/test/java/org/apache/hadoop/hbase/io/TestHalfStoreFileReader.java
          Hide
          Hudson added a comment -

          Integrated in HBase-TRUNK #3277 (See https://builds.apache.org/job/HBase-TRUNK/3277/)
          HBASE-5997 Fix concerns raised in HBASE-5922 related to HalfStoreFileReader (Revision 1377366)

          Result = FAILURE
          stack :
          Files :

          • /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/io/HalfStoreFileReader.java
          • /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HStore.java
          • /hbase/trunk/hbase-server/src/test/java/org/apache/hadoop/hbase/io/TestHalfStoreFileReader.java
          Show
          Hudson added a comment - Integrated in HBase-TRUNK #3277 (See https://builds.apache.org/job/HBase-TRUNK/3277/ ) HBASE-5997 Fix concerns raised in HBASE-5922 related to HalfStoreFileReader (Revision 1377366) Result = FAILURE stack : Files : /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/io/HalfStoreFileReader.java /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HStore.java /hbase/trunk/hbase-server/src/test/java/org/apache/hadoop/hbase/io/TestHalfStoreFileReader.java
          Hide
          Hudson added a comment -

          Integrated in HBase-0.94-security #27 (See https://builds.apache.org/job/HBase-0.94-security/27/)
          HBASE-5922 HalfStoreFileReader seekBefore causes StackOverflowError (Revision 1337411)

          Result = SUCCESS
          stack :
          Files :

          • /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/io/HalfStoreFileReader.java
          • /hbase/branches/0.94/src/test/java/org/apache/hadoop/hbase/io/TestHalfStoreFileReader.java
          Show
          Hudson added a comment - Integrated in HBase-0.94-security #27 (See https://builds.apache.org/job/HBase-0.94-security/27/ ) HBASE-5922 HalfStoreFileReader seekBefore causes StackOverflowError (Revision 1337411) Result = SUCCESS stack : Files : /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/io/HalfStoreFileReader.java /hbase/branches/0.94/src/test/java/org/apache/hadoop/hbase/io/TestHalfStoreFileReader.java
          Hide
          Hudson added a comment -

          Integrated in HBase-0.92-security #107 (See https://builds.apache.org/job/HBase-0.92-security/107/)
          HBASE-5922 HalfStoreFileReader seekBefore causes StackOverflowError (Revision 1337412)

          Result = FAILURE
          stack :
          Files :

          • /hbase/branches/0.92/CHANGES.txt
          • /hbase/branches/0.92/src/main/java/org/apache/hadoop/hbase/io/HalfStoreFileReader.java
          Show
          Hudson added a comment - Integrated in HBase-0.92-security #107 (See https://builds.apache.org/job/HBase-0.92-security/107/ ) HBASE-5922 HalfStoreFileReader seekBefore causes StackOverflowError (Revision 1337412) Result = FAILURE stack : Files : /hbase/branches/0.92/CHANGES.txt /hbase/branches/0.92/src/main/java/org/apache/hadoop/hbase/io/HalfStoreFileReader.java
          Hide
          Hudson added a comment -

          Integrated in HBase-0.92 #404 (See https://builds.apache.org/job/HBase-0.92/404/)
          HBASE-5922 HalfStoreFileReader seekBefore causes StackOverflowError (Revision 1337412)

          Result = FAILURE
          stack :
          Files :

          • /hbase/branches/0.92/CHANGES.txt
          • /hbase/branches/0.92/src/main/java/org/apache/hadoop/hbase/io/HalfStoreFileReader.java
          Show
          Hudson added a comment - Integrated in HBase-0.92 #404 (See https://builds.apache.org/job/HBase-0.92/404/ ) HBASE-5922 HalfStoreFileReader seekBefore causes StackOverflowError (Revision 1337412) Result = FAILURE stack : Files : /hbase/branches/0.92/CHANGES.txt /hbase/branches/0.92/src/main/java/org/apache/hadoop/hbase/io/HalfStoreFileReader.java
          Hide
          Hudson added a comment -

          Integrated in HBase-TRUNK #2866 (See https://builds.apache.org/job/HBase-TRUNK/2866/)
          HBASE-5922 HalfStoreFileReader seekBefore causes StackOverflowError (Revision 1337409)

          Result = FAILURE
          stack :
          Files :

          • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/io/HalfStoreFileReader.java
          • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/io/TestHalfStoreFileReader.java
          Show
          Hudson added a comment - Integrated in HBase-TRUNK #2866 (See https://builds.apache.org/job/HBase-TRUNK/2866/ ) HBASE-5922 HalfStoreFileReader seekBefore causes StackOverflowError (Revision 1337409) Result = FAILURE stack : Files : /hbase/trunk/src/main/java/org/apache/hadoop/hbase/io/HalfStoreFileReader.java /hbase/trunk/src/test/java/org/apache/hadoop/hbase/io/TestHalfStoreFileReader.java
          Hide
          Hudson added a comment -

          Integrated in HBase-0.94 #188 (See https://builds.apache.org/job/HBase-0.94/188/)
          HBASE-5922 HalfStoreFileReader seekBefore causes StackOverflowError (Revision 1337411)

          Result = FAILURE
          stack :
          Files :

          • /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/io/HalfStoreFileReader.java
          • /hbase/branches/0.94/src/test/java/org/apache/hadoop/hbase/io/TestHalfStoreFileReader.java
          Show
          Hudson added a comment - Integrated in HBase-0.94 #188 (See https://builds.apache.org/job/HBase-0.94/188/ ) HBASE-5922 HalfStoreFileReader seekBefore causes StackOverflowError (Revision 1337411) Result = FAILURE stack : Files : /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/io/HalfStoreFileReader.java /hbase/branches/0.94/src/test/java/org/apache/hadoop/hbase/io/TestHalfStoreFileReader.java
          Hide
          stack added a comment -

          Committed to 0.90, 0.92, 0.94 and trunk.

          Thanks for the patch Nate. Thanks Anoop for helping get it in.

          Show
          stack added a comment - Committed to 0.90, 0.92, 0.94 and trunk. Thanks for the patch Nate. Thanks Anoop for helping get it in.
          Hide
          stack added a comment -

          What I applied to 0.92 and to 0.90. Its just the change to HalfStoreFileReader, not the test change. The test change would not apply.

          Show
          stack added a comment - What I applied to 0.92 and to 0.90. Its just the change to HalfStoreFileReader, not the test change. The test change would not apply.
          Hide
          ramkrishna.s.vasudevan added a comment -

          @Stack what is the procedure for doing the back port to all the affected versions?

          As Stack did not reply to this i can tell this.
          Just create similar patches for different branches, like download the svn code and prepare patches and again submit it in this JIRA.
          Generally patches affecting across versions will be individually committed to all versions with the respective patches prepared against the version.
          Hope am clear. Let me know if you have any questions.

          Show
          ramkrishna.s.vasudevan added a comment - @Stack what is the procedure for doing the back port to all the affected versions? As Stack did not reply to this i can tell this. Just create similar patches for different branches, like download the svn code and prepare patches and again submit it in this JIRA. Generally patches affecting across versions will be individually committed to all versions with the respective patches prepared against the version. Hope am clear. Let me know if you have any questions.
          Hide
          Anoop Sam John added a comment -

          +1 for patch V4

          Show
          Anoop Sam John added a comment - +1 for patch 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/12526185/HBASE-5922.v4.patch
          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 hadoop23. The patch compiles against the hadoop 0.23.x 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 core tests. The patch passed unit tests in .

          Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/1819//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/1819//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/1819//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/12526185/HBASE-5922.v4.patch 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 hadoop23. The patch compiles against the hadoop 0.23.x 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 core tests. The patch passed unit tests in . Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/1819//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/1819//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/1819//console This message is automatically generated.
          Hide
          ramkrishna.s.vasudevan added a comment -

          +1 on patch.

          Show
          ramkrishna.s.vasudevan added a comment - +1 on patch.
          Hide
          Nate Putnam added a comment -

          @Anoop agreed, v4 patch with that change uploaded.

          @Stack what is the procedure for doing the back port to all the affected versions?

          Show
          Nate Putnam added a comment - @Anoop agreed, v4 patch with that change uploaded. @Stack what is the procedure for doing the back port to all the affected versions?
          Hide
          Anoop Sam John added a comment -

          +1 on patch V3.. I am fine with another JIRA in handling the issue with the top half file seek...

          One suggestion about the test case is pls remove this assertion from the test case

          +      foundKeyValue = doTestOfSeekBefore(p, fs, top, midKV, cacheConf);
          +      assertEquals(beforeMidKey, foundKeyValue);
          

          This is actually a bug as per our above discussion... Ideally it should be returning NULL. At the time when we fix the issue we can have another test case with proper assert... What do u say?

          Otherwise I am fine with the code change and test cases

          Show
          Anoop Sam John added a comment - +1 on patch V3.. I am fine with another JIRA in handling the issue with the top half file seek... One suggestion about the test case is pls remove this assertion from the test case + foundKeyValue = doTestOfSeekBefore(p, fs, top, midKV, cacheConf); + assertEquals(beforeMidKey, foundKeyValue); This is actually a bug as per our above discussion... Ideally it should be returning NULL. At the time when we fix the issue we can have another test case with proper assert... What do u say? Otherwise I am fine with the code change and test cases
          Hide
          stack added a comment -

          I'm good w/ committing v3 and opening a new issue to address other concerns. Anoop?

          Show
          stack added a comment - I'm good w/ committing v3 and opening a new issue to address other concerns. Anoop?
          Hide
          Nate Putnam added a comment -

          @Anoop

          Agreed that the <= is probably the right thing to do. I dug into that piece of code and think there is some issue with the underlying KeyValue.comparator that will keep that from working. It seems that it's nearly impossible for key to exactly equal splitkey.

          This particular issue is a production blocker for us so I would like to get the current fix into the code base and file another issue for the splitkey equality work. Sound good?

          Show
          Nate Putnam added a comment - @Anoop Agreed that the <= is probably the right thing to do. I dug into that piece of code and think there is some issue with the underlying KeyValue.comparator that will keep that from working. It seems that it's nearly impossible for key to exactly equal splitkey. This particular issue is a production blocker for us so I would like to get the current fix into the code base and file another issue for the splitkey equality work. Sound good?
          Hide
          Anoop Sam John added a comment -

          @Nate
          Thanks for the patch

          Just one more thing which is wrong with the seekBefore() code

           if (top) {
                    if (getComparator().compare(key, offset, length, splitkey, 0,
                        splitkey.length) < 0) {
                      return false;
                    }
                  }
          

          This is the code for the special check with top file..When the passed key is < splitKey return false as there wont be any key less than the midkey in this file. Just because of this factor, we need to return false and should not do any seek when the passed key is equal to splitKey, in case of top scanner...

          I know this is out of interest of this bug as such.. But would be nice to fix as part of this. Or need another JIRA?

          One check in your test case also reveals this issue..

          +      foundKeyValue = doTestOfSeekBefore(p, fs, top, midKV, cacheConf);
          +      assertEquals(beforeMidKey, foundKeyValue);
          

          In this try to seekBefore midKV in the top file and it returns beforeMidKey, which is there in the bottom file !

          2. On the top half file a seekBefore() call with a key = splitkey is supposed to return false but it wont happen. It will try to seek into the bottom half I fear ..

          Yes the same thing is happening...

          Good on you Nate and Johnson... The test case also try to cover all boundary cases
          One minor comment about the test case : Can we assert the return value of this seekBefore() especially when the return value is false?

          Show
          Anoop Sam John added a comment - @Nate Thanks for the patch Just one more thing which is wrong with the seekBefore() code if (top) { if (getComparator().compare(key, offset, length, splitkey, 0, splitkey.length) < 0) { return false ; } } This is the code for the special check with top file..When the passed key is < splitKey return false as there wont be any key less than the midkey in this file. Just because of this factor, we need to return false and should not do any seek when the passed key is equal to splitKey, in case of top scanner... I know this is out of interest of this bug as such.. But would be nice to fix as part of this. Or need another JIRA? One check in your test case also reveals this issue.. + foundKeyValue = doTestOfSeekBefore(p, fs, top, midKV, cacheConf); + assertEquals(beforeMidKey, foundKeyValue); In this try to seekBefore midKV in the top file and it returns beforeMidKey, which is there in the bottom file ! 2. On the top half file a seekBefore() call with a key = splitkey is supposed to return false but it wont happen. It will try to seek into the bottom half I fear .. Yes the same thing is happening... Good on you Nate and Johnson... The test case also try to cover all boundary cases One minor comment about the test case : Can we assert the return value of this seekBefore() especially when the return value is false?
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12525680/HBASE-5922.v3.patch
          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 hadoop23. The patch compiles against the hadoop 0.23.x 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 core tests. The patch failed these unit tests:

          Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/1774//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/1774//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/1774//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/12525680/HBASE-5922.v3.patch 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 hadoop23. The patch compiles against the hadoop 0.23.x 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 core tests. The patch failed these unit tests: Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/1774//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/1774//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/1774//console This message is automatically generated.
          Hide
          Todd Johnson added a comment -

          @Anoop, Thanks for the help understanding what was going on here.

          Show
          Todd Johnson added a comment - @Anoop, Thanks for the help understanding what was going on here.
          Hide
          Nate Putnam added a comment -

          @Anoop happy to back port the fix to 92.x,94.x and 90.x once we agree on the solution. I've submitted another patch (v3) witch changes the return false to a delegate.seekBefore.

          I'm fairly confident that this is the final solution. If you have any concerns/comments let me know.

          Show
          Nate Putnam added a comment - @Anoop happy to back port the fix to 92.x,94.x and 90.x once we agree on the solution. I've submitted another patch (v3) witch changes the return false to a delegate.seekBefore. I'm fairly confident that this is the final solution. If you have any concerns/comments let me know.
          Hide
          Anoop Sam John added a comment -

          Also this fix is needed in all versions...
          92.2, 94.1 and trunk

          Show
          Anoop Sam John added a comment - Also this fix is needed in all versions... 92.2, 94.1 and trunk
          Hide
          Anoop Sam John added a comment -
          most readable would change the recursive call on line 153 from seekBefore to delegate.seekBefore

          +1 for this.. Yes recursion always confusing..
          So when key > splitKey -> delegate.seekBefore(splitKey)

          No harm in having key >= splitKey also...

          Show
          Anoop Sam John added a comment - most readable would change the recursive call on line 153 from seekBefore to delegate.seekBefore +1 for this.. Yes recursion always confusing.. So when key > splitKey -> delegate.seekBefore(splitKey) No harm in having key >= splitKey also...
          Hide
          ramkrishna.s.vasudevan added a comment -

          I have been following this JIRA and also discussed with Anoop.

          The passed key is >=splitkey and in the file we need to seek to a position which comes before this key

          I agree with Anoop in this case. Anyway the splitkey is actually part of the top and not the bottom. The '>=' is what is causing the StackOverflow.

          Show
          ramkrishna.s.vasudevan added a comment - I have been following this JIRA and also discussed with Anoop. The passed key is >=splitkey and in the file we need to seek to a position which comes before this key I agree with Anoop in this case. Anyway the splitkey is actually part of the top and not the bottom. The '>=' is what is causing the StackOverflow.
          Hide
          Todd Johnson added a comment -

          "This seems correct to me... that we should be returning last key in the bottom (the 'before') rather than a null."

          I guess this requires a little more testing. My understanding has been that, when 'bottom' is being asked about something that is really in 'top,' there is nothing present (in 'bottom') which could qualify as 'before,' so to move the pointer anywhere would be a waste.

          If the correct thing to do is end up with a call to delegate.seekBefore(splitKey ...), I don't think the best fix is to change >= to >. I agree that this would have that result, but it creates a single unnecessary (and somewhat confusing) recursive call that is guaranteed to result in delegate.seekBefore(splitKey ...) anyway. So the solution which has the behavior that Adoop expects but is most readable would change the recursive call on line 153 from seekBefore to delegate.seekBefore. This would also make it look more like the analogous code in seekTo().

          But I would still prefer to spend a few more minutes with Nate and the code tomorrow.

          Show
          Todd Johnson added a comment - "This seems correct to me... that we should be returning last key in the bottom (the 'before') rather than a null." I guess this requires a little more testing. My understanding has been that, when 'bottom' is being asked about something that is really in 'top,' there is nothing present (in 'bottom') which could qualify as 'before,' so to move the pointer anywhere would be a waste. If the correct thing to do is end up with a call to delegate.seekBefore(splitKey ...), I don't think the best fix is to change >= to >. I agree that this would have that result, but it creates a single unnecessary (and somewhat confusing) recursive call that is guaranteed to result in delegate.seekBefore(splitKey ...) anyway. So the solution which has the behavior that Adoop expects but is most readable would change the recursive call on line 153 from seekBefore to delegate.seekBefore. This would also make it look more like the analogous code in seekTo(). But I would still prefer to spend a few more minutes with Nate and the code tomorrow.
          Hide
          stack added a comment -

          What about you Stack?

          I'm staying out of you fellas' way. You lot seem to be figuring it out just fine. Let me know if you want me to write a test.

          If you test seekTo() on the bottom file passing the splitKey as the key, it will give the KV as the last KV in the file.. Not null... So we need to be consistent right

          This seems correct to me... that we should be returning last key in the bottom (the 'before') rather than a null.

          Going by Todd's comment above, the test to write should be at the StoreFile level since it is the user of HalfFile. I could do a test that had a StoreFile on a top half and another on a bottom half. Then I'd verify the seekBefore around the extremes (it should be doing the before as Anoop suggests and not returning the passed key, if it exists).

          This is a bad bug that you fellas have found.

          Show
          stack added a comment - What about you Stack? I'm staying out of you fellas' way. You lot seem to be figuring it out just fine. Let me know if you want me to write a test. If you test seekTo() on the bottom file passing the splitKey as the key, it will give the KV as the last KV in the file.. Not null... So we need to be consistent right This seems correct to me... that we should be returning last key in the bottom (the 'before') rather than a null. Going by Todd's comment above, the test to write should be at the StoreFile level since it is the user of HalfFile. I could do a test that had a StoreFile on a top half and another on a bottom half. Then I'd verify the seekBefore around the extremes (it should be doing the before as Anoop suggests and not returning the passed key, if it exists). This is a bad bug that you fellas have found.
          Hide
          Anoop Sam John added a comment -

          @Johnson
          I ran your test case and seen that...
          + // Seek on the splitKey, should be in top, not in bottom
          + KeyValue foundKeyValue = doTestOfSeekBefore(p, fs, bottom, midKV,cacheConf);
          + assertNull(foundKeyValue);

          You say here seek[seekBefore] on the splitkey in bottom file and as it is not there in this file for sure, it should return found KV as null ! If you test seekTo() on the bottom file passing the splitKey as the key, it will give the KV as the last KV in the file.. Not null... So we need to be consistent right

          I think it should not be null in this case... It is seekBefore splitkey... The greatest key in the file which is less than the passed key.. Correct right... So it should point to the lest key in the file right...

          Also wrt the functional reproduce I dont think it will happen.. This seekBefore is used by the getClosestRowBefore() call... When this call happens from the client with key as the splitkey or > splitkey, the region selected after looking into META will be top the region and thus call wont come to bottom file I think...

          But still the code in the HalfStoreFile, if it creates the StackOverflow, we can fix I feel...

          What about you Stack?

          Show
          Anoop Sam John added a comment - @Johnson I ran your test case and seen that... + // Seek on the splitKey, should be in top, not in bottom + KeyValue foundKeyValue = doTestOfSeekBefore(p, fs, bottom, midKV,cacheConf); + assertNull(foundKeyValue); You say here seek [seekBefore] on the splitkey in bottom file and as it is not there in this file for sure, it should return found KV as null ! If you test seekTo() on the bottom file passing the splitKey as the key, it will give the KV as the last KV in the file.. Not null... So we need to be consistent right I think it should not be null in this case... It is seekBefore splitkey... The greatest key in the file which is less than the passed key.. Correct right... So it should point to the lest key in the file right... Also wrt the functional reproduce I dont think it will happen.. This seekBefore is used by the getClosestRowBefore() call... When this call happens from the client with key as the splitkey or > splitkey, the region selected after looking into META will be top the region and thus call wont come to bottom file I think... But still the code in the HalfStoreFile, if it creates the StackOverflow, we can fix I feel... What about you Stack?
          Hide
          Todd Johnson added a comment -

          @Anoop, I think the confusion is that documentation quote is the HFileScanner doc. As far as we can tell, it seems that another class is coordinating two instances of the HalfStoreFileReader, one for the top and one for the bottom. Each of these will return false if the search key is in the other half, but the coordinating class will only return false in the condition you mention.

          It seems to me that if your interpretation was correct and our was incorrect, the test we submitted that searches for every key would fail the first time it searched for something in the bottom half. Have you run this test? Do you have any specific concerns about its accuracy? Do you have another test where our fix would fail, but yours would succeed? Can you submit it as a patch?

          Show
          Todd Johnson added a comment - @Anoop, I think the confusion is that documentation quote is the HFileScanner doc. As far as we can tell, it seems that another class is coordinating two instances of the HalfStoreFileReader, one for the top and one for the bottom. Each of these will return false if the search key is in the other half, but the coordinating class will only return false in the condition you mention. It seems to me that if your interpretation was correct and our was incorrect, the test we submitted that searches for every key would fail the first time it searched for something in the bottom half. Have you run this test? Do you have any specific concerns about its accuracy? Do you have another test where our fix would fail, but yours would succeed? Can you submit it as a patch?
          Hide
          Anoop Sam John added a comment -

          @Nate still the patch contains the return false change....
          Just see the javadoc for the seekBefore

          /**
          * Consider the key stream of all the keys in the file,
          * <code>k[0] .. k[n]</code>, where there are n keys in the file.
          * @param key Key to find
          * @return false if key <= k[0] or true with scanner in position 'i' such
          * that: k[i] < key.  Furthermore: there may be a k[i+1], such that
          * k[i] < key <= k[i+1] but there may also NOT be a k[i+1], and next() will
          * return false (EOF).
          * @throws IOException
          */
          public boolean seekBefore(byte [] key) throws IOException;
          

          This can return false only when the 1st key in the file is <= passed key

          In our case we check against the top boundary of the file...
          The passed key is >=splitkey and in the file we need to seek to a position which comes before this key... And our last key in the file will be that position... So no chance of returning false in the if check...

          U getting my point... Instead of this the fix for avoiding the stackoverflow to be

          if (getComparator().compare(key, offset, length, splitkey, 0,
          -   splitkey.length) >= 0) {
          +	splitkey.length) > 0) {
                      return seekBefore(splitkey, 0, splitkey.length);
          }
          

          IMO

          Show
          Anoop Sam John added a comment - @Nate still the patch contains the return false change.... Just see the javadoc for the seekBefore /** * Consider the key stream of all the keys in the file, * <code>k[0] .. k[n]</code>, where there are n keys in the file. * @param key Key to find * @ return false if key <= k[0] or true with scanner in position 'i' such * that: k[i] < key. Furthermore: there may be a k[i+1], such that * k[i] < key <= k[i+1] but there may also NOT be a k[i+1], and next() will * return false (EOF). * @ throws IOException */ public boolean seekBefore( byte [] key) throws IOException; This can return false only when the 1st key in the file is <= passed key In our case we check against the top boundary of the file... The passed key is >=splitkey and in the file we need to seek to a position which comes before this key... And our last key in the file will be that position... So no chance of returning false in the if check... U getting my point... Instead of this the fix for avoiding the stackoverflow to be if (getComparator().compare(key, offset, length, splitkey, 0, - splitkey.length) >= 0) { + splitkey.length) > 0) { return seekBefore(splitkey, 0, splitkey.length); } IMO
          Hide
          Hadoop QA added a comment -

          +1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12525534/HBASE-5922.v2.patch
          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 hadoop23. The patch compiles against the hadoop 0.23.x 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 core tests. The patch passed unit tests in .

          Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/1758//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/1758//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/1758//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/12525534/HBASE-5922.v2.patch 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 hadoop23. The patch compiles against the hadoop 0.23.x 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 core tests. The patch passed unit tests in . Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/1758//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/1758//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/1758//console This message is automatically generated.
          Hide
          Nate Putnam added a comment -

          Cool. Added a test that operates on the HFile instead of the References. That test will also reproduce the issue if the HalfStoreFileReader fix is removed.

          Thanks for your help on this.

          Show
          Nate Putnam added a comment - Cool. Added a test that operates on the HFile instead of the References. That test will also reproduce the issue if the HalfStoreFileReader fix is removed. Thanks for your help on this.
          Hide
          stack added a comment -

          Can we write a test where we have a multi-block hfile (can have small block sizes) and then have the file filled with keys that differ by one position only: e.g. a, b, c, etc. Then we'd test by getting every entry including outside of the files' bounds but also having top half reach into the bottom half and vice-versa? Would that help figure the code?

          Good stuff lads.

          Show
          stack added a comment - Can we write a test where we have a multi-block hfile (can have small block sizes) and then have the file filled with keys that differ by one position only: e.g. a, b, c, etc. Then we'd test by getting every entry including outside of the files' bounds but also having top half reach into the bottom half and vice-versa? Would that help figure the code? Good stuff lads.
          Hide
          Nate Putnam added a comment -

          @Anoop as far as reproducing the issue, I'm not sure the exact steps that would cause this in a production environment. The test case in the patch will reproduce the issue though.

          Show
          Nate Putnam added a comment - @Anoop as far as reproducing the issue, I'm not sure the exact steps that would cause this in a production environment. The test case in the patch will reproduce the issue though.
          Hide
          Todd Johnson added a comment -

          Yeah, that's not how it seems to me. But then, I didn't write the code originally, so perhaps I misunderstand it.

          We added a test case that causes infinite recursion with the old code, but appears to work with the patch. The test case searches for a key that is not equal to the split key. Given this, I don't see how the equals check could be the problem.

          Show
          Todd Johnson added a comment - Yeah, that's not how it seems to me. But then, I didn't write the code originally, so perhaps I misunderstand it. We added a test case that causes infinite recursion with the old code, but appears to work with the patch. The test case searches for a key that is not equal to the split key. Given this, I don't see how the equals check could be the problem.
          Hide
          Anoop Sam John added a comment -

          In case of the bottom file, if the passed key is >= splitkey, that is the case where we need to work with the passed key. This is not a case to return false. At this case ideally the the scanner should get pointed to the last key in the bottom file. Yes bottom file will not have the split key in it.
          So we should change the key and need to seekBefore the splitKey, which in turn can make the pointer to the last key. I think why the stack overflow was coming is clear to you... It is because of the = check .. That is some thing unwanted...

          Show
          Anoop Sam John added a comment - In case of the bottom file, if the passed key is >= splitkey, that is the case where we need to work with the passed key. This is not a case to return false. At this case ideally the the scanner should get pointed to the last key in the bottom file. Yes bottom file will not have the split key in it. So we should change the key and need to seekBefore the splitKey, which in turn can make the pointer to the last key. I think why the stack overflow was coming is clear to you... It is because of the = check .. That is some thing unwanted...
          Hide
          Todd Johnson added a comment -

          I worked with Nate on this yesterday. We couldn't think of any reason you would want the delegate to search for the splitkey, nor any reason this method would need to recursively call itself.

          Our reading of the code was that there are two reasons to return false: if 'top' is true (you're in the top half of the split file) and the search key is greater than the splitkey (this works now) OR if 'top' is false (you're in the bottom half of the file) and the search key is less-than-or-equal-to the splitkey (presumably, the splitkey is stored in the top half, thus or-equal-to).

          If neither of those conditions exist, there is a possibility of finding the search key in the half-file you're looking at, so you call the delegate.

          Show
          Todd Johnson added a comment - I worked with Nate on this yesterday. We couldn't think of any reason you would want the delegate to search for the splitkey, nor any reason this method would need to recursively call itself. Our reading of the code was that there are two reasons to return false: if 'top' is true (you're in the top half of the split file) and the search key is greater than the splitkey (this works now) OR if 'top' is false (you're in the bottom half of the file) and the search key is less-than-or-equal-to the splitkey (presumably, the splitkey is stored in the top half, thus or-equal-to). If neither of those conditions exist, there is a possibility of finding the search key in the half-file you're looking at, so you call the delegate.
          Hide
          Anoop Sam John added a comment -

          Checked other methods in HalfStoreFileReader. Looks ok to me...

          As Stack also asked how u get this issue in cluster? [Functionaly reproduce]

          Any way the code is supposed to handle these cases I feel and needs fix

          Show
          Anoop Sam John added a comment - Checked other methods in HalfStoreFileReader. Looks ok to me... As Stack also asked how u get this issue in cluster? [Functionaly reproduce] Any way the code is supposed to handle these cases I feel and needs fix
          Hide
          Anoop Sam John added a comment -

          @Stack, I have not gone though the test cases.. Seems like boudary condition is not checked.. As far as my analysis there are 2 bugs in this seekBefore().. I will take a look at the tests and the other methods of HalfStoreFileReader...
          Bugs
          1. As the case with Nate, Stackoverflow when seekBefore() called with a key>=splitKey
          on the bottom half file
          2. On the top half file a seekBefore() call with a key = splitkey is supposed to return false but it wont happen. It will try to seek into the bottom half I fear ..

          Show
          Anoop Sam John added a comment - @Stack, I have not gone though the test cases.. Seems like boudary condition is not checked.. As far as my analysis there are 2 bugs in this seekBefore().. I will take a look at the tests and the other methods of HalfStoreFileReader... Bugs 1. As the case with Nate, Stackoverflow when seekBefore() called with a key>=splitKey on the bottom half file 2. On the top half file a seekBefore() call with a key = splitkey is supposed to return false but it wont happen. It will try to seek into the bottom half I fear ..
          Hide
          stack added a comment -

          @Anoop Its late, but what you say makes sense. Looking over in our half file test, TestHalfStoreFileReader, it seems pretty poor coverage. What do you think? It does not seem to test the boundary condition Nate ran into or that you reason above?

          Show
          stack added a comment - @Anoop Its late, but what you say makes sense. Looking over in our half file test, TestHalfStoreFileReader, it seems pretty poor coverage. What do you think? It does not seem to test the boundary condition Nate ran into or that you reason above?
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12525391/HBASE-5922.patch
          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 hadoop23. The patch compiles against the hadoop 0.23.x 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 core tests. The patch failed these unit tests:
          org.apache.hadoop.hbase.TestDrainingServer
          org.apache.hadoop.hbase.avro.TestAvroServer
          org.apache.hadoop.hbase.master.TestSplitLogManager

          Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/1739//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/1739//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/1739//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/12525391/HBASE-5922.patch 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 hadoop23. The patch compiles against the hadoop 0.23.x 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 core tests. The patch failed these unit tests: org.apache.hadoop.hbase.TestDrainingServer org.apache.hadoop.hbase.avro.TestAvroServer org.apache.hadoop.hbase.master.TestSplitLogManager Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/1739//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/1739//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/1739//console This message is automatically generated.
          Hide
          Anoop Sam John added a comment -

          After reading the code for HalfStoreFileReader and the javadoc for the HFileScanner.seekBefore(), got more confusion with the code...

          /**
          * Consider the key stream of all the keys in the file,
          * <code>k[0] .. k[n]</code>, where there are n keys in the file.
          * @param key Key to find
          * @return false if key <= k[0] or true with scanner in position 'i' such
          * that: k[i] < key.  Furthermore: there may be a k[i+1], such that
          * k[i] < key <= k[i+1] but there may also NOT be a k[i+1], and next() will
          * return false (EOF).
          * @throws IOException
          */
          public boolean seekBefore(byte [] key) throws IOException;
          

          Here is the code for seekBefore in HalfStoreFileReader
          For top half file

          if (top) {
            if (getComparator().compare(key, offset, length, splitkey, 0,
          	  splitkey.length) < 0) {
          	return false;
            }
          }
          

          The top file is from [splitkey,endkey). When the passed key is less than or equal to the splitkey, we wont be able to find a point i in the file such that k[i]<key.. This should be a case of return false. So here we should have if condition with <= 0 ?

          Code for bottom file

          else{
          	if (getComparator().compare(key, offset, length, splitkey, 0,
          	  splitkey.length) >= 0) {
          	return seekBefore(splitkey, 0, splitkey.length);
          	}
          }
          

          The bottom file is from [startkey,splitkey). When the passed key is >= the split key we can point to the last key in the file. I think for this only seekBefore splitKey was done. But as it checks =0 also this will result in infinite calls to seekBefore

          So may be we can change the condition from >=0 to >0 only... Then the actual seekBefore will be done by the delegate.seekBefore() call... But return false for getComparator().compare(key, offset, length, splitkey, 0, splitkey.length) >= 0 will be wrong I feel.....

          @Stack, @Nate What do u say?

          Also this bug is applicable for other versions too...

          Show
          Anoop Sam John added a comment - After reading the code for HalfStoreFileReader and the javadoc for the HFileScanner.seekBefore(), got more confusion with the code... /** * Consider the key stream of all the keys in the file, * <code>k[0] .. k[n]</code>, where there are n keys in the file. * @param key Key to find * @ return false if key <= k[0] or true with scanner in position 'i' such * that: k[i] < key. Furthermore: there may be a k[i+1], such that * k[i] < key <= k[i+1] but there may also NOT be a k[i+1], and next() will * return false (EOF). * @ throws IOException */ public boolean seekBefore( byte [] key) throws IOException; Here is the code for seekBefore in HalfStoreFileReader For top half file if (top) { if (getComparator().compare(key, offset, length, splitkey, 0, splitkey.length) < 0) { return false ; } } The top file is from [splitkey,endkey). When the passed key is less than or equal to the splitkey, we wont be able to find a point i in the file such that k [i] <key.. This should be a case of return false. So here we should have if condition with <= 0 ? Code for bottom file else { if (getComparator().compare(key, offset, length, splitkey, 0, splitkey.length) >= 0) { return seekBefore(splitkey, 0, splitkey.length); } } The bottom file is from [startkey,splitkey). When the passed key is >= the split key we can point to the last key in the file. I think for this only seekBefore splitKey was done. But as it checks =0 also this will result in infinite calls to seekBefore So may be we can change the condition from >=0 to >0 only... Then the actual seekBefore will be done by the delegate.seekBefore() call... But return false for getComparator().compare(key, offset, length, splitkey, 0, splitkey.length) >= 0 will be wrong I feel..... @Stack, @Nate What do u say? Also this bug is applicable for other versions too...
          Hide
          stack added a comment -

          Retry

          Show
          stack added a comment - Retry
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12525389/HBASE-5922.patch
          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 hadoop23. The patch compiles against the hadoop 0.23.x 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 core tests. The patch failed these unit tests:
          org.apache.hadoop.hbase.regionserver.TestServerCustomProtocol
          org.apache.hadoop.hbase.master.TestSplitLogManager

          Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/1738//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/1738//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/1738//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/12525389/HBASE-5922.patch 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 hadoop23. The patch compiles against the hadoop 0.23.x 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 core tests. The patch failed these unit tests: org.apache.hadoop.hbase.regionserver.TestServerCustomProtocol org.apache.hadoop.hbase.master.TestSplitLogManager Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/1738//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/1738//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/1738//console This message is automatically generated.
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12525365/HBase-5922.patch
          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 hadoop23. The patch compiles against the hadoop 0.23.x 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 core tests. The patch failed these unit tests:
          org.apache.hadoop.hbase.io.TestHalfStoreFileReader

          Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/1734//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/1734//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/1734//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/12525365/HBase-5922.patch 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 hadoop23. The patch compiles against the hadoop 0.23.x 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 core tests. The patch failed these unit tests: org.apache.hadoop.hbase.io.TestHalfStoreFileReader Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/1734//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/1734//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/1734//console This message is automatically generated.
          Hide
          Hadoop QA added a comment -

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

          Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/1731//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/12525357/HBase-5922.patch 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 patch. The patch command could not apply the patch. Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/1731//console This message is automatically generated.
          Hide
          stack added a comment -

          Hmm... maybe your fix is right. There are other tests of half file, ones we added when we ran into issues w/ it in the past. Lets try your patch against hadoopqa.

          Show
          stack added a comment - Hmm... maybe your fix is right. There are other tests of half file, ones we added when we ran into issues w/ it in the past. Lets try your patch against hadoopqa.
          Hide
          Nate Putnam added a comment -

          I was able to reproduce it by doing a seekBefore on a row key that was contained in the top of the HalfStoreFile.

          Show
          Nate Putnam added a comment - I was able to reproduce it by doing a seekBefore on a row key that was contained in the top of the HalfStoreFile.
          Hide
          stack added a comment -
          ===================================================================
          --- src/main/java/org/apache/hadoop/hbase/io/HalfStoreFileReader.java	(revision 1169834)
          +++ src/main/java/org/apache/hadoop/hbase/io/HalfStoreFileReader.java	(revision )
          @@ -146,7 +146,7 @@
                   } else {
                     if (getComparator().compare(key, offset, length, splitkey, 0,
                         splitkey.length) >= 0) {
          -            return seekBefore(splitkey, 0, splitkey.length);
          +            return false;
                     }
          

          So, if result is > splitKey, we need to get something before the splitkey, not fail?

          Maybe we should check what comes out of the seekBefore.... how is it that its returning us splitkey again?

          Show
          stack added a comment - =================================================================== --- src/main/java/org/apache/hadoop/hbase/io/HalfStoreFileReader.java (revision 1169834) +++ src/main/java/org/apache/hadoop/hbase/io/HalfStoreFileReader.java (revision ) @@ -146,7 +146,7 @@ } else { if (getComparator().compare(key, offset, length, splitkey, 0, splitkey.length) >= 0) { - return seekBefore(splitkey, 0, splitkey.length); + return false ; } So, if result is > splitKey, we need to get something before the splitkey, not fail? Maybe we should check what comes out of the seekBefore.... how is it that its returning us splitkey again?
          Hide
          stack added a comment -

          When does this issue arise? When we try to getclosest on split key?

          Show
          stack added a comment - When does this issue arise? When we try to getclosest on split key?

            People

            • Assignee:
              Nate Putnam
              Reporter:
              Nate Putnam
            • Votes:
              0 Vote for this issue
              Watchers:
              5 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development