HBase
  1. HBase
  2. HBASE-5520

Support reseek() at RegionScanner

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 0.92.0
    • Fix Version/s: 0.94.0, 0.95.0
    • Component/s: regionserver
    • Labels:
      None
    • Hadoop Flags:
      Reviewed

      Description

      reseek() is not supported currently at the RegionScanner level. We can support the same.
      This is created following the discussion under HBASE-2038

      1. HBASE-5520_1.patch
        13 kB
        ramkrishna.s.vasudevan
      2. HBASE-5520_2.patch
        13 kB
        ramkrishna.s.vasudevan
      3. HBASE-5520_3.patch
        11 kB
        ramkrishna.s.vasudevan
      4. HBASE-5520_4.patch
        9 kB
        ramkrishna.s.vasudevan

        Issue Links

          Activity

          Lars Hofhansl made changes -
          Fix Version/s 0.94.0 [ 12316419 ]
          stack made changes -
          Fix Version/s 0.95.0 [ 12324094 ]
          Fix Version/s 0.94.0 [ 12316419 ]
          Fix Version/s 0.96.0 [ 12320040 ]
          Lars Hofhansl made changes -
          Status Resolved [ 5 ] Closed [ 6 ]
          Hide
          Hudson added a comment -

          Integrated in HBase-TRUNK-security #144 (See https://builds.apache.org/job/HBase-TRUNK-security/144/)
          HBASE-5520 Support reseek() at RegionScanner (Ram) (Revision 1303005)

          Result = FAILURE
          ramkrishna :
          Files :

          • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
          • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/RegionScanner.java
          • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/coprocessor/TestCoprocessorInterface.java
          • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/filter/TestFilter.java
          Show
          Hudson added a comment - Integrated in HBase-TRUNK-security #144 (See https://builds.apache.org/job/HBase-TRUNK-security/144/ ) HBASE-5520 Support reseek() at RegionScanner (Ram) (Revision 1303005) Result = FAILURE ramkrishna : Files : /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/RegionScanner.java /hbase/trunk/src/test/java/org/apache/hadoop/hbase/coprocessor/TestCoprocessorInterface.java /hbase/trunk/src/test/java/org/apache/hadoop/hbase/filter/TestFilter.java
          ramkrishna.s.vasudevan made changes -
          Status Patch Available [ 10002 ] Resolved [ 5 ]
          Hadoop Flags Reviewed [ 10343 ]
          Resolution Fixed [ 1 ]
          Hide
          Hudson added a comment -

          Integrated in HBase-TRUNK #2688 (See https://builds.apache.org/job/HBase-TRUNK/2688/)
          HBASE-5520 Support reseek() at RegionScanner (Ram) (Revision 1303005)

          Result = SUCCESS
          ramkrishna :
          Files :

          • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
          • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/RegionScanner.java
          • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/coprocessor/TestCoprocessorInterface.java
          • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/filter/TestFilter.java
          Show
          Hudson added a comment - Integrated in HBase-TRUNK #2688 (See https://builds.apache.org/job/HBase-TRUNK/2688/ ) HBASE-5520 Support reseek() at RegionScanner (Ram) (Revision 1303005) Result = SUCCESS ramkrishna : Files : /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/RegionScanner.java /hbase/trunk/src/test/java/org/apache/hadoop/hbase/coprocessor/TestCoprocessorInterface.java /hbase/trunk/src/test/java/org/apache/hadoop/hbase/filter/TestFilter.java
          Hide
          Hudson added a comment -

          Integrated in HBase-0.94 #42 (See https://builds.apache.org/job/HBase-0.94/42/)
          HBASE-5520 - 0.94 patch did not apply cleanly . Updated it (Revision 1303019)

          Result = FAILURE
          ramkrishna :
          Files :

          • /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/regionserver/RegionScanner.java
          Show
          Hudson added a comment - Integrated in HBase-0.94 #42 (See https://builds.apache.org/job/HBase-0.94/42/ ) HBASE-5520 - 0.94 patch did not apply cleanly . Updated it (Revision 1303019) Result = FAILURE ramkrishna : Files : /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/regionserver/RegionScanner.java
          Hide
          ramkrishna.s.vasudevan added a comment -

          @Lars
          I have immediately corrected that and now it is running fine.

          Show
          ramkrishna.s.vasudevan added a comment - @Lars I have immediately corrected that and now it is running fine.
          Hide
          Lars Hofhansl added a comment -

          Wait... Just looked at the runs output again, this is related to this patch.
          So Ted's right, either commit an addendum or revert the patch please. Thanks.

          Show
          Lars Hofhansl added a comment - Wait... Just looked at the runs output again, this is related to this patch. So Ted's right, either commit an addendum or revert the patch please. Thanks.
          Hide
          Lars Hofhansl added a comment -

          We got an earlier successful test run. The patch is small and adds only new functionality.
          Ram, I think you can you just run the tests locally and revert only if you see any problem.

          Show
          Lars Hofhansl added a comment - We got an earlier successful test run. The patch is small and adds only new functionality. Ram, I think you can you just run the tests locally and revert only if you see any problem.
          Hide
          Ted Yu added a comment -

          The patch resulted in compilation error.

          Hadoop QA has stopped functioning.

          Please revert the patch, fix the compilation error, run the whole test suite and publish test result.

          Show
          Ted Yu added a comment - The patch resulted in compilation error. Hadoop QA has stopped functioning. Please revert the patch, fix the compilation error, run the whole test suite and publish test result.
          Hide
          Hudson added a comment -

          Integrated in HBase-0.94 #41 (See https://builds.apache.org/job/HBase-0.94/41/)
          HBASE-5520 Support reseek() at RegionScanner (Ram) (Revision 1303006)

          Result = FAILURE
          ramkrishna :
          Files :

          • /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
          • /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/regionserver/RegionScanner.java
          • /hbase/branches/0.94/src/test/java/org/apache/hadoop/hbase/coprocessor/TestCoprocessorInterface.java
          • /hbase/branches/0.94/src/test/java/org/apache/hadoop/hbase/filter/TestFilter.java
          Show
          Hudson added a comment - Integrated in HBase-0.94 #41 (See https://builds.apache.org/job/HBase-0.94/41/ ) HBASE-5520 Support reseek() at RegionScanner (Ram) (Revision 1303006) Result = FAILURE ramkrishna : Files : /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/regionserver/RegionScanner.java /hbase/branches/0.94/src/test/java/org/apache/hadoop/hbase/coprocessor/TestCoprocessorInterface.java /hbase/branches/0.94/src/test/java/org/apache/hadoop/hbase/filter/TestFilter.java
          Hide
          ramkrishna.s.vasudevan added a comment -

          Committed to 0.94 and trunk.
          Thanks Lars, Stack and Anoop for review.
          Special thanks to Lars for taking it into 0.94

          Show
          ramkrishna.s.vasudevan added a comment - Committed to 0.94 and trunk. Thanks Lars, Stack and Anoop for review. Special thanks to Lars for taking it into 0.94
          Hide
          ramkrishna.s.vasudevan added a comment -

          @Lars
          Thanks for reviewing. I will go ahead and commit this now.

          Show
          ramkrishna.s.vasudevan added a comment - @Lars Thanks for reviewing. I will go ahead and commit this now.
          Hide
          Lars Hofhansl added a comment -

          +1 on patch.
          I think this is good for commit.

          Show
          Lars Hofhansl added a comment - +1 on patch. I think this is good for commit.
          Lars Hofhansl made changes -
          Fix Version/s 0.94.0 [ 12316419 ]
          Hide
          Lars Hofhansl added a comment -

          Yeah, I think this should be in 0.94.

          Show
          Lars Hofhansl added a comment - Yeah, I think this should be in 0.94.
          Hide
          ramkrishna.s.vasudevan added a comment -

          @Lars
          Are you considering this for 0.94 RC? If you feel the patch is fine and if you include this in RC it will be of great help for us.
          But even if you feel this cant go in RC then no issues.

          Show
          ramkrishna.s.vasudevan added a comment - @Lars Are you considering this for 0.94 RC? If you feel the patch is fine and if you include this in RC it will be of great help for us. But even if you feel this cant go in RC then no issues.
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12518777/HBASE-5520_4.patch
          against trunk revision .

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

          +1 tests included. The patch appears to include 6 new or modified tests.

          +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 appears to introduce 161 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
          org.apache.hadoop.hbase.master.TestSplitLogManager

          Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/1216//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/1216//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/1216//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/12518777/HBASE-5520_4.patch against trunk revision . +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 6 new or modified tests. +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 appears to introduce 161 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 org.apache.hadoop.hbase.master.TestSplitLogManager Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/1216//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/1216//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/1216//console This message is automatically generated.
          ramkrishna.s.vasudevan made changes -
          Attachment HBASE-5520_4.patch [ 12518777 ]
          Hide
          ramkrishna.s.vasudevan added a comment -

          An udpated patch not w.r.t to the comments given by Lars. In my previous patch some commented lines were present hence removed them.
          Reg, startRegionOperation i go with Anoop's reply.
          Lars, pls share your comments on this.

          Show
          ramkrishna.s.vasudevan added a comment - An udpated patch not w.r.t to the comments given by Lars. In my previous patch some commented lines were present hence removed them. Reg, startRegionOperation i go with Anoop's reply. Lars, pls share your comments on this.
          Hide
          Anoop Sam John added a comment -

          @Lars
          The co processor preScannerNext() method will be getting called from the HRegionServer

          // Call coprocessor. Get region info from scanner.
                HRegion region = getRegion(s.getRegionInfo().getRegionName());
                if (region != null && region.getCoprocessorHost() != null) {
                  Boolean bypass = region.getCoprocessorHost().preScannerNext(s,
                      results, nbRows);
          

          So by the time co processor calls this seek, there wont be any region op already started. Pls correct me if I am wrong.

          Show
          Anoop Sam John added a comment - @Lars The co processor preScannerNext() method will be getting called from the HRegionServer // Call coprocessor. Get region info from scanner. HRegion region = getRegion(s.getRegionInfo().getRegionName()); if (region != null && region.getCoprocessorHost() != null ) { Boolean bypass = region.getCoprocessorHost().preScannerNext(s, results, nbRows); So by the time co processor calls this seek, there wont be any region op already started. Pls correct me if I am wrong.
          Hide
          Lars Hofhansl added a comment -

          +1 on 0.94.
          Another question about the patch:

          +    public synchronized boolean reseek(byte[] row) throws IOException {
          ...
          +      startRegionOperation();
          
          1. why start a region operation here? This is called only called from a coprocessor, right? So should already be in a region operation.
          2. why synchronized?
          Show
          Lars Hofhansl added a comment - +1 on 0.94. Another question about the patch: + public synchronized boolean reseek( byte [] row) throws IOException { ... + startRegionOperation(); why start a region operation here? This is called only called from a coprocessor, right? So should already be in a region operation. why synchronized?
          Hide
          ramkrishna.s.vasudevan added a comment -

          @Lars
          I need to rebase the patch once again related to test case.
          Currently the performance numbers am not able to get it because our clusters are busy with other tasks.
          If it is ok i will rebase the patch now and upload it so that it can be committed.
          If not i will wait to get the results. This can go into 0.94?
          What do you say Lars?

          Show
          ramkrishna.s.vasudevan added a comment - @Lars I need to rebase the patch once again related to test case. Currently the performance numbers am not able to get it because our clusters are busy with other tasks. If it is ok i will rebase the patch now and upload it so that it can be committed. If not i will wait to get the results. This can go into 0.94? What do you say Lars?
          Hide
          Anoop Sam John added a comment -

          @Lars
          Thanks for your thoughts and suggestion.
          Yes we felt that this will be a nice feature for the coprocessor guy..

          Now we support seek to the begin boundary of row.
          Will it be nice to support seek to the end of a given row also.

          Co processor dont know the exact rowid to be seeked... But it know to skip till some rowId.. In that case if we allow to seek to the end boundary of a row , it would be useful.. What do u say?

          In our case we dont have this use case now. But it might be useful for some other co processor guys

          Show
          Anoop Sam John added a comment - @Lars Thanks for your thoughts and suggestion. Yes we felt that this will be a nice feature for the coprocessor guy.. Now we support seek to the begin boundary of row. Will it be nice to support seek to the end of a given row also. Co processor dont know the exact rowid to be seeked... But it know to skip till some rowId.. In that case if we allow to seek to the end boundary of a row , it would be useful.. What do u say? In our case we dont have this use case now. But it might be useful for some other co processor guys
          Hide
          Lars Hofhansl added a comment -

          Have been thinking a bit more about this.

          I think if we allow the caller just to pass a rowkey, and then enforce the we only seek forward this would be a great feature to have.
          Since we're operation at the level of a region scanner (where we think about regions, rather than stores) this would be the right abstraction too.

          Alternatively looking at the MemstoreScanner code, seeking backwards would just be a no-op, so that is another option.

          Sorry, didn't mean to be dis-encouraging before just cautious

          Show
          Lars Hofhansl added a comment - Have been thinking a bit more about this. I think if we allow the caller just to pass a rowkey, and then enforce the we only seek forward this would be a great feature to have. Since we're operation at the level of a region scanner (where we think about regions, rather than stores) this would be the right abstraction too. Alternatively looking at the MemstoreScanner code, seeking backwards would just be a no-op, so that is another option. Sorry, didn't mean to be dis-encouraging before just cautious
          Hide
          ramkrishna.s.vasudevan added a comment -

          @Lars
          We will come up with some performance numbers based on our scenario.

          Thanks Lars and Stack.

          Show
          ramkrishna.s.vasudevan added a comment - @Lars We will come up with some performance numbers based on our scenario. Thanks Lars and Stack.
          Hide
          Lars Hofhansl added a comment -

          Generally it would be very useful for coprocessors to have this ability!
          After looking at the discussion here (the restrictions imposed on what KVs one can passed - previous ones, row only, column family needs to be ignored, etc) we just need to be careful how we add this.

          For this specific issue it would be nice to get some comparative performance numbers between this and filters. Maybe we're missing a level of abstraction...?

          Show
          Lars Hofhansl added a comment - Generally it would be very useful for coprocessors to have this ability! After looking at the discussion here (the restrictions imposed on what KVs one can passed - previous ones, row only, column family needs to be ignored, etc) we just need to be careful how we add this. For this specific issue it would be nice to get some comparative performance numbers between this and filters. Maybe we're missing a level of abstraction...?
          Hide
          stack added a comment -

          We are reseeking to the start of the passed 'row'? Is that what we want? I thought we were trying to go to start of next row.

          If reseeking the start of passed in 'row', then this patch looks fine.

          I defer to Lars's opinion though. I don't know this area of the code well.

          Show
          stack added a comment - We are reseeking to the start of the passed 'row'? Is that what we want? I thought we were trying to go to start of next row. If reseeking the start of passed in 'row', then this patch looks fine. I defer to Lars's opinion though. I don't know this area of the code well.
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12518028/HBASE-5520_3.patch
          against trunk revision .

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

          +1 tests included. The patch appears to include 6 new or modified tests.

          -1 patch. The patch command could not apply the patch.

          Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/1164//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/12518028/HBASE-5520_3.patch against trunk revision . +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 6 new or modified tests. -1 patch. The patch command could not apply the patch. Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/1164//console This message is automatically generated.
          Hide
          ramkrishna.s.vasudevan added a comment -

          @Lars and @Stack
          Updated the api to take row instead of kv.
          @Lars
          Yes SEEK_NEXT_USING_HINT is a definite gain but this reseek will help to avoid that one problem that filters have to reseek one more kv incase where we know the row that we need to seek to.

          Just a thought of me and Anoop. Pls let us know your suggestions and comments.

          Show
          ramkrishna.s.vasudevan added a comment - @Lars and @Stack Updated the api to take row instead of kv. @Lars Yes SEEK_NEXT_USING_HINT is a definite gain but this reseek will help to avoid that one problem that filters have to reseek one more kv incase where we know the row that we need to seek to. Just a thought of me and Anoop. Pls let us know your suggestions and comments.
          ramkrishna.s.vasudevan made changes -
          Attachment HBASE-5520_3.patch [ 12518028 ]
          Hide
          Lars Hofhansl added a comment -

          Agree with stack that the comment can be simplified. Also as stated in HBASE-2038, this reseek only works correctly on row boundaries, so the API should capture this and only accept a rowkey (rather than a KV).

          Then maybe the comment could say something like this:
          "Seek forward to the first KV matching the passed rowkey.
          Only forward seeking is allowed, otherwise the behavior is undefined."

          There might be problems with this approach too. What if the scanner already saw some KVs for this rowkey? It puts a lot of burden on the caller that we only seek forward.

          As an aside:
          Looking back at the various conversations, I actually have not heard a convincing argument on why using a Filter that returns the standard SEEK_NEXT_USING_HINT does not work. It would be doing a little bit more work (because each store would separately need to seek, and it needs to look at at least one more KV to decide to skip ahead), but that would still be huge win over a full scan that needs to look at every KV.
          Filters do not have the above problems, since they are inherently per column family.
          You wouldn't use filterRow() but use filterKeyValue() together with getNextKeyHint()
          (see MultipleColumnPrefixFilter).

          Show
          Lars Hofhansl added a comment - Agree with stack that the comment can be simplified. Also as stated in HBASE-2038 , this reseek only works correctly on row boundaries, so the API should capture this and only accept a rowkey (rather than a KV). Then maybe the comment could say something like this: "Seek forward to the first KV matching the passed rowkey. Only forward seeking is allowed, otherwise the behavior is undefined." There might be problems with this approach too. What if the scanner already saw some KVs for this rowkey? It puts a lot of burden on the caller that we only seek forward. As an aside: Looking back at the various conversations, I actually have not heard a convincing argument on why using a Filter that returns the standard SEEK_NEXT_USING_HINT does not work. It would be doing a little bit more work (because each store would separately need to seek, and it needs to look at at least one more KV to decide to skip ahead), but that would still be huge win over a full scan that needs to look at every KV. Filters do not have the above problems, since they are inherently per column family. You wouldn't use filterRow() but use filterKeyValue() together with getNextKeyHint() (see MultipleColumnPrefixFilter).
          Hide
          ramkrishna.s.vasudevan added a comment -

          @Stack
          If you see the comments above 05/Mar/12 13:30.
          Always allowing to createFirstOnRow kv and createLastOnRow kv will help us to ensure that we are on row boundaries.
          If you see the comments above
          -> Is it ok if we expose the api accepting a row rather than kv.
          -> Or get a kv but still internally create a start key? (We cannot create an end key in this case)

          Show
          ramkrishna.s.vasudevan added a comment - @Stack If you see the comments above 05/Mar/12 13:30. Always allowing to createFirstOnRow kv and createLastOnRow kv will help us to ensure that we are on row boundaries. If you see the comments above -> Is it ok if we expose the api accepting a row rather than kv. -> Or get a kv but still internally create a start key? (We cannot create an end key in this case)
          Hide
          stack added a comment -
          +      if (kv == null) {$
          +        throw new IllegalArgumentException("Row cannot be null.");$
          +      }$
          

          Should be kv cannot be null?

          I don't understand what this means:

          The kv that is required to seek must be given$
          +   * explicitly for reseek. Should not be used to seek to a key which may come$
          +   * before the current position.$
          +   * Note : Recommended to use knowing how seek can be done on different kvs.$
          +   * Suggested to seek to row boundaries like start of a row or end of a row.$
          +   * Seeking to the middle of a row may lead to inconsistencies across stores.$
          

          ... its probably because I'm slow but I'm sure there will be slow fellows following behind me. Is it saying you need to create a KV to do this?

          How would I know what the next row is in advance? I suppose with the mvcc, you have some hope of isolating this row... but if we are going for row boundaries, this patch looks to be missing some heft.

          Show
          stack added a comment - + if (kv == null ) {$ + throw new IllegalArgumentException( "Row cannot be null ." );$ + }$ Should be kv cannot be null? I don't understand what this means: The kv that is required to seek must be given$ + * explicitly for reseek. Should not be used to seek to a key which may come$ + * before the current position.$ + * Note : Recommended to use knowing how seek can be done on different kvs.$ + * Suggested to seek to row boundaries like start of a row or end of a row.$ + * Seeking to the middle of a row may lead to inconsistencies across stores.$ ... its probably because I'm slow but I'm sure there will be slow fellows following behind me. Is it saying you need to create a KV to do this? How would I know what the next row is in advance? I suppose with the mvcc, you have some hope of isolating this row... but if we are going for row boundaries, this patch looks to be missing some heft.
          Hide
          Ted Yu added a comment -

          Compilation failed:

          [ERROR] /home/jenkins/jenkins-slave/workspace/PreCommit-HBASE-Build/trunk/src/test/java/org/apache/hadoop/hbase/coprocessor/TestCoprocessorInterface.java:[54,17] org.apache.hadoop.hbase.coprocessor.TestCoprocessorInterface.CustomScanner is not abstract and does not override abstract method reseek(org.apache.hadoop.hbase.KeyValue) in org.apache.hadoop.hbase.regionserver.RegionScanner
          
          Show
          Ted Yu added a comment - Compilation failed: [ERROR] /home/jenkins/jenkins-slave/workspace/PreCommit-HBASE-Build/trunk/src/test/java/org/apache/hadoop/hbase/coprocessor/TestCoprocessorInterface.java:[54,17] org.apache.hadoop.hbase.coprocessor.TestCoprocessorInterface.CustomScanner is not abstract and does not override abstract method reseek(org.apache.hadoop.hbase.KeyValue) in org.apache.hadoop.hbase.regionserver.RegionScanner
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12517734/HBASE-5520_2.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 -123 warning messages.

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

          -1 findbugs. The patch appears to introduce 159 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/1145//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/1145//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/1145//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/12517734/HBASE-5520_2.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 -123 warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. -1 findbugs. The patch appears to introduce 159 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/1145//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/1145//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/1145//console This message is automatically generated.
          ramkrishna.s.vasudevan made changes -
          Status Open [ 1 ] Patch Available [ 10002 ]
          Fix Version/s 0.96.0 [ 12320040 ]
          ramkrishna.s.vasudevan made changes -
          Attachment HBASE-5520_2.patch [ 12517734 ]
          Hide
          ramkrishna.s.vasudevan added a comment -

          Updated the patch with requestSeek.

          Show
          ramkrishna.s.vasudevan added a comment - Updated the patch with requestSeek.
          Anoop Sam John made changes -
          Summary Support seek() reseek() at RegionScanner Support reseek() at RegionScanner
          Assignee ramkrishna.s.vasudevan [ ram_krish ]
          Description seek() reseek() is not supported currently at the RegionScanner level. We can support the same.
          This is created following the discussion under HBASE-2038
          reseek() is not supported currently at the RegionScanner level. We can support the same.
          This is created following the discussion under HBASE-2038
          Hide
          ramkrishna.s.vasudevan added a comment -

          @Lars
          Thanks for the review. I will check the requestSeek option and get back.

          Show
          ramkrishna.s.vasudevan added a comment - @Lars Thanks for the review. I will check the requestSeek option and get back.
          Hide
          Lars Hofhansl added a comment -

          Didn't realize that KeyValueHeap does have a reseek method already. This makes this much simpler than I thought (so strike my comment in HBASE-2028).
          Do you want to use requestSeek (with forward == true) as opposed to reseek, to make use of the lazy-seek optimization?

          Show
          Lars Hofhansl added a comment - Didn't realize that KeyValueHeap does have a reseek method already. This makes this much simpler than I thought (so strike my comment in HBASE-2028 ). Do you want to use requestSeek (with forward == true) as opposed to reseek, to make use of the lazy-seek optimization?
          Hide
          ramkrishna.s.vasudevan added a comment -

          seek() api if we provide currently the Memstore scanner and store scanner does not allow going back from a current position.
          So only reseek() has been provided. Please provide your comments.

          Show
          ramkrishna.s.vasudevan added a comment - seek() api if we provide currently the Memstore scanner and store scanner does not allow going back from a current position. So only reseek() has been provided. Please provide your comments.
          ramkrishna.s.vasudevan made changes -
          Attachment HBASE-5520_1.patch [ 12517686 ]
          Hide
          ramkrishna.s.vasudevan added a comment -

          Patch for trunk.

          Show
          ramkrishna.s.vasudevan added a comment - Patch for trunk.
          Hide
          ramkrishna.s.vasudevan added a comment -

          We can upload a patch if we are fine with any one of the approaches?

          Please provide your suggestions.

          Show
          ramkrishna.s.vasudevan added a comment - We can upload a patch if we are fine with any one of the approaches? Please provide your suggestions.
          Anoop Sam John made changes -
          Link This issue is required by HBASE-2038 [ HBASE-2038 ]
          Hide
          Anoop Sam John added a comment -

          We can support only seek() and reseek() at the row boundary level.
          We can take any of the below approaches
          1. The APIs make use of the rowkey and timestamp only from the KeyValue passed.
          2. Check at the RegionScannerImpl level that it is not having the CF, qualifier in the passed KV. If so throw exception. Only the KV can have the rowkey and timestamp also.[It is ok.Timestamp can be there...]
          3. Dont bother let the seek happen. But may be dangerous??
          4. We can give the signature of the seek() and reseek() at the RegionScanner as seek( byte[] rowKey ) reseek( byte[] rowKey )? So that the seek will be always to the begin KV of the row in every CF. [ if CF contains that key ]

          Show
          Anoop Sam John added a comment - We can support only seek() and reseek() at the row boundary level. We can take any of the below approaches 1. The APIs make use of the rowkey and timestamp only from the KeyValue passed. 2. Check at the RegionScannerImpl level that it is not having the CF, qualifier in the passed KV. If so throw exception. Only the KV can have the rowkey and timestamp also. [It is ok.Timestamp can be there...] 3. Dont bother let the seek happen. But may be dangerous?? 4. We can give the signature of the seek() and reseek() at the RegionScanner as seek( byte[] rowKey ) reseek( byte[] rowKey )? So that the seek will be always to the begin KV of the row in every CF. [ if CF contains that key ]
          Anoop Sam John made changes -
          Field Original Value New Value
          Description seek() reseek() is not supported currently at the RegionScanner level. We can support the same.
          This is created following the discussion under HBASE_2038
          seek() reseek() is not supported currently at the RegionScanner level. We can support the same.
          This is created following the discussion under HBASE-2038
          Anoop Sam John created issue -

            People

            • Assignee:
              ramkrishna.s.vasudevan
              Reporter:
              Anoop Sam John
            • Votes:
              0 Vote for this issue
              Watchers:
              6 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development