HBase
  1. HBase
  2. HBASE-5121

MajorCompaction may affect scan's correctness

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Critical Critical
    • Resolution: Fixed
    • Affects Version/s: 0.90.4
    • Fix Version/s: 0.92.1, 0.94.0
    • Component/s: regionserver
    • Labels:
      None
    • Hadoop Flags:
      Reviewed

      Description

      In our test, there are two families' keyvalue for one row.

      But we could find a infrequent problem when doing scan's next if majorCompaction happens concurrently.
      In the client's two continuous doing scan.next():
      1.First time, scan's next returns the result where family A is null.
      2.Second time, scan's next returns the result where family B is null.
      The two next()'s result have the same row.

      If there are more families, I think the scenario will be more strange...

      We find the reason is that storescanner.peek() is changed after majorCompaction if there are delete type KeyValue.
      This change causes the PriorityQueue<KeyValueScanner> of RegionScanner's heap is not sure to be sorted.

      1. hbase-5121v2.patch
        3 kB
        chunhui shen
      2. hbase-5121-testcase.patch
        4 kB
        chunhui shen
      3. hbase-5121.patch
        4 kB
        chunhui shen
      4. 5121-trunk-combined.txt
        9 kB
        Ted Yu
      5. 5121-suggest.txt
        7 kB
        Lars Hofhansl
      6. 5121-0.92.txt
        7 kB
        Lars Hofhansl
      7. 5121.90
        8 kB
        Ted Yu

        Issue Links

          Activity

          Hide
          Ted Yu added a comment - - edited
          +        this.lastTop = null;
          +        LOG.debug("Storescanner.peek() is changed where before = "
          +            + this.lastTop.toString() + ",and after = "
          

          Should null assignment for lastTop be after the debug log ?

          Show
          Ted Yu added a comment - - edited + this .lastTop = null ; + LOG.debug( "Storescanner.peek() is changed where before = " + + this .lastTop.toString() + ",and after = " Should null assignment for lastTop be after the debug log ?
          Hide
          Lars Hofhansl added a comment -

          I see the patch is against trunk. Does this happen in trunk only?

          It might have to do with the various optimization for scanning that the FB folks put in (if I recall FB rarely does major compactions).
          Maybe Mikhail could have a look at this too.

          Show
          Lars Hofhansl added a comment - I see the patch is against trunk. Does this happen in trunk only? It might have to do with the various optimization for scanning that the FB folks put in (if I recall FB rarely does major compactions). Maybe Mikhail could have a look at this too.
          Hide
          Todd Lipcon added a comment -

          No test for this? Can we get a functional test that can at least be run from the command line to trigger it?

          Nits: the javadoc on the constructors are unnecessary (they don't give any extra information)
          Using the exception for control flow here looks dirty IMO.

          Show
          Todd Lipcon added a comment - No test for this? Can we get a functional test that can at least be run from the command line to trigger it? Nits: the javadoc on the constructors are unnecessary (they don't give any extra information) Using the exception for control flow here looks dirty IMO.
          Hide
          chunhui shen added a comment -

          @Ted
          I'm sorry for the mistake when making patch for trunk.

          Show
          chunhui shen added a comment - @Ted I'm sorry for the mistake when making patch for trunk.
          Hide
          chunhui shen added a comment -

          @Lars
          Our test environment uses the 0.90 version,but I think it will also happens in 0.92 and trunk too.

          Show
          chunhui shen added a comment - @Lars Our test environment uses the 0.90 version,but I think it will also happens in 0.92 and trunk too.
          Hide
          chunhui shen added a comment -

          @Todd
          I will append tests for this patch later.

          Show
          chunhui shen added a comment - @Todd I will append tests for this patch later.
          Hide
          chunhui shen added a comment -

          I add a unit test case in hbase-5121-testcase.patch for this issue.
          And the solution is hbase-5121v2.patch

          Show
          chunhui shen added a comment - I add a unit test case in hbase-5121-testcase.patch for this issue. And the solution is hbase-5121v2.patch
          Hide
          Ted Yu added a comment -

          From https://builds.apache.org/job/PreCommit-HBASE-Build/672/console:

          HBASE-5121 patch is being downloaded at Thu Jan  5 10:51:42 UTC 2012 from
          http://issues.apache.org/jira/secure/attachment/12509522/hbase-5121-testcase.patch
          

          In the future, please attach the patch with solution last so that Hadoop QA can pick up.

          Show
          Ted Yu added a comment - From https://builds.apache.org/job/PreCommit-HBASE-Build/672/console: HBASE-5121 patch is being downloaded at Thu Jan 5 10:51:42 UTC 2012 from http: //issues.apache.org/jira/secure/attachment/12509522/hbase-5121-testcase.patch In the future, please attach the patch with solution last so that Hadoop QA can pick up.
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12509522/hbase-5121-testcase.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 javadoc. The javadoc tool appears to have generated -151 warning messages.

          +1 javac. The applied patch does not increase the total number of javac compiler warnings.

          -1 findbugs. The patch appears to introduce 79 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.TestScanner

          Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/672//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/672//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/672//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/12509522/hbase-5121-testcase.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 javadoc. The javadoc tool appears to have generated -151 warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. -1 findbugs. The patch appears to introduce 79 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.TestScanner Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/672//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/672//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/672//console This message is automatically generated.
          Hide
          Ted Yu added a comment - - edited

          The above test failure shows the bug, that's great.

          About the patch itself, some minor comments:
          Please add Apache license to PeekChangedException header.

          +            + this.lastTop.toString() + ",and after = "
          +            + (this.heap.peek() == null ? "null" : this.heap.peek().toString()));
          

          The null check on the second line is not needed for string concatenation. See http://docs.oracle.com/javase/1.5.0/docs/api/java/lang/StringBuffer.html#append%28java.lang.String%29

          +      mayContainsMoreRows = currentAsInternal.next(result, limit);
          

          Since this part of the code is changed, please name the variable mayContainMoreRows.

          Please combine the two patches in one next time they're uploaded.

          Great work, Chunhui.

          Show
          Ted Yu added a comment - - edited The above test failure shows the bug, that's great. About the patch itself, some minor comments: Please add Apache license to PeekChangedException header. + + this .lastTop.toString() + ",and after = " + + ( this .heap.peek() == null ? " null " : this .heap.peek().toString())); The null check on the second line is not needed for string concatenation. See http://docs.oracle.com/javase/1.5.0/docs/api/java/lang/StringBuffer.html#append%28java.lang.String%29 + mayContainsMoreRows = currentAsInternal.next(result, limit); Since this part of the code is changed, please name the variable mayContainMoreRows. Please combine the two patches in one next time they're uploaded. Great work, Chunhui.
          Hide
          Ted Yu added a comment -

          For the new test:

          +    } catch (Exception e) {
          +      LOG.error("Failed", e);
          +      throw e;
          

          Is the above needed ? The exception, if thrown, would fail the test anyway.

          Show
          Ted Yu added a comment - For the new test: + } catch (Exception e) { + LOG.error( "Failed" , e); + throw e; Is the above needed ? The exception, if thrown, would fail the test anyway.
          Hide
          chunhui shen added a comment -

          @Ted
          Thanks for the advice.
          I will submit a new patch later.

          Show
          chunhui shen added a comment - @Ted Thanks for the advice. I will submit a new patch later.
          Hide
          Ted Yu added a comment -

          Combined patch for TRUNK based on the two files Chunhui provided.

          Putting it through tests and waiting for more reviews.

          Show
          Ted Yu added a comment - Combined patch for TRUNK based on the two files Chunhui provided. Putting it through tests and waiting for more reviews.
          Hide
          Lars Hofhansl added a comment -

          We find the reason is that storescanner.peek() is changed after majorCompaction if there are delete type KeyValue.

          Do we know why/how that happens? Just want to make sure that we're not just pasting over symptoms with this.

          Show
          Lars Hofhansl added a comment - We find the reason is that storescanner.peek() is changed after majorCompaction if there are delete type KeyValue. Do we know why/how that happens? Just want to make sure that we're not just pasting over symptoms with this.
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12509573/5121-trunk-combined.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 javadoc. The javadoc tool appears to have generated -151 warning messages.

          +1 javac. The applied patch does not increase the total number of javac compiler warnings.

          -1 findbugs. The patch appears to introduce 79 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.replication.TestReplicationPeer
          org.apache.hadoop.hbase.replication.TestReplication
          org.apache.hadoop.hbase.mapreduce.TestImportTsv
          org.apache.hadoop.hbase.mapred.TestTableMapReduce
          org.apache.hadoop.hbase.mapreduce.TestHFileOutputFormat

          Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/673//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/673//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/673//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/12509573/5121-trunk-combined.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 javadoc. The javadoc tool appears to have generated -151 warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. -1 findbugs. The patch appears to introduce 79 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.replication.TestReplicationPeer org.apache.hadoop.hbase.replication.TestReplication org.apache.hadoop.hbase.mapreduce.TestImportTsv org.apache.hadoop.hbase.mapred.TestTableMapReduce org.apache.hadoop.hbase.mapreduce.TestHFileOutputFormat Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/673//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/673//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/673//console This message is automatically generated.
          Hide
          Nicolas Spiegelberg added a comment -

          Seems like this touches the work that Amitanand worked on with HBASE-2856. You should have him look at this.

          Show
          Nicolas Spiegelberg added a comment - Seems like this touches the work that Amitanand worked on with HBASE-2856 . You should have him look at this.
          Hide
          Ted Yu added a comment -

          Amit's review is certainly welcome.
          We may add more tests involving deletion and major compaction.

          Show
          Ted Yu added a comment - Amit's review is certainly welcome. We may add more tests involving deletion and major compaction.
          Hide
          chunhui shen added a comment -

          @Lars
          Its happen need two conditions.
          1.storescanner.peek() is delete type keyvalue.
          2.do majorcompaction before scan.next(), and this majorcompaction combine the del type KV and put type KV to nothing.

          Show
          chunhui shen added a comment - @Lars Its happen need two conditions. 1.storescanner.peek() is delete type keyvalue. 2.do majorcompaction before scan.next(), and this majorcompaction combine the del type KV and put type KV to nothing.
          Hide
          Ted Yu added a comment -

          @Chunhui:
          Do you mind producing a patch for 0.90 branch ?
          If you're busy, I can do it.

          Show
          Ted Yu added a comment - @Chunhui: Do you mind producing a patch for 0.90 branch ? If you're busy, I can do it.
          Hide
          Ted Yu added a comment -

          Patch for 0.90 branch.

          Show
          Ted Yu added a comment - Patch for 0.90 branch.
          Hide
          ramkrishna.s.vasudevan added a comment -

          @Ted
          Thanks for the 0.90 patch.

          Show
          ramkrishna.s.vasudevan added a comment - @Ted Thanks for the 0.90 patch.
          Hide
          Ted Yu added a comment -

          People expressed concern whether introducing a new exception is the best approach to take.

          @Chunhui:
          Please extrapolate on other alternatives.

          Thanks

          Show
          Ted Yu added a comment - People expressed concern whether introducing a new exception is the best approach to take. @Chunhui: Please extrapolate on other alternatives. Thanks
          Hide
          chunhui shen added a comment -

          @Ted
          when majorcompaction changes storescanner.peek, we need to tell the upper storeheap(regionscanner's heap) to resort this storescanner again.
          we have tried some other approaches for the above motivation.But we think throwing an exception is much easier and available to transfer the message of peek changed.
          It only happen in a read handler. So it is healthy and will not affect the kernal thread of regionserver.

          If there are other approaches, I'd like to discuss about them.
          Thanks.

          Show
          chunhui shen added a comment - @Ted when majorcompaction changes storescanner.peek, we need to tell the upper storeheap(regionscanner's heap) to resort this storescanner again. we have tried some other approaches for the above motivation.But we think throwing an exception is much easier and available to transfer the message of peek changed. It only happen in a read handler. So it is healthy and will not affect the kernal thread of regionserver. If there are other approaches, I'd like to discuss about them. Thanks.
          Hide
          Ted Yu added a comment -

          w.r.t. introducing new exception, here is StoreScanner.next():

             * @return true if there are more rows, false if scanner is done
             */
            @Override
            public synchronized boolean next(List<KeyValue> outResult, int limit) throws IOException {
          
              checkReseek();
          

          The return value has clear meaning. One approach is to change return type to an enum.

          @Chunhui:
          Do you have time to come up with a patch that doesn't rely on new exception. Instead, it relies on StoreScanner.next() returning an enum (SCAN_DONE, MORE_ROWS, PEEK_CHANGED).
          This way, people can evaluate both approaches.

          Of course, Stack and Todd may comment on other areas of the patch.

          Show
          Ted Yu added a comment - w.r.t. introducing new exception, here is StoreScanner.next(): * @ return true if there are more rows, false if scanner is done */ @Override public synchronized boolean next(List<KeyValue> outResult, int limit) throws IOException { checkReseek(); The return value has clear meaning. One approach is to change return type to an enum. @Chunhui: Do you have time to come up with a patch that doesn't rely on new exception. Instead, it relies on StoreScanner.next() returning an enum (SCAN_DONE, MORE_ROWS, PEEK_CHANGED). This way, people can evaluate both approaches. Of course, Stack and Todd may comment on other areas of the patch.
          Hide
          chunhui shen added a comment -

          @Ted
          ok, I will make a new patch

          Show
          chunhui shen added a comment - @Ted ok, I will make a new patch
          Hide
          chunhui shen added a comment -

          @Ted

          If we change StoreScanner.next() to return an enum , we should change all the implement of InternalScanner.next(), therefore KeyValueHeap.next()'s return should also be changed to an enum. It needs to change the logic who calles KeyValueHeap.next() .

          I think it changes too much, does it need?

          Show
          chunhui shen added a comment - @Ted If we change StoreScanner.next() to return an enum , we should change all the implement of InternalScanner.next(), therefore KeyValueHeap.next()'s return should also be changed to an enum. It needs to change the logic who calles KeyValueHeap.next() . I think it changes too much, does it need?
          Hide
          Ted Yu added a comment -

          @Chunhui:
          I agree.

          That's why I think creating a new exception is acceptable.
          Maybe Todd or Stack has better idea.

          Show
          Ted Yu added a comment - @Chunhui: I agree. That's why I think creating a new exception is acceptable. Maybe Todd or Stack has better idea.
          Hide
          Lars Hofhansl added a comment -

          I was looking at the patch a bit. Maybe there is a simpler solution:
          You say in that when this scenario happens the KV just "vanishes".
          What your logic in KeyValueHeap is essentially doing is to retry with the next KV on the heap.
          So, we can just tell the KeyValueHeap that there are more KVs in this case (returning true for mayContainMoreRows).

          With this your test passes.
          (it is entirely possible that my reasoning is incorrect, and it just accidentally lets the test pass).

          Show
          Lars Hofhansl added a comment - I was looking at the patch a bit. Maybe there is a simpler solution: You say in that when this scenario happens the KV just "vanishes". What your logic in KeyValueHeap is essentially doing is to retry with the next KV on the heap. So, we can just tell the KeyValueHeap that there are more KVs in this case (returning true for mayContainMoreRows). With this your test passes. (it is entirely possible that my reasoning is incorrect, and it just accidentally lets the test pass).
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12509823/5121-suggest.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 javadoc. The javadoc tool appears to have generated -151 warning messages.

          +1 javac. The applied patch does not increase the total number of javac compiler warnings.

          -1 findbugs. The patch appears to introduce 79 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.mapreduce.TestImportTsv
          org.apache.hadoop.hbase.mapred.TestTableMapReduce
          org.apache.hadoop.hbase.mapreduce.TestHFileOutputFormat

          Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/700//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/700//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/700//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/12509823/5121-suggest.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 javadoc. The javadoc tool appears to have generated -151 warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. -1 findbugs. The patch appears to introduce 79 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.mapreduce.TestImportTsv org.apache.hadoop.hbase.mapred.TestTableMapReduce org.apache.hadoop.hbase.mapreduce.TestHFileOutputFormat Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/700//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/700//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/700//console This message is automatically generated.
          Hide
          chunhui shen added a comment -

          @Lars
          I agree with the suggested patch which is simpler and available.

          Show
          chunhui shen added a comment - @Lars I agree with the suggested patch which is simpler and available.
          Hide
          Lars Hofhansl added a comment -

          Any more input on the latest patch?

          Re: 0.90.6. It seems we should do this only in 0.92 and above as 0.90.x does not provide many of the ACID guarantees anyway (see HBASE-2856 and HBASE-4838).

          Show
          Lars Hofhansl added a comment - Any more input on the latest patch? Re: 0.90.6. It seems we should do this only in 0.92 and above as 0.90.x does not provide many of the ACID guarantees anyway (see HBASE-2856 and HBASE-4838 ).
          Hide
          Ted Yu added a comment -

          +1 on latest patch going to 0.92 and TRUNK.

          Show
          Ted Yu added a comment - +1 on latest patch going to 0.92 and TRUNK.
          Hide
          Lars Hofhansl added a comment -

          OK... will commit tomorrow unless there are any objections.
          @Ram: You OK with this not being in 0.90.6?

          Show
          Lars Hofhansl added a comment - OK... will commit tomorrow unless there are any objections. @Ram: You OK with this not being in 0.90.6?
          Hide
          ramkrishna.s.vasudevan added a comment -

          @Lars
          Ok for not going into 0.90.6. Thanks Lars.

          Show
          ramkrishna.s.vasudevan added a comment - @Lars Ok for not going into 0.90.6. Thanks Lars.
          Hide
          Lars Hofhansl added a comment -

          Patch against 0.92

          Show
          Lars Hofhansl added a comment - Patch against 0.92
          Hide
          Lars Hofhansl added a comment -

          Committed to 0.92 and trunk (5121-suggest.txt)

          Show
          Lars Hofhansl added a comment - Committed to 0.92 and trunk (5121-suggest.txt)
          Hide
          Hudson added a comment -

          Integrated in HBase-0.92 #239 (See https://builds.apache.org/job/HBase-0.92/239/)
          HBASE-5121 MajorCompaction may affect scan's correctness (chunhui shen and Lars H)

          larsh :
          Files :

          • /hbase/branches/0.92/CHANGES.txt
          • /hbase/branches/0.92/src/main/java/org/apache/hadoop/hbase/regionserver/KeyValueHeap.java
          • /hbase/branches/0.92/src/main/java/org/apache/hadoop/hbase/regionserver/StoreScanner.java
          • /hbase/branches/0.92/src/test/java/org/apache/hadoop/hbase/regionserver/TestScanner.java
          Show
          Hudson added a comment - Integrated in HBase-0.92 #239 (See https://builds.apache.org/job/HBase-0.92/239/ ) HBASE-5121 MajorCompaction may affect scan's correctness (chunhui shen and Lars H) larsh : Files : /hbase/branches/0.92/CHANGES.txt /hbase/branches/0.92/src/main/java/org/apache/hadoop/hbase/regionserver/KeyValueHeap.java /hbase/branches/0.92/src/main/java/org/apache/hadoop/hbase/regionserver/StoreScanner.java /hbase/branches/0.92/src/test/java/org/apache/hadoop/hbase/regionserver/TestScanner.java
          Hide
          Hudson added a comment -

          Integrated in HBase-0.92-security #71 (See https://builds.apache.org/job/HBase-0.92-security/71/)
          HBASE-5121 MajorCompaction may affect scan's correctness (chunhui shen and Lars H)

          larsh :
          Files :

          • /hbase/branches/0.92/CHANGES.txt
          • /hbase/branches/0.92/src/main/java/org/apache/hadoop/hbase/regionserver/KeyValueHeap.java
          • /hbase/branches/0.92/src/main/java/org/apache/hadoop/hbase/regionserver/StoreScanner.java
          • /hbase/branches/0.92/src/test/java/org/apache/hadoop/hbase/regionserver/TestScanner.java
          Show
          Hudson added a comment - Integrated in HBase-0.92-security #71 (See https://builds.apache.org/job/HBase-0.92-security/71/ ) HBASE-5121 MajorCompaction may affect scan's correctness (chunhui shen and Lars H) larsh : Files : /hbase/branches/0.92/CHANGES.txt /hbase/branches/0.92/src/main/java/org/apache/hadoop/hbase/regionserver/KeyValueHeap.java /hbase/branches/0.92/src/main/java/org/apache/hadoop/hbase/regionserver/StoreScanner.java /hbase/branches/0.92/src/test/java/org/apache/hadoop/hbase/regionserver/TestScanner.java
          Hide
          Hudson added a comment -

          Integrated in HBase-TRUNK-security #72 (See https://builds.apache.org/job/HBase-TRUNK-security/72/)
          HBASE-5121 MajorCompaction may affect scan's correctness (chunhui shen and Lars H)

          larsh :
          Files :

          • /hbase/trunk/CHANGES.txt
          • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/KeyValueHeap.java
          • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/StoreScanner.java
          • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/regionserver/TestScanner.java
          Show
          Hudson added a comment - Integrated in HBase-TRUNK-security #72 (See https://builds.apache.org/job/HBase-TRUNK-security/72/ ) HBASE-5121 MajorCompaction may affect scan's correctness (chunhui shen and Lars H) larsh : Files : /hbase/trunk/CHANGES.txt /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/KeyValueHeap.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/StoreScanner.java /hbase/trunk/src/test/java/org/apache/hadoop/hbase/regionserver/TestScanner.java
          Hide
          Hudson added a comment -

          Integrated in HBase-TRUNK #2617 (See https://builds.apache.org/job/HBase-TRUNK/2617/)
          HBASE-5121 MajorCompaction may affect scan's correctness (chunhui shen and Lars H)

          larsh :
          Files :

          • /hbase/trunk/CHANGES.txt
          • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/KeyValueHeap.java
          • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/StoreScanner.java
          • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/regionserver/TestScanner.java
          Show
          Hudson added a comment - Integrated in HBase-TRUNK #2617 (See https://builds.apache.org/job/HBase-TRUNK/2617/ ) HBASE-5121 MajorCompaction may affect scan's correctness (chunhui shen and Lars H) larsh : Files : /hbase/trunk/CHANGES.txt /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/KeyValueHeap.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/StoreScanner.java /hbase/trunk/src/test/java/org/apache/hadoop/hbase/regionserver/TestScanner.java
          Hide
          Lars Hofhansl added a comment -

          Thanks for finding and patch chunhui.

          Show
          Lars Hofhansl added a comment - Thanks for finding and patch chunhui.
          Hide
          Lars Hofhansl added a comment - - edited

          @chunhui: Could have a look at the suggested change in HBASE-5659?
          The observation is that:

          1. after the scanner is reset the top KV will in the vast majority of all case be changed
          2. that is only a problem when the top KV's row has changed, in many case StoreScanner.next indicates more work than necessary to a caller.

          Edit: Misspelled chunhui's name...

          Show
          Lars Hofhansl added a comment - - edited @chunhui: Could have a look at the suggested change in HBASE-5659 ? The observation is that: after the scanner is reset the top KV will in the vast majority of all case be changed that is only a problem when the top KV's row has changed, in many case StoreScanner.next indicates more work than necessary to a caller. Edit: Misspelled chunhui's name...

            People

            • Assignee:
              chunhui shen
              Reporter:
              chunhui shen
            • Votes:
              0 Vote for this issue
              Watchers:
              10 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development