hbase-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Jonathan Gray" <jg...@apache.org>
Subject Re: Review Request: HBASE-3256: Coprocessors: Coprocessor host and observer for HMaster
Date Tue, 21 Dec 2010 03:16:32 GMT

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://review.cloudera.org/r/1321/#review2126
-----------------------------------------------------------

Ship it!


great work!  just a few small comments but otherwise +1


src/main/java/org/apache/hadoop/hbase/coprocessor/CoprocessorHost.java
<http://review.cloudera.org/r/1321/#comment6607>

    does DEFAULT really mean REGION/REGIONSERVER?  or is it both?
    
    not a big deal if it's just variable names but since it's a config param, we should nail
it now before it gets out in a release.



src/main/java/org/apache/hadoop/hbase/coprocessor/CoprocessorHost.java
<http://review.cloudera.org/r/1321/#comment6608>

    this code might have been in other earlier patches but could there be false positives
with this?  it'd be silly to load FancyCoprocessor and then MyFancyCoprocessor but i guess
this is to cover the package?  maybe parse out the class name?



src/main/java/org/apache/hadoop/hbase/master/HMaster.java
<http://review.cloudera.org/r/1321/#comment6616>

    doesn't preBalance() return a void?  it's preBalanceSwitch that returns boolean



src/main/java/org/apache/hadoop/hbase/master/HMaster.java
<http://review.cloudera.org/r/1321/#comment6617>

    and here we should get the boolean return value (and base class should return the input
value)



src/main/java/org/apache/hadoop/hbase/master/HMaster.java
<http://review.cloudera.org/r/1321/#comment6618>

    would we ever want to override default assign behavior?  it's feasible... might want to
be future proof w/ the api?



src/main/java/org/apache/hadoop/hbase/master/HMaster.java
<http://review.cloudera.org/r/1321/#comment6619>

    same here


- Jonathan


On 2010-12-20 18:04:33, Gary Helmling wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://review.cloudera.org/r/1321/
> -----------------------------------------------------------
> 
> (Updated 2010-12-20 18:04:33)
> 
> 
> Review request for hbase, stack, Andrew Purtell, and Jonathan Gray.
> 
> 
> Summary
> -------
> 
> This patch adds a new MasterObserver interface with pre/post hooks provided for operations
defined in org.apache.hadoop.hbase.ipc.HMasterInterface.
> 
> In order to accommodate the new MasterObserver interface, I've also refactored out common
coprocessor base code, with subclasses providing for region-specific and master-specific behavior.
> 
> The new code structure is (excuse my poor ascii art):
> 
> CoprocessorEnvironment - base interface for common facilities provided to CP implementations
>     | 
>     |- RegionCoprocessorEnvironment - adds access to current HRegion and RegionServerServices
(for RegionObservers)
>     |
>     |- MasterCoprocessorEnvironment - adds access to MasterServerServices (for MasterObservers)
> 
> CoprocessorHost - abstract base providing core CP loading and invocation code and the
base CoprocessorEnvironment implementation
>     |
>     |- RegionCoprocessorHost - provides hooks for invoking RegionObserver pre/post methods
and RegionCoprocessorEnvironment implementation
>     |
>     |- MasterCoprocessorHost - provides hooks for invoking MasterObserver pre/post methods
and MasterCoprocessorEnvironment implementation
> 
> Also added:
>  - org.apache.hadoop.hbase.coprocessor.BaseMasterObserver - stubs out full MasterObserver
interface with empty methods for convenience
>  - org.apache.hadoop.hbase.coprocessor.TestMasterObserver - tests that MasterObserver
pre/post methods are called during master operations.
> 
> In particular, please let me know if the MasterObserver method inputs and outputs are
sufficient for whatever you anticipate doing with it.  It should meet our needs for security
checks, but more input would be helpful.
> 
> 
> This addresses bug HBASE-3256.
>     http://issues.apache.org/jira/browse/HBASE-3256
> 
> 
> Diffs
> -----
> 
>   src/main/java/org/apache/hadoop/hbase/coprocessor/BaseMasterObserver.java PRE-CREATION

>   src/main/java/org/apache/hadoop/hbase/coprocessor/BaseRegionObserverCoprocessor.java
1ffead0 
>   src/main/java/org/apache/hadoop/hbase/coprocessor/CoprocessorEnvironment.java c4fa526

>   src/main/java/org/apache/hadoop/hbase/coprocessor/CoprocessorHost.java PRE-CREATION

>   src/main/java/org/apache/hadoop/hbase/coprocessor/MasterCoprocessorEnvironment.java
PRE-CREATION 
>   src/main/java/org/apache/hadoop/hbase/coprocessor/MasterObserver.java PRE-CREATION

>   src/main/java/org/apache/hadoop/hbase/coprocessor/RegionCoprocessorEnvironment.java
PRE-CREATION 
>   src/main/java/org/apache/hadoop/hbase/coprocessor/RegionObserver.java 97198ec 
>   src/main/java/org/apache/hadoop/hbase/master/HMaster.java 18f7787 
>   src/main/java/org/apache/hadoop/hbase/master/MasterCoprocessorHost.java PRE-CREATION

>   src/main/java/org/apache/hadoop/hbase/master/MasterServices.java 593254b 
>   src/main/java/org/apache/hadoop/hbase/regionserver/CoprocessorHost.java f71fea6 
>   src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java 1d48131 
>   src/main/java/org/apache/hadoop/hbase/regionserver/RegionCoprocessorHost.java PRE-CREATION

>   src/test/java/org/apache/hadoop/hbase/coprocessor/ColumnAggregationEndpoint.java 43569f1

>   src/test/java/org/apache/hadoop/hbase/coprocessor/SimpleRegionObserver.java 902a60f

>   src/test/java/org/apache/hadoop/hbase/coprocessor/TestCoprocessorInterface.java 5434d01

>   src/test/java/org/apache/hadoop/hbase/coprocessor/TestMasterObserver.java PRE-CREATION

>   src/test/java/org/apache/hadoop/hbase/coprocessor/TestRegionObserverInterface.java
5f5fc9a 
>   src/test/java/org/apache/hadoop/hbase/coprocessor/TestRegionObserverStacking.java 3193abf

>   src/test/java/org/apache/hadoop/hbase/master/TestCatalogJanitor.java 5be8daa 
> 
> Diff: http://review.cloudera.org/r/1321/diff
> 
> 
> Testing
> -------
> 
> Added a new test (org.apache.hadoop.hbase.coprocessor.TestMasterObserver) to cover pre/post
hook invocation.
> 
> All existing coprocessor tests still pass.
> 
> 
> Thanks,
> 
> Gary
> 
>


Mime
View raw message