----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://review.cloudera.org/r/1306/#review2102 -----------------------------------------------------------
Ship it! src/main/java/org/apache/hadoop/hbase/coprocessor/BaseEndpointCoprocessor.java <http://review.cloudera.org/r/1306/#comment6557> What are these arguments about? src/main/java/org/apache/hadoop/hbase/regionserver/CoprocessorHost.java <http://review.cloudera.org/r/1306/#comment6558> Should be a WARN? src/main/java/org/apache/hadoop/hbase/regionserver/CoprocessorHost.java <http://review.cloudera.org/r/1306/#comment6559> Should be a WARN? src/main/java/org/apache/hadoop/hbase/regionserver/CoprocessorHost.java <http://review.cloudera.org/r/1306/#comment6560> Since you are committing a change set in this area, Ryan suggested no need for AtomicBoolean here, could just be plain volatile boolean. I think that's right. - Andrew On 2010-12-16 16:26:20, Gary Helmling wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://review.cloudera.org/r/1306/ > ----------------------------------------------------------- > > (Updated 2010-12-16 16:26:20) > > > Review request for hbase, stack and Andrew Purtell. > > > Summary > ------- > > This patch adds explicit start() and stop() methods for lifecycle management > to the Coprocessor interface and refactors some of the > Coprocessor/RegionObserver distinction, moving the region-related pre/post > hooks that were previously in Coprocessor to RegionObserver. > > Coprocessor is now the base interface, containing only: > - start() > - stop() > - Priority enum > - State enum > > RegionObserver extends Coprocessor, and now contains the additional pre/post > hooks, moved from Coprocessor: > - pre/postOpen > - pre/postClose > - pre/postFlush > - pre/postCompact > - pre/postSplit > > This will allow cleaner extension in the future, to allow addition of a > MasterObserver interface, for example. > > As shown above, I've also added a new Coprocessor.State enum consisting of > the states: > UNINSTALLED -> INSTALLED -> STARTING -> ACTIVE -> STOPPING -> STOPPED > > However, the UNINSTALLED/INSTALLED distinction is not particularly useful at > the moment. I'd appreciate other feedback on what's necessary here. The > current handling could make do with: > UNINSTALLED -> STARTING -> ACTIVE -> STOPPING -> UNINSTALLED (4 total states) > > However, the UNINSTALLED/INSTALLED distinction may be useful if we want to > add class level initialization in the future... > > > This addresses bug HBASE-3260. > http://issues.apache.org/jira/browse/HBASE-3260 > > > Diffs > ----- > > > src/main/java/org/apache/hadoop/hbase/coprocessor/BaseEndpointCoprocessor.java > b81a465 > > src/main/java/org/apache/hadoop/hbase/coprocessor/BaseRegionObserverCoprocessor.java > f022598 > src/main/java/org/apache/hadoop/hbase/coprocessor/Coprocessor.java 7ea1c5e > src/main/java/org/apache/hadoop/hbase/coprocessor/RegionObserver.java > 1792290 > src/main/java/org/apache/hadoop/hbase/regionserver/CoprocessorHost.java > f028525 > src/test/java/org/apache/hadoop/hbase/coprocessor/SimpleRegionObserver.java > 3db4c36 > > src/test/java/org/apache/hadoop/hbase/coprocessor/TestCoprocessorInterface.java > 81cb75d > > Diff: http://review.cloudera.org/r/1306/diff > > > Testing > ------- > > Added tests for start() and stop() method invocation in > org.apache.hadoop.hbase.coprocessor.TestCoprocessorInterface > > The existing TestCoprocessorEndpoint, TestCoprocessorInterface, > TestRegionObserverInterface, TestRegionObserverStacking tests continue to > work. I'm not seeing any new failures in the rest of the tests, but > TestReplication is timing out for me, preventing all tests from executing. > > > Thanks, > > Gary > >
