Details

    • Type: Sub-task Sub-task
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 0.94.1, 0.95.2
    • Fix Version/s: 0.94.1, 0.95.0
    • Component/s: Coprocessors
    • Labels:
      None
    • Hadoop Flags:
      Reviewed
    1. HBASE-6224.patch
      16 kB
      Francis Liu
    2. HBASE-6224-apurtell.patch
      15 kB
      Andrew Purtell
    3. HBASE-6224-apurtell-0.94.patch
      14 kB
      Andrew Purtell

      Issue Links

        Activity

        Hide
        Francis Liu added a comment -

        After a discussion with Andy during the HBase BOF. I simplified the signature to just pass the CF-Path pair and let the user create the StoreFile instance if he needs to. I've also added a post hook.

        Show
        Francis Liu added a comment - After a discussion with Andy during the HBase BOF. I simplified the signature to just pass the CF-Path pair and let the user create the StoreFile instance if he needs to. I've also added a post hook.
        Hide
        Andrew Purtell added a comment - - edited

        This is the patch on https://reviews.apache.org/r/5380/ right?

        +1 if so.

        Edit: Found issues on re-review.

        Show
        Andrew Purtell added a comment - - edited This is the patch on https://reviews.apache.org/r/5380/ right? +1 if so. Edit: Found issues on re-review.
        Hide
        Andrew Purtell added a comment -

        Attached patch that addresses my re-review comments.

        Here's the functional difference:

        Index: hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java
        ===================================================================
        --- hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java	(revision 1352336)
        +++ hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java	(working copy)
        @@ -3122,10 +3122,20 @@
               HRegion region = getRegion(request.getRegion());
               List<Pair<byte[], String>> familyPaths = new ArrayList<Pair<byte[], String>>();
               for (FamilyPath familyPath: request.getFamilyPathList()) {
        -        familyPaths.add(new Pair<byte[], String>(
        -          familyPath.getFamily().toByteArray(), familyPath.getPath()));
        +        familyPaths.add(new Pair<byte[], String>(familyPath.getFamily().toByteArray(),
        +          familyPath.getPath()));
               }
        -      boolean loaded = region.bulkLoadHFiles(familyPaths);
        +      boolean bypass = false;
        +      if (region.getCoprocessorHost() != null) {
        +        bypass = region.getCoprocessorHost().preBulkLoadHFile(familyPaths);
        +      }
        +      boolean loaded = false;
        +      if (!bypass) {
        +        loaded = region.bulkLoadHFiles(familyPaths);
        +      }
        +      if (region.getCoprocessorHost() != null) {
        +        loaded = region.getCoprocessorHost().postBulkLoadHFile(familyPaths, loaded);
        +      }
               BulkLoadHFileResponse.Builder builder = BulkLoadHFileResponse.newBuilder();
               builder.setLoaded(loaded);
               return builder.build();
        
        Show
        Andrew Purtell added a comment - Attached patch that addresses my re-review comments. Here's the functional difference: Index: hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java =================================================================== --- hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java (revision 1352336) +++ hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java (working copy) @@ -3122,10 +3122,20 @@ HRegion region = getRegion(request.getRegion()); List<Pair< byte [], String >> familyPaths = new ArrayList<Pair< byte [], String >>(); for (FamilyPath familyPath: request.getFamilyPathList()) { - familyPaths.add( new Pair< byte [], String >( - familyPath.getFamily().toByteArray(), familyPath.getPath())); + familyPaths.add( new Pair< byte [], String >(familyPath.getFamily().toByteArray(), + familyPath.getPath())); } - boolean loaded = region.bulkLoadHFiles(familyPaths); + boolean bypass = false ; + if (region.getCoprocessorHost() != null ) { + bypass = region.getCoprocessorHost().preBulkLoadHFile(familyPaths); + } + boolean loaded = false ; + if (!bypass) { + loaded = region.bulkLoadHFiles(familyPaths); + } + if (region.getCoprocessorHost() != null ) { + loaded = region.getCoprocessorHost().postBulkLoadHFile(familyPaths, loaded); + } BulkLoadHFileResponse.Builder builder = BulkLoadHFileResponse.newBuilder(); builder.setLoaded(loaded); return builder.build();
        Hide
        Andrew Purtell added a comment -

        @Francis, if you are agreeable to my modifications to your patch, we can commit this.

        Show
        Andrew Purtell added a comment - @Francis, if you are agreeable to my modifications to your patch, we can commit this.
        Hide
        Francis Liu added a comment -

        @Andy, looks good to me. Good catch on the bypass and loaded. Should I come up with another patch?

        Show
        Francis Liu added a comment - @Andy, looks good to me. Good catch on the bypass and loaded. Should I come up with another patch?
        Hide
        Andrew Purtell added a comment -

        @Francis, not unless you want to add more. Otherwise I'll commit the -apurtell patch in a few hours. Thanks for the ltgm.

        Show
        Andrew Purtell added a comment - @Francis, not unless you want to add more. Otherwise I'll commit the -apurtell patch in a few hours. Thanks for the ltgm.
        Hide
        stack added a comment -

        Patch looks good. Its a pity StoreFile has to be made public but at list its limited private. Fix this on commit:

        +   * @param familyPaths family and path pairs to be bulk loaded
        +   * @param hasLoaded whether load was successful or not
        +   * @return
        +   * @throws IOException
        +   */
        

        Add something for return since this is a class that will be implemented (Maybe say what the pair Paths are too? StoreFiles?)

        Else lgtm. +1

        Show
        stack added a comment - Patch looks good. Its a pity StoreFile has to be made public but at list its limited private. Fix this on commit: + * @param familyPaths family and path pairs to be bulk loaded + * @param hasLoaded whether load was successful or not + * @ return + * @ throws IOException + */ Add something for return since this is a class that will be implemented (Maybe say what the pair Paths are too? StoreFiles?) Else lgtm. +1
        Hide
        Andrew Purtell added a comment -

        @stack Ok.

        Show
        Andrew Purtell added a comment - @stack Ok.
        Hide
        Francis Liu added a comment -

        @Andy, nope nothing to add. BTW can we backport this to 0.94 and 0.92? We're still mulling over wether to pickup 0.92 or 0.94. Though it would be good to get bulkload security covered in the current stable release anyway?

        Show
        Francis Liu added a comment - @Andy, nope nothing to add. BTW can we backport this to 0.94 and 0.92? We're still mulling over wether to pickup 0.92 or 0.94. Though it would be good to get bulkload security covered in the current stable release anyway?
        Hide
        Andrew Purtell added a comment -

        @Francis These hooks can go back to 0.92 IMHO, no problem there. However, we've not been committing "AccessController v2" changes to 0.92, only 0.94 and trunk. Seems prudent to leave 0.92 as "AccessController v1". Therefore I'd recommend you pick up 0.94.

        Show
        Andrew Purtell added a comment - @Francis These hooks can go back to 0.92 IMHO, no problem there. However, we've not been committing "AccessController v2" changes to 0.92, only 0.94 and trunk. Seems prudent to leave 0.92 as "AccessController v1". Therefore I'd recommend you pick up 0.94.
        Hide
        Francis Liu added a comment -

        Is "AccessController v2" already in a usable state in 0.94? If not what are the patches that need to go in?

        Show
        Francis Liu added a comment - Is "AccessController v2" already in a usable state in 0.94? If not what are the patches that need to go in?
        Hide
        Andrew Purtell added a comment -

        Is "AccessController v2" already in a usable state in 0.94?

        Yes, but it's under development (in a backwards compatible way). See umbrella issue HBASE-6096.

        Show
        Andrew Purtell added a comment - Is "AccessController v2" already in a usable state in 0.94? Yes, but it's under development (in a backwards compatible way). See umbrella issue HBASE-6096 .
        Hide
        Andrew Purtell added a comment -

        Committed to trunk and 0.94 branches. Tests pass locally. Thanks for the patch Francis!

        Show
        Andrew Purtell added a comment - Committed to trunk and 0.94 branches. Tests pass locally. Thanks for the patch Francis!
        Hide
        Hudson added a comment -

        Integrated in HBase-0.94 #274 (See https://builds.apache.org/job/HBase-0.94/274/)
        HBASE-6224. Add pre and post coprocessor hooks for bulk load (Francis Liu) (Revision 1353060)

        Result = FAILURE
        apurtell :
        Files :

        • /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/coprocessor/BaseRegionObserver.java
        • /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/coprocessor/RegionObserver.java
        • /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java
        • /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/regionserver/RegionCoprocessorHost.java
        • /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/regionserver/StoreFile.java
        • /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/regionserver/StoreFileScanner.java
        • /hbase/branches/0.94/src/test/java/org/apache/hadoop/hbase/coprocessor/SimpleRegionObserver.java
        • /hbase/branches/0.94/src/test/java/org/apache/hadoop/hbase/coprocessor/TestRegionObserverInterface.java
        Show
        Hudson added a comment - Integrated in HBase-0.94 #274 (See https://builds.apache.org/job/HBase-0.94/274/ ) HBASE-6224 . Add pre and post coprocessor hooks for bulk load (Francis Liu) (Revision 1353060) Result = FAILURE apurtell : Files : /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/coprocessor/BaseRegionObserver.java /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/coprocessor/RegionObserver.java /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/regionserver/RegionCoprocessorHost.java /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/regionserver/StoreFile.java /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/regionserver/StoreFileScanner.java /hbase/branches/0.94/src/test/java/org/apache/hadoop/hbase/coprocessor/SimpleRegionObserver.java /hbase/branches/0.94/src/test/java/org/apache/hadoop/hbase/coprocessor/TestRegionObserverInterface.java
        Hide
        Hudson added a comment -

        Integrated in HBase-TRUNK-on-Hadoop-2.0.0 #66 (See https://builds.apache.org/job/HBase-TRUNK-on-Hadoop-2.0.0/66/)
        HBASE-6224. Add pre and post coprocessor hooks for bulk load (Francis Liu) (Revision 1353057)

        Result = FAILURE
        apurtell :
        Files :

        • /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/coprocessor/BaseRegionObserver.java
        • /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/coprocessor/RegionObserver.java
        • /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java
        • /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/RegionCoprocessorHost.java
        • /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/StoreFile.java
        • /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/StoreFileScanner.java
        • /hbase/trunk/hbase-server/src/test/java/org/apache/hadoop/hbase/coprocessor/SimpleRegionObserver.java
        • /hbase/trunk/hbase-server/src/test/java/org/apache/hadoop/hbase/coprocessor/TestRegionObserverInterface.java
        Show
        Hudson added a comment - Integrated in HBase-TRUNK-on-Hadoop-2.0.0 #66 (See https://builds.apache.org/job/HBase-TRUNK-on-Hadoop-2.0.0/66/ ) HBASE-6224 . Add pre and post coprocessor hooks for bulk load (Francis Liu) (Revision 1353057) Result = FAILURE apurtell : Files : /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/coprocessor/BaseRegionObserver.java /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/coprocessor/RegionObserver.java /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/RegionCoprocessorHost.java /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/StoreFile.java /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/StoreFileScanner.java /hbase/trunk/hbase-server/src/test/java/org/apache/hadoop/hbase/coprocessor/SimpleRegionObserver.java /hbase/trunk/hbase-server/src/test/java/org/apache/hadoop/hbase/coprocessor/TestRegionObserverInterface.java
        Hide
        Hudson added a comment -

        Integrated in HBase-0.94-security #37 (See https://builds.apache.org/job/HBase-0.94-security/37/)
        HBASE-6224. Add pre and post coprocessor hooks for bulk load (Francis Liu) (Revision 1353060)

        Result = SUCCESS
        apurtell :
        Files :

        • /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/coprocessor/BaseRegionObserver.java
        • /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/coprocessor/RegionObserver.java
        • /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java
        • /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/regionserver/RegionCoprocessorHost.java
        • /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/regionserver/StoreFile.java
        • /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/regionserver/StoreFileScanner.java
        • /hbase/branches/0.94/src/test/java/org/apache/hadoop/hbase/coprocessor/SimpleRegionObserver.java
        • /hbase/branches/0.94/src/test/java/org/apache/hadoop/hbase/coprocessor/TestRegionObserverInterface.java
        Show
        Hudson added a comment - Integrated in HBase-0.94-security #37 (See https://builds.apache.org/job/HBase-0.94-security/37/ ) HBASE-6224 . Add pre and post coprocessor hooks for bulk load (Francis Liu) (Revision 1353060) Result = SUCCESS apurtell : Files : /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/coprocessor/BaseRegionObserver.java /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/coprocessor/RegionObserver.java /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/regionserver/RegionCoprocessorHost.java /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/regionserver/StoreFile.java /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/regionserver/StoreFileScanner.java /hbase/branches/0.94/src/test/java/org/apache/hadoop/hbase/coprocessor/SimpleRegionObserver.java /hbase/branches/0.94/src/test/java/org/apache/hadoop/hbase/coprocessor/TestRegionObserverInterface.java

          People

          • Assignee:
            Francis Liu
            Reporter:
            Francis Liu
          • Votes:
            0 Vote for this issue
            Watchers:
            7 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development