----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://review.cloudera.org/r/1321/#review2132 -----------------------------------------------------------
Ship it! looks good. clean and nicely commented, well done. - Jonathan On 2010-12-20 22:31:38, Gary Helmling wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://review.cloudera.org/r/1321/ > ----------------------------------------------------------- > > (Updated 2010-12-20 22:31:38) > > > 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/coprocessor/package-info.java 1b7918c > 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/main/resources/hbase-default.xml f1cc4ae > > 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/TestCoprocessorEndpoint.java > 8eb2787 > > 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 > >
