----------------------------------------------------------- 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 > >
