HBase
  1. HBase
  2. HBASE-4197

RegionServer expects all scanner to be subclasses of HRegion.RegionScanner

    Details

    • Type: Bug Bug
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 0.92.0
    • Fix Version/s: 0.92.0
    • Component/s: Coprocessors
    • Labels:
      None
    • Hadoop Flags:
      Incompatible change, Reviewed
    • Release Note:
      RegionScanner is now an interface which extends InternalScanner.

      Description

      Returning just an InternalScanner from RegionObsever.

      {pre|post}

      OpenScanner leads to the following exception when using the scanner.

      java.io.IOException: InternalScanner implementation is expected to be HRegion.RegionScanner.
      at org.apache.hadoop.hbase.regionserver.HRegionServer.next(HRegionServer.java:2023)
      at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
      at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:57)
      at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
      at java.lang.reflect.Method.invoke(Method.java:616)
      at org.apache.hadoop.hbase.ipc.WritableRpcEngine$Server.call(WritableRpcEngine.java:314)
      at org.apache.hadoop.hbase.ipc.HBaseServer$Handler.run(HBaseServer.java:1225)

      The problem is in HRegionServer.next(...):

       
          InternalScanner s = this.scanners.get(scannerName);
      ...
            // Call coprocessor. Get region info from scanner.
            HRegion region = null;
            if (s instanceof HRegion.RegionScanner) {
              HRegion.RegionScanner rs = (HRegion.RegionScanner) s;
              region = getRegion(rs.getRegionName().getRegionName());
            } else {
              throw new IOException("InternalScanner implementation is expected " +
                  "to be HRegion.RegionScanner.");
            }
      
      1. 4197.txt
        8 kB
        Lars Hofhansl
      2. 4197-bigger.txt
        18 kB
        Lars Hofhansl
      3. 4197-test.diff
        3 kB
        Lars Hofhansl
      4. 4197-v2.txt
        20 kB
        Lars Hofhansl
      5. ScannerTest.java
        2 kB
        Lars Hofhansl

        Activity

        Hide
        Lars Hofhansl added a comment -

        I attached a minimal patch that makes it work for me.
        I am not happy with the patch, though, for two reason:
        1. isFilterDone() now needs to be public.
        2. If the regionserver can only ever deal with RegionScanners, maybe all the interfaces in coprocessors should also take RegionScanner instead.

        Show
        Lars Hofhansl added a comment - I attached a minimal patch that makes it work for me. I am not happy with the patch, though, for two reason: 1. isFilterDone() now needs to be public. 2. If the regionserver can only ever deal with RegionScanners, maybe all the interfaces in coprocessors should also take RegionScanner instead.
        Hide
        Lars Hofhansl added a comment -

        And of course if we'll go this route, I'll comments, Copyright notices, etc.

        Show
        Lars Hofhansl added a comment - And of course if we'll go this route, I'll comments, Copyright notices, etc.
        Hide
        Mingjie Lai added a comment -

        @lars

        Yes, RegionScanner'd better to be an interface instead of a class for better extension. Overall the patch looks good to me.

        Can you finish the patch and post it to reviewboard?

        Show
        Mingjie Lai added a comment - @lars Yes, RegionScanner'd better to be an interface instead of a class for better extension. Overall the patch looks good to me. Can you finish the patch and post it to reviewboard?
        Hide
        Lars Hofhansl added a comment -

        Hey Mingjie,

        how do I do that? Is there some documentation where I can read about the process?

        Thanks.

        – Lars

        Show
        Lars Hofhansl added a comment - Hey Mingjie, how do I do that? Is there some documentation where I can read about the process? Thanks. – Lars
        Hide
        Mingjie Lai added a comment -

        Lars.

        I'm not aware of any existing document regarding reviewboard usage for hbase. You need to create an account at https://reviews.apache.org, and create a new review request.

        repository: hbase-git (since I use git)
        group: hbase

        It should be pretty straight forward to use.

        About coding conversion: http://hbase.apache.org/book.html#eclipse

        -mingjie

        Show
        Mingjie Lai added a comment - Lars. I'm not aware of any existing document regarding reviewboard usage for hbase. You need to create an account at https://reviews.apache.org , and create a new review request. repository: hbase-git (since I use git) group: hbase It should be pretty straight forward to use. About coding conversion: http://hbase.apache.org/book.html#eclipse -mingjie
        Hide
        Lars Hofhansl added a comment -

        Thanks.

        I am working on a bigger patch that avoids all the casting and instanceof checking w.r.t. scanners in HRegionServer/etc.

        Show
        Lars Hofhansl added a comment - Thanks. I am working on a bigger patch that avoids all the casting and instanceof checking w.r.t. scanners in HRegionServer/etc.
        Hide
        Lars Hofhansl added a comment -

        Slightly larger patch that does away with all casting and instanceof "nonsense" for scanners.

        Please let me know if you generally agree with the approach, if so I'll get the review started.

        Show
        Lars Hofhansl added a comment - Slightly larger patch that does away with all casting and instanceof "nonsense" for scanners. Please let me know if you generally agree with the approach, if so I'll get the review started.
        Hide
        Ted Yu added a comment -

        I like the cleaner code after the change.
        I know the following existed prior to your patch:

        +    public HRegionInfo getRegionName();
        

        Can we rename the method to getRegionInfo ?
        This would make the following code a little easier to understand:

                region = getRegion(rs.getRegionName().getRegionName());
        

        Thanks for your effort, Lars.

        Show
        Ted Yu added a comment - I like the cleaner code after the change. I know the following existed prior to your patch: + public HRegionInfo getRegionName(); Can we rename the method to getRegionInfo ? This would make the following code a little easier to understand: region = getRegion(rs.getRegionName().getRegionName()); Thanks for your effort, Lars.
        Hide
        Lars Hofhansl added a comment -

        Renamed getRegionName() to getRegionInfo().
        Also cleaned up some more comments, and removed all references to
        InternalScanner from HRegionServer and HRegion (there were only 3 or 4 left anyway).

        Show
        Lars Hofhansl added a comment - Renamed getRegionName() to getRegionInfo(). Also cleaned up some more comments, and removed all references to InternalScanner from HRegionServer and HRegion (there were only 3 or 4 left anyway).
        Hide
        Ted Yu added a comment -

        +1 on patch version 2.
        Please use review board to get more feedback.

        Show
        Ted Yu added a comment - +1 on patch version 2. Please use review board to get more feedback.
        Hide
        Lars Hofhansl added a comment -

        The stripped test I used to narrow down the problem initially.

        Show
        Lars Hofhansl added a comment - The stripped test I used to narrow down the problem initially.
        Hide
        jiraposter@reviews.apache.org added a comment -

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/1496/
        -----------------------------------------------------------

        Review request for Ted Yu and Mingjie Lai.

        Summary
        -------

        1. Don't require custom scanners created by conprocessors to be subclasses of HRegion.RegionScanner (see HBASE-4197).
        2. Simplify the interfaces for Scanners in HRegion, HRegionServer, and RegionObserver. This avoids a bunch instanceof checks and casts to HRegion.RegionScanner.

        (Sorry HBase-git would accept my patch)

        This addresses bug HBASE-4197.
        https://issues.apache.org/jira/browse/HBASE-4197

        Diffs


        http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/coprocessor/BaseRegionObserver.java 1157311
        http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/coprocessor/RegionObserver.java 1157311
        http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java 1157311
        http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java 1157311
        http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/RegionCoprocessorHost.java 1157311
        http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/RegionScanner.java PRE-CREATION
        http://svn.apache.org/repos/asf/hbase/trunk/src/test/java/org/apache/hadoop/hbase/coprocessor/SimpleRegionObserver.java 1157311
        http://svn.apache.org/repos/asf/hbase/trunk/src/test/java/org/apache/hadoop/hbase/regionserver/TestHRegion.java 1157311
        http://svn.apache.org/repos/asf/hbase/trunk/src/test/java/org/apache/hadoop/hbase/regionserver/TestWideScanner.java 1157311

        Diff: https://reviews.apache.org/r/1496/diff

        Testing
        -------

        Manual test attached to the bug.

        Thanks,

        Lars

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/1496/ ----------------------------------------------------------- Review request for Ted Yu and Mingjie Lai. Summary ------- 1. Don't require custom scanners created by conprocessors to be subclasses of HRegion.RegionScanner (see HBASE-4197 ). 2. Simplify the interfaces for Scanners in HRegion, HRegionServer, and RegionObserver. This avoids a bunch instanceof checks and casts to HRegion.RegionScanner. (Sorry HBase-git would accept my patch) This addresses bug HBASE-4197 . https://issues.apache.org/jira/browse/HBASE-4197 Diffs http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/coprocessor/BaseRegionObserver.java 1157311 http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/coprocessor/RegionObserver.java 1157311 http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java 1157311 http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java 1157311 http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/RegionCoprocessorHost.java 1157311 http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/RegionScanner.java PRE-CREATION http://svn.apache.org/repos/asf/hbase/trunk/src/test/java/org/apache/hadoop/hbase/coprocessor/SimpleRegionObserver.java 1157311 http://svn.apache.org/repos/asf/hbase/trunk/src/test/java/org/apache/hadoop/hbase/regionserver/TestHRegion.java 1157311 http://svn.apache.org/repos/asf/hbase/trunk/src/test/java/org/apache/hadoop/hbase/regionserver/TestWideScanner.java 1157311 Diff: https://reviews.apache.org/r/1496/diff Testing ------- Manual test attached to the bug. Thanks, Lars
        Hide
        jiraposter@reviews.apache.org added a comment -

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/1496/
        -----------------------------------------------------------

        (Updated 2011-08-13 04:38:38.030763)

        Review request for Ted Yu and Mingjie Lai.

        Summary (updated)
        -------

        1. Don't require custom scanners created by conprocessors to be subclasses of HRegion.RegionScanner (see HBASE-4197).
        2. Simplify the interfaces for Scanners in HRegion, HRegionServer, and RegionObserver. This avoids a bunch instanceof checks and casts to HRegion.RegionScanner.

        (Sorry HBase-git would not accept my patch)

        This addresses bug HBASE-4197.
        https://issues.apache.org/jira/browse/HBASE-4197

        Diffs


        http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/coprocessor/BaseRegionObserver.java 1157311
        http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/coprocessor/RegionObserver.java 1157311
        http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java 1157311
        http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java 1157311
        http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/RegionCoprocessorHost.java 1157311
        http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/RegionScanner.java PRE-CREATION
        http://svn.apache.org/repos/asf/hbase/trunk/src/test/java/org/apache/hadoop/hbase/coprocessor/SimpleRegionObserver.java 1157311
        http://svn.apache.org/repos/asf/hbase/trunk/src/test/java/org/apache/hadoop/hbase/regionserver/TestHRegion.java 1157311
        http://svn.apache.org/repos/asf/hbase/trunk/src/test/java/org/apache/hadoop/hbase/regionserver/TestWideScanner.java 1157311

        Diff: https://reviews.apache.org/r/1496/diff

        Testing
        -------

        Manual test attached to the bug.

        Thanks,

        Lars

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/1496/ ----------------------------------------------------------- (Updated 2011-08-13 04:38:38.030763) Review request for Ted Yu and Mingjie Lai. Summary (updated) ------- 1. Don't require custom scanners created by conprocessors to be subclasses of HRegion.RegionScanner (see HBASE-4197 ). 2. Simplify the interfaces for Scanners in HRegion, HRegionServer, and RegionObserver. This avoids a bunch instanceof checks and casts to HRegion.RegionScanner. (Sorry HBase-git would not accept my patch) This addresses bug HBASE-4197 . https://issues.apache.org/jira/browse/HBASE-4197 Diffs http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/coprocessor/BaseRegionObserver.java 1157311 http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/coprocessor/RegionObserver.java 1157311 http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java 1157311 http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java 1157311 http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/RegionCoprocessorHost.java 1157311 http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/RegionScanner.java PRE-CREATION http://svn.apache.org/repos/asf/hbase/trunk/src/test/java/org/apache/hadoop/hbase/coprocessor/SimpleRegionObserver.java 1157311 http://svn.apache.org/repos/asf/hbase/trunk/src/test/java/org/apache/hadoop/hbase/regionserver/TestHRegion.java 1157311 http://svn.apache.org/repos/asf/hbase/trunk/src/test/java/org/apache/hadoop/hbase/regionserver/TestWideScanner.java 1157311 Diff: https://reviews.apache.org/r/1496/diff Testing ------- Manual test attached to the bug. Thanks, Lars
        Hide
        Ted Yu added a comment -

        Please specify hbase for Groups on review board so that more people can see the review request.
        Good job Lars.

        Show
        Ted Yu added a comment - Please specify hbase for Groups on review board so that more people can see the review request. Good job Lars.
        Hide
        jiraposter@reviews.apache.org added a comment -

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/1496/
        -----------------------------------------------------------

        (Updated 2011-08-13 15:56:28.746919)

        Review request for hbase, Ted Yu and Mingjie Lai.

        Summary
        -------

        1. Don't require custom scanners created by conprocessors to be subclasses of HRegion.RegionScanner (see HBASE-4197).
        2. Simplify the interfaces for Scanners in HRegion, HRegionServer, and RegionObserver. This avoids a bunch instanceof checks and casts to HRegion.RegionScanner.

        (Sorry HBase-git would not accept my patch)

        This addresses bug HBASE-4197.
        https://issues.apache.org/jira/browse/HBASE-4197

        Diffs


        http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/coprocessor/BaseRegionObserver.java 1157311
        http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/coprocessor/RegionObserver.java 1157311
        http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java 1157311
        http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java 1157311
        http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/RegionCoprocessorHost.java 1157311
        http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/RegionScanner.java PRE-CREATION
        http://svn.apache.org/repos/asf/hbase/trunk/src/test/java/org/apache/hadoop/hbase/coprocessor/SimpleRegionObserver.java 1157311
        http://svn.apache.org/repos/asf/hbase/trunk/src/test/java/org/apache/hadoop/hbase/regionserver/TestHRegion.java 1157311
        http://svn.apache.org/repos/asf/hbase/trunk/src/test/java/org/apache/hadoop/hbase/regionserver/TestWideScanner.java 1157311

        Diff: https://reviews.apache.org/r/1496/diff

        Testing
        -------

        Manual test attached to the bug.

        Thanks,

        Lars

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/1496/ ----------------------------------------------------------- (Updated 2011-08-13 15:56:28.746919) Review request for hbase, Ted Yu and Mingjie Lai. Summary ------- 1. Don't require custom scanners created by conprocessors to be subclasses of HRegion.RegionScanner (see HBASE-4197 ). 2. Simplify the interfaces for Scanners in HRegion, HRegionServer, and RegionObserver. This avoids a bunch instanceof checks and casts to HRegion.RegionScanner. (Sorry HBase-git would not accept my patch) This addresses bug HBASE-4197 . https://issues.apache.org/jira/browse/HBASE-4197 Diffs http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/coprocessor/BaseRegionObserver.java 1157311 http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/coprocessor/RegionObserver.java 1157311 http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java 1157311 http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java 1157311 http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/RegionCoprocessorHost.java 1157311 http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/RegionScanner.java PRE-CREATION http://svn.apache.org/repos/asf/hbase/trunk/src/test/java/org/apache/hadoop/hbase/coprocessor/SimpleRegionObserver.java 1157311 http://svn.apache.org/repos/asf/hbase/trunk/src/test/java/org/apache/hadoop/hbase/regionserver/TestHRegion.java 1157311 http://svn.apache.org/repos/asf/hbase/trunk/src/test/java/org/apache/hadoop/hbase/regionserver/TestWideScanner.java 1157311 Diff: https://reviews.apache.org/r/1496/diff Testing ------- Manual test attached to the bug. Thanks, Lars
        Hide
        jiraposter@reviews.apache.org added a comment -

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/1496/#review1446
        -----------------------------------------------------------

        http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/RegionScanner.java
        <https://reviews.apache.org/r/1496/#comment3351>

        Tab should be 2 spaces.

        • Ted

        On 2011-08-13 15:56:28, Lars Hofhansl wrote:

        -----------------------------------------------------------

        This is an automatically generated e-mail. To reply, visit:

        https://reviews.apache.org/r/1496/

        -----------------------------------------------------------

        (Updated 2011-08-13 15:56:28)

        Review request for hbase, Ted Yu and Mingjie Lai.

        Summary

        -------

        1. Don't require custom scanners created by conprocessors to be subclasses of HRegion.RegionScanner (see HBASE-4197).

        2. Simplify the interfaces for Scanners in HRegion, HRegionServer, and RegionObserver. This avoids a bunch instanceof checks and casts to HRegion.RegionScanner.

        (Sorry HBase-git would not accept my patch)

        This addresses bug HBASE-4197.

        https://issues.apache.org/jira/browse/HBASE-4197

        Diffs

        -----

        http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/coprocessor/BaseRegionObserver.java 1157311

        http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/coprocessor/RegionObserver.java 1157311

        http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java 1157311

        http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java 1157311

        http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/RegionCoprocessorHost.java 1157311

        http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/RegionScanner.java PRE-CREATION

        http://svn.apache.org/repos/asf/hbase/trunk/src/test/java/org/apache/hadoop/hbase/coprocessor/SimpleRegionObserver.java 1157311

        http://svn.apache.org/repos/asf/hbase/trunk/src/test/java/org/apache/hadoop/hbase/regionserver/TestHRegion.java 1157311

        http://svn.apache.org/repos/asf/hbase/trunk/src/test/java/org/apache/hadoop/hbase/regionserver/TestWideScanner.java 1157311

        Diff: https://reviews.apache.org/r/1496/diff

        Testing

        -------

        Manual test attached to the bug.

        Thanks,

        Lars

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/1496/#review1446 ----------------------------------------------------------- http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/RegionScanner.java < https://reviews.apache.org/r/1496/#comment3351 > Tab should be 2 spaces. Ted On 2011-08-13 15:56:28, Lars Hofhansl wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/1496/ ----------------------------------------------------------- (Updated 2011-08-13 15:56:28) Review request for hbase, Ted Yu and Mingjie Lai. Summary ------- 1. Don't require custom scanners created by conprocessors to be subclasses of HRegion.RegionScanner (see HBASE-4197 ). 2. Simplify the interfaces for Scanners in HRegion, HRegionServer, and RegionObserver. This avoids a bunch instanceof checks and casts to HRegion.RegionScanner. (Sorry HBase-git would not accept my patch) This addresses bug HBASE-4197 . https://issues.apache.org/jira/browse/HBASE-4197 Diffs ----- http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/coprocessor/BaseRegionObserver.java 1157311 http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/coprocessor/RegionObserver.java 1157311 http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java 1157311 http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java 1157311 http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/RegionCoprocessorHost.java 1157311 http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/RegionScanner.java PRE-CREATION http://svn.apache.org/repos/asf/hbase/trunk/src/test/java/org/apache/hadoop/hbase/coprocessor/SimpleRegionObserver.java 1157311 http://svn.apache.org/repos/asf/hbase/trunk/src/test/java/org/apache/hadoop/hbase/regionserver/TestHRegion.java 1157311 http://svn.apache.org/repos/asf/hbase/trunk/src/test/java/org/apache/hadoop/hbase/regionserver/TestWideScanner.java 1157311 Diff: https://reviews.apache.org/r/1496/diff Testing ------- Manual test attached to the bug. Thanks, Lars
        Hide
        Ted Yu added a comment -

        See http://hbase.apache.org/book.html#eclipse which would tell you where to find formatter for Eclipse.

        Show
        Ted Yu added a comment - See http://hbase.apache.org/book.html#eclipse which would tell you where to find formatter for Eclipse.
        Hide
        Lars Hofhansl added a comment -

        Thanks Ted... Installed the formatter. I will wait for some more feedback and then upload a new version.

        Show
        Lars Hofhansl added a comment - Thanks Ted... Installed the formatter. I will wait for some more feedback and then upload a new version.
        Hide
        jiraposter@reviews.apache.org added a comment -

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/1496/#review1448
        -----------------------------------------------------------

        http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/coprocessor/RegionObserver.java
        <https://reviews.apache.org/r/1496/#comment3353>

        I need to change the javadoc to say RegionScanner as well.

        • Lars

        On 2011-08-13 15:56:28, Lars Hofhansl wrote:

        -----------------------------------------------------------

        This is an automatically generated e-mail. To reply, visit:

        https://reviews.apache.org/r/1496/

        -----------------------------------------------------------

        (Updated 2011-08-13 15:56:28)

        Review request for hbase, Ted Yu and Mingjie Lai.

        Summary

        -------

        1. Don't require custom scanners created by conprocessors to be subclasses of HRegion.RegionScanner (see HBASE-4197).

        2. Simplify the interfaces for Scanners in HRegion, HRegionServer, and RegionObserver. This avoids a bunch instanceof checks and casts to HRegion.RegionScanner.

        (Sorry HBase-git would not accept my patch)

        This addresses bug HBASE-4197.

        https://issues.apache.org/jira/browse/HBASE-4197

        Diffs

        -----

        http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/coprocessor/BaseRegionObserver.java 1157311

        http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/coprocessor/RegionObserver.java 1157311

        http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java 1157311

        http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java 1157311

        http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/RegionCoprocessorHost.java 1157311

        http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/RegionScanner.java PRE-CREATION

        http://svn.apache.org/repos/asf/hbase/trunk/src/test/java/org/apache/hadoop/hbase/coprocessor/SimpleRegionObserver.java 1157311

        http://svn.apache.org/repos/asf/hbase/trunk/src/test/java/org/apache/hadoop/hbase/regionserver/TestHRegion.java 1157311

        http://svn.apache.org/repos/asf/hbase/trunk/src/test/java/org/apache/hadoop/hbase/regionserver/TestWideScanner.java 1157311

        Diff: https://reviews.apache.org/r/1496/diff

        Testing

        -------

        Manual test attached to the bug.

        Thanks,

        Lars

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/1496/#review1448 ----------------------------------------------------------- http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/coprocessor/RegionObserver.java < https://reviews.apache.org/r/1496/#comment3353 > I need to change the javadoc to say RegionScanner as well. Lars On 2011-08-13 15:56:28, Lars Hofhansl wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/1496/ ----------------------------------------------------------- (Updated 2011-08-13 15:56:28) Review request for hbase, Ted Yu and Mingjie Lai. Summary ------- 1. Don't require custom scanners created by conprocessors to be subclasses of HRegion.RegionScanner (see HBASE-4197 ). 2. Simplify the interfaces for Scanners in HRegion, HRegionServer, and RegionObserver. This avoids a bunch instanceof checks and casts to HRegion.RegionScanner. (Sorry HBase-git would not accept my patch) This addresses bug HBASE-4197 . https://issues.apache.org/jira/browse/HBASE-4197 Diffs ----- http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/coprocessor/BaseRegionObserver.java 1157311 http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/coprocessor/RegionObserver.java 1157311 http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java 1157311 http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java 1157311 http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/RegionCoprocessorHost.java 1157311 http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/RegionScanner.java PRE-CREATION http://svn.apache.org/repos/asf/hbase/trunk/src/test/java/org/apache/hadoop/hbase/coprocessor/SimpleRegionObserver.java 1157311 http://svn.apache.org/repos/asf/hbase/trunk/src/test/java/org/apache/hadoop/hbase/regionserver/TestHRegion.java 1157311 http://svn.apache.org/repos/asf/hbase/trunk/src/test/java/org/apache/hadoop/hbase/regionserver/TestWideScanner.java 1157311 Diff: https://reviews.apache.org/r/1496/diff Testing ------- Manual test attached to the bug. Thanks, Lars
        Hide
        jiraposter@reviews.apache.org added a comment -

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/1496/
        -----------------------------------------------------------

        (Updated 2011-08-14 18:39:53.875778)

        Review request for hbase, Ted Yu and Mingjie Lai.

        Summary
        -------

        1. Don't require custom scanners created by conprocessors to be subclasses of HRegion.RegionScanner (see HBASE-4197).
        2. Simplify the interfaces for Scanners in HRegion, HRegionServer, and RegionObserver. This avoids a bunch instanceof checks and casts to HRegion.RegionScanner.

        (Sorry HBase-git would not accept my patch)

        This addresses bug HBASE-4197.
        https://issues.apache.org/jira/browse/HBASE-4197

        Diffs (updated)


        http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/coprocessor/BaseRegionObserver.java 1157388
        http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/coprocessor/RegionObserver.java 1157388
        http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java 1157388
        http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java 1157388
        http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/RegionCoprocessorHost.java 1157388
        http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/RegionScanner.java PRE-CREATION
        http://svn.apache.org/repos/asf/hbase/trunk/src/test/java/org/apache/hadoop/hbase/coprocessor/SimpleRegionObserver.java 1157388
        http://svn.apache.org/repos/asf/hbase/trunk/src/test/java/org/apache/hadoop/hbase/regionserver/TestHRegion.java 1157388
        http://svn.apache.org/repos/asf/hbase/trunk/src/test/java/org/apache/hadoop/hbase/regionserver/TestWideScanner.java 1157388

        Diff: https://reviews.apache.org/r/1496/diff

        Testing
        -------

        Manual test attached to the bug.

        Thanks,

        Lars

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/1496/ ----------------------------------------------------------- (Updated 2011-08-14 18:39:53.875778) Review request for hbase, Ted Yu and Mingjie Lai. Summary ------- 1. Don't require custom scanners created by conprocessors to be subclasses of HRegion.RegionScanner (see HBASE-4197 ). 2. Simplify the interfaces for Scanners in HRegion, HRegionServer, and RegionObserver. This avoids a bunch instanceof checks and casts to HRegion.RegionScanner. (Sorry HBase-git would not accept my patch) This addresses bug HBASE-4197 . https://issues.apache.org/jira/browse/HBASE-4197 Diffs (updated) http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/coprocessor/BaseRegionObserver.java 1157388 http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/coprocessor/RegionObserver.java 1157388 http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java 1157388 http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java 1157388 http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/RegionCoprocessorHost.java 1157388 http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/RegionScanner.java PRE-CREATION http://svn.apache.org/repos/asf/hbase/trunk/src/test/java/org/apache/hadoop/hbase/coprocessor/SimpleRegionObserver.java 1157388 http://svn.apache.org/repos/asf/hbase/trunk/src/test/java/org/apache/hadoop/hbase/regionserver/TestHRegion.java 1157388 http://svn.apache.org/repos/asf/hbase/trunk/src/test/java/org/apache/hadoop/hbase/regionserver/TestWideScanner.java 1157388 Diff: https://reviews.apache.org/r/1496/diff Testing ------- Manual test attached to the bug. Thanks, Lars
        Hide
        jiraposter@reviews.apache.org added a comment -

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/1496/#review1449
        -----------------------------------------------------------

        Ship it!

        • Ted

        On 2011-08-14 18:39:53, Lars Hofhansl wrote:

        -----------------------------------------------------------

        This is an automatically generated e-mail. To reply, visit:

        https://reviews.apache.org/r/1496/

        -----------------------------------------------------------

        (Updated 2011-08-14 18:39:53)

        Review request for hbase, Ted Yu and Mingjie Lai.

        Summary

        -------

        1. Don't require custom scanners created by conprocessors to be subclasses of HRegion.RegionScanner (see HBASE-4197).

        2. Simplify the interfaces for Scanners in HRegion, HRegionServer, and RegionObserver. This avoids a bunch instanceof checks and casts to HRegion.RegionScanner.

        (Sorry HBase-git would not accept my patch)

        This addresses bug HBASE-4197.

        https://issues.apache.org/jira/browse/HBASE-4197

        Diffs

        -----

        http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/coprocessor/BaseRegionObserver.java 1157388

        http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/coprocessor/RegionObserver.java 1157388

        http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java 1157388

        http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java 1157388

        http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/RegionCoprocessorHost.java 1157388

        http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/RegionScanner.java PRE-CREATION

        http://svn.apache.org/repos/asf/hbase/trunk/src/test/java/org/apache/hadoop/hbase/coprocessor/SimpleRegionObserver.java 1157388

        http://svn.apache.org/repos/asf/hbase/trunk/src/test/java/org/apache/hadoop/hbase/regionserver/TestHRegion.java 1157388

        http://svn.apache.org/repos/asf/hbase/trunk/src/test/java/org/apache/hadoop/hbase/regionserver/TestWideScanner.java 1157388

        Diff: https://reviews.apache.org/r/1496/diff

        Testing

        -------

        Manual test attached to the bug.

        Thanks,

        Lars

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/1496/#review1449 ----------------------------------------------------------- Ship it! Ted On 2011-08-14 18:39:53, Lars Hofhansl wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/1496/ ----------------------------------------------------------- (Updated 2011-08-14 18:39:53) Review request for hbase, Ted Yu and Mingjie Lai. Summary ------- 1. Don't require custom scanners created by conprocessors to be subclasses of HRegion.RegionScanner (see HBASE-4197 ). 2. Simplify the interfaces for Scanners in HRegion, HRegionServer, and RegionObserver. This avoids a bunch instanceof checks and casts to HRegion.RegionScanner. (Sorry HBase-git would not accept my patch) This addresses bug HBASE-4197 . https://issues.apache.org/jira/browse/HBASE-4197 Diffs ----- http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/coprocessor/BaseRegionObserver.java 1157388 http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/coprocessor/RegionObserver.java 1157388 http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java 1157388 http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java 1157388 http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/RegionCoprocessorHost.java 1157388 http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/RegionScanner.java PRE-CREATION http://svn.apache.org/repos/asf/hbase/trunk/src/test/java/org/apache/hadoop/hbase/coprocessor/SimpleRegionObserver.java 1157388 http://svn.apache.org/repos/asf/hbase/trunk/src/test/java/org/apache/hadoop/hbase/regionserver/TestHRegion.java 1157388 http://svn.apache.org/repos/asf/hbase/trunk/src/test/java/org/apache/hadoop/hbase/regionserver/TestWideScanner.java 1157388 Diff: https://reviews.apache.org/r/1496/diff Testing ------- Manual test attached to the bug. Thanks, Lars
        Hide
        Lars Hofhansl added a comment -

        Yeah!

        @Ted: Will you commit the patch attached to the review?

        Note that this changes the externally visible interface of Coprocessors (RegionScanner instead of InternalScanner)... Just calling this out, so that there are no surprises later.

        Show
        Lars Hofhansl added a comment - Yeah! @Ted: Will you commit the patch attached to the review? Note that this changes the externally visible interface of Coprocessors (RegionScanner instead of InternalScanner)... Just calling this out, so that there are no surprises later.
        Hide
        jiraposter@reviews.apache.org added a comment -

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/1496/
        -----------------------------------------------------------

        (Updated 2011-08-15 21:21:59.072725)

        Review request for hbase, Ted Yu and Mingjie Lai.

        Summary (updated)
        -------

        1. Don't require custom scanners created by coprocessors to be subclasses of HRegion.RegionScanner (see HBASE-4197).
        2. Simplify the interfaces for Scanners in HRegion, HRegionServer, and RegionObserver. This avoids a bunch instanceof checks and casts to HRegion.RegionScanner.

        (Sorry HBase-git would not accept my patch)

        This addresses bug HBASE-4197.
        https://issues.apache.org/jira/browse/HBASE-4197

        Diffs


        http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/coprocessor/BaseRegionObserver.java 1157388
        http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/coprocessor/RegionObserver.java 1157388
        http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java 1157388
        http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java 1157388
        http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/RegionCoprocessorHost.java 1157388
        http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/RegionScanner.java PRE-CREATION
        http://svn.apache.org/repos/asf/hbase/trunk/src/test/java/org/apache/hadoop/hbase/coprocessor/SimpleRegionObserver.java 1157388
        http://svn.apache.org/repos/asf/hbase/trunk/src/test/java/org/apache/hadoop/hbase/regionserver/TestHRegion.java 1157388
        http://svn.apache.org/repos/asf/hbase/trunk/src/test/java/org/apache/hadoop/hbase/regionserver/TestWideScanner.java 1157388

        Diff: https://reviews.apache.org/r/1496/diff

        Testing
        -------

        Manual test attached to the bug.

        Thanks,

        Lars

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/1496/ ----------------------------------------------------------- (Updated 2011-08-15 21:21:59.072725) Review request for hbase, Ted Yu and Mingjie Lai. Summary (updated) ------- 1. Don't require custom scanners created by coprocessors to be subclasses of HRegion.RegionScanner (see HBASE-4197 ). 2. Simplify the interfaces for Scanners in HRegion, HRegionServer, and RegionObserver. This avoids a bunch instanceof checks and casts to HRegion.RegionScanner. (Sorry HBase-git would not accept my patch) This addresses bug HBASE-4197 . https://issues.apache.org/jira/browse/HBASE-4197 Diffs http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/coprocessor/BaseRegionObserver.java 1157388 http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/coprocessor/RegionObserver.java 1157388 http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java 1157388 http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java 1157388 http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/RegionCoprocessorHost.java 1157388 http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/RegionScanner.java PRE-CREATION http://svn.apache.org/repos/asf/hbase/trunk/src/test/java/org/apache/hadoop/hbase/coprocessor/SimpleRegionObserver.java 1157388 http://svn.apache.org/repos/asf/hbase/trunk/src/test/java/org/apache/hadoop/hbase/regionserver/TestHRegion.java 1157388 http://svn.apache.org/repos/asf/hbase/trunk/src/test/java/org/apache/hadoop/hbase/regionserver/TestWideScanner.java 1157388 Diff: https://reviews.apache.org/r/1496/diff Testing ------- Manual test attached to the bug. Thanks, Lars
        Hide
        jiraposter@reviews.apache.org added a comment -

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/1496/#review1459
        -----------------------------------------------------------

        I saw you have a test case posted to jira as attachment – ScannerTest.java. Are you gonna make it part of this patch?

        The patch looks good to me.

        http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/RegionScanner.java
        <https://reviews.apache.org/r/1496/#comment3365>

        This line has > 80 characters. Can you wrap it?

        • Mingjie

        On 2011-08-15 21:21:59, Lars Hofhansl wrote:

        -----------------------------------------------------------

        This is an automatically generated e-mail. To reply, visit:

        https://reviews.apache.org/r/1496/

        -----------------------------------------------------------

        (Updated 2011-08-15 21:21:59)

        Review request for hbase, Ted Yu and Mingjie Lai.

        Summary

        -------

        1. Don't require custom scanners created by coprocessors to be subclasses of HRegion.RegionScanner (see HBASE-4197).

        2. Simplify the interfaces for Scanners in HRegion, HRegionServer, and RegionObserver. This avoids a bunch instanceof checks and casts to HRegion.RegionScanner.

        (Sorry HBase-git would not accept my patch)

        This addresses bug HBASE-4197.

        https://issues.apache.org/jira/browse/HBASE-4197

        Diffs

        -----

        http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/coprocessor/BaseRegionObserver.java 1157388

        http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/coprocessor/RegionObserver.java 1157388

        http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java 1157388

        http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java 1157388

        http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/RegionCoprocessorHost.java 1157388

        http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/RegionScanner.java PRE-CREATION

        http://svn.apache.org/repos/asf/hbase/trunk/src/test/java/org/apache/hadoop/hbase/coprocessor/SimpleRegionObserver.java 1157388

        http://svn.apache.org/repos/asf/hbase/trunk/src/test/java/org/apache/hadoop/hbase/regionserver/TestHRegion.java 1157388

        http://svn.apache.org/repos/asf/hbase/trunk/src/test/java/org/apache/hadoop/hbase/regionserver/TestWideScanner.java 1157388

        Diff: https://reviews.apache.org/r/1496/diff

        Testing

        -------

        Manual test attached to the bug.

        Thanks,

        Lars

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/1496/#review1459 ----------------------------------------------------------- I saw you have a test case posted to jira as attachment – ScannerTest.java. Are you gonna make it part of this patch? The patch looks good to me. http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/RegionScanner.java < https://reviews.apache.org/r/1496/#comment3365 > This line has > 80 characters. Can you wrap it? Mingjie On 2011-08-15 21:21:59, Lars Hofhansl wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/1496/ ----------------------------------------------------------- (Updated 2011-08-15 21:21:59) Review request for hbase, Ted Yu and Mingjie Lai. Summary ------- 1. Don't require custom scanners created by coprocessors to be subclasses of HRegion.RegionScanner (see HBASE-4197 ). 2. Simplify the interfaces for Scanners in HRegion, HRegionServer, and RegionObserver. This avoids a bunch instanceof checks and casts to HRegion.RegionScanner. (Sorry HBase-git would not accept my patch) This addresses bug HBASE-4197 . https://issues.apache.org/jira/browse/HBASE-4197 Diffs ----- http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/coprocessor/BaseRegionObserver.java 1157388 http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/coprocessor/RegionObserver.java 1157388 http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java 1157388 http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java 1157388 http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/RegionCoprocessorHost.java 1157388 http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/RegionScanner.java PRE-CREATION http://svn.apache.org/repos/asf/hbase/trunk/src/test/java/org/apache/hadoop/hbase/coprocessor/SimpleRegionObserver.java 1157388 http://svn.apache.org/repos/asf/hbase/trunk/src/test/java/org/apache/hadoop/hbase/regionserver/TestHRegion.java 1157388 http://svn.apache.org/repos/asf/hbase/trunk/src/test/java/org/apache/hadoop/hbase/regionserver/TestWideScanner.java 1157388 Diff: https://reviews.apache.org/r/1496/diff Testing ------- Manual test attached to the bug. Thanks, Lars
        Hide
        jiraposter@reviews.apache.org added a comment -

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/1496/#review1460
        -----------------------------------------------------------

        Ship it!

        • Andrew

        On 2011-08-15 21:21:59, Lars Hofhansl wrote:

        -----------------------------------------------------------

        This is an automatically generated e-mail. To reply, visit:

        https://reviews.apache.org/r/1496/

        -----------------------------------------------------------

        (Updated 2011-08-15 21:21:59)

        Review request for hbase, Ted Yu and Mingjie Lai.

        Summary

        -------

        1. Don't require custom scanners created by coprocessors to be subclasses of HRegion.RegionScanner (see HBASE-4197).

        2. Simplify the interfaces for Scanners in HRegion, HRegionServer, and RegionObserver. This avoids a bunch instanceof checks and casts to HRegion.RegionScanner.

        (Sorry HBase-git would not accept my patch)

        This addresses bug HBASE-4197.

        https://issues.apache.org/jira/browse/HBASE-4197

        Diffs

        -----

        http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/coprocessor/BaseRegionObserver.java 1157388

        http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/coprocessor/RegionObserver.java 1157388

        http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java 1157388

        http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java 1157388

        http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/RegionCoprocessorHost.java 1157388

        http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/RegionScanner.java PRE-CREATION

        http://svn.apache.org/repos/asf/hbase/trunk/src/test/java/org/apache/hadoop/hbase/coprocessor/SimpleRegionObserver.java 1157388

        http://svn.apache.org/repos/asf/hbase/trunk/src/test/java/org/apache/hadoop/hbase/regionserver/TestHRegion.java 1157388

        http://svn.apache.org/repos/asf/hbase/trunk/src/test/java/org/apache/hadoop/hbase/regionserver/TestWideScanner.java 1157388

        Diff: https://reviews.apache.org/r/1496/diff

        Testing

        -------

        Manual test attached to the bug.

        Thanks,

        Lars

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/1496/#review1460 ----------------------------------------------------------- Ship it! Andrew On 2011-08-15 21:21:59, Lars Hofhansl wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/1496/ ----------------------------------------------------------- (Updated 2011-08-15 21:21:59) Review request for hbase, Ted Yu and Mingjie Lai. Summary ------- 1. Don't require custom scanners created by coprocessors to be subclasses of HRegion.RegionScanner (see HBASE-4197 ). 2. Simplify the interfaces for Scanners in HRegion, HRegionServer, and RegionObserver. This avoids a bunch instanceof checks and casts to HRegion.RegionScanner. (Sorry HBase-git would not accept my patch) This addresses bug HBASE-4197 . https://issues.apache.org/jira/browse/HBASE-4197 Diffs ----- http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/coprocessor/BaseRegionObserver.java 1157388 http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/coprocessor/RegionObserver.java 1157388 http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java 1157388 http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java 1157388 http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/RegionCoprocessorHost.java 1157388 http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/RegionScanner.java PRE-CREATION http://svn.apache.org/repos/asf/hbase/trunk/src/test/java/org/apache/hadoop/hbase/coprocessor/SimpleRegionObserver.java 1157388 http://svn.apache.org/repos/asf/hbase/trunk/src/test/java/org/apache/hadoop/hbase/regionserver/TestHRegion.java 1157388 http://svn.apache.org/repos/asf/hbase/trunk/src/test/java/org/apache/hadoop/hbase/regionserver/TestWideScanner.java 1157388 Diff: https://reviews.apache.org/r/1496/diff Testing ------- Manual test attached to the bug. Thanks, Lars
        Hide
        Ted Yu added a comment -

        Integrated to TRUNK.
        I wrapped the long line in RegionScanner.java

        Thanks for the patch Lars.
        Thanks for the review Andy and Mingjie.

        Show
        Ted Yu added a comment - Integrated to TRUNK. I wrapped the long line in RegionScanner.java Thanks for the patch Lars. Thanks for the review Andy and Mingjie.
        Hide
        Lars Hofhansl added a comment -

        Thanks Ted!

        Here an additional diff for a simple test.
        Passes locally.

        Show
        Lars Hofhansl added a comment - Thanks Ted! Here an additional diff for a simple test. Passes locally.
        Hide
        Ted Yu added a comment -

        It passed for me as well.
        Integrated to TRUNK.

        Show
        Ted Yu added a comment - It passed for me as well. Integrated to TRUNK.
        Hide
        Hudson added a comment -

        Integrated in HBase-TRUNK #2115 (See https://builds.apache.org/job/HBase-TRUNK/2115/)
        HBASE-4197 RegionServer expects all scanner to be subclasses of
        HRegion.RegionScanner (Lars Hofhansl)

        tedyu :
        Files :

        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/coprocessor/BaseRegionObserver.java
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/coprocessor/RegionObserver.java
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/RegionCoprocessorHost.java
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/RegionScanner.java
        • /hbase/trunk/CHANGES.txt
        • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/coprocessor/SimpleRegionObserver.java
        • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/regionserver/TestHRegion.java
        • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/regionserver/TestWideScanner.java
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java
        Show
        Hudson added a comment - Integrated in HBase-TRUNK #2115 (See https://builds.apache.org/job/HBase-TRUNK/2115/ ) HBASE-4197 RegionServer expects all scanner to be subclasses of HRegion.RegionScanner (Lars Hofhansl) tedyu : Files : /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/coprocessor/BaseRegionObserver.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/coprocessor/RegionObserver.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/RegionCoprocessorHost.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/RegionScanner.java /hbase/trunk/CHANGES.txt /hbase/trunk/src/test/java/org/apache/hadoop/hbase/coprocessor/SimpleRegionObserver.java /hbase/trunk/src/test/java/org/apache/hadoop/hbase/regionserver/TestHRegion.java /hbase/trunk/src/test/java/org/apache/hadoop/hbase/regionserver/TestWideScanner.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java
        Hide
        Lars Hofhansl added a comment -

        There are 7 new failures. They all look transient. I'll have a more detailed look tomorrow.

        Show
        Lars Hofhansl added a comment - There are 7 new failures. They all look transient. I'll have a more detailed look tomorrow.
        Hide
        Ted Yu added a comment -

        The test failure of TestScannerTimeout wasn't transient.
        Here is why:

        java.lang.NullPointerException
                at org.apache.hadoop.hbase.regionserver.HRegionServer$QosFunction.apply(HRegionServer.java:455)
                at org.apache.hadoop.hbase.regionserver.HRegionServer$QosFunction.apply(HRegionServer.java:406)
        

        I have checked in an addendum that fixes the NPE. TestScannerTimeout passes on my laptop with the fix.

        Show
        Ted Yu added a comment - The test failure of TestScannerTimeout wasn't transient. Here is why: java.lang.NullPointerException at org.apache.hadoop.hbase.regionserver.HRegionServer$QosFunction.apply(HRegionServer.java:455) at org.apache.hadoop.hbase.regionserver.HRegionServer$QosFunction.apply(HRegionServer.java:406) I have checked in an addendum that fixes the NPE. TestScannerTimeout passes on my laptop with the fix.
        Hide
        Hudson added a comment -

        Integrated in HBase-TRUNK #2116 (See https://builds.apache.org/job/HBase-TRUNK/2116/)
        HBASE-4197 TestCoprocessorInterface demonstrates CustomScanner usage

        tedyu :
        Files :

        • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/coprocessor/TestCoprocessorInterface.java
        Show
        Hudson added a comment - Integrated in HBase-TRUNK #2116 (See https://builds.apache.org/job/HBase-TRUNK/2116/ ) HBASE-4197 TestCoprocessorInterface demonstrates CustomScanner usage tedyu : Files : /hbase/trunk/src/test/java/org/apache/hadoop/hbase/coprocessor/TestCoprocessorInterface.java
        Hide
        Lars Hofhansl added a comment -

        Ah yes. I missed the null instanceof X == false case. Thanks again Ted.
        The other two instanceof cases are already guarded against null, so all is good now.

        Show
        Lars Hofhansl added a comment - Ah yes. I missed the null instanceof X == false case. Thanks again Ted. The other two instanceof cases are already guarded against null, so all is good now.
        Hide
        Lars Hofhansl added a comment -

        Huh... 11 new failures from the test change? I am not closing the CustomScanner, but I don't think that can cause the new failures.

        Show
        Lars Hofhansl added a comment - Huh... 11 new failures from the test change? I am not closing the CustomScanner, but I don't think that can cause the new failures.
        Hide
        Lars Hofhansl added a comment -

        This really look transient to me.

        Show
        Lars Hofhansl added a comment - This really look transient to me.
        Hide
        Hudson added a comment -

        Integrated in HBase-TRUNK #2117 (See https://builds.apache.org/job/HBase-TRUNK/2117/)
        HBASE-4197 Added check of scanner against null

        tedyu :
        Files :

        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java
        Show
        Hudson added a comment - Integrated in HBase-TRUNK #2117 (See https://builds.apache.org/job/HBase-TRUNK/2117/ ) HBASE-4197 Added check of scanner against null tedyu : Files : /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java
        Hide
        Andrew Purtell added a comment -

        This was committed too quickly.

        Show
        Andrew Purtell added a comment - This was committed too quickly.
        Hide
        Ted Yu added a comment -

        @Andrew:
        You meant this: Added check of scanner against null, right ?
        If you have a better way of handling null pointer in HRegionServer$QosFunction.apply(), please share.

        Show
        Ted Yu added a comment - @Andrew: You meant this: Added check of scanner against null, right ? If you have a better way of handling null pointer in HRegionServer$QosFunction.apply(), please share.
        Hide
        Lars Hofhansl added a comment -

        @Andrew: The change generally is neutral in terms of behavior (mostly interface changes). The only case that I missed was that instanceof also guards again null.
        Or are you referring to Todd's request (recently on the dev mailing list) to have changes bake for a few days before they are committed?

        Show
        Lars Hofhansl added a comment - @Andrew: The change generally is neutral in terms of behavior (mostly interface changes). The only case that I missed was that instanceof also guards again null. Or are you referring to Todd's request (recently on the dev mailing list) to have changes bake for a few days before they are committed?

          People

          • Assignee:
            Lars Hofhansl
            Reporter:
            Lars Hofhansl
          • Votes:
            0 Vote for this issue
            Watchers:
            2 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development