hbase-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Andrew Purtell" <apurt...@apache.org>
Subject Re: Review Request: HBASE-3260: Add explicit lifecycle methods to Coprocessor interface
Date Fri, 17 Dec 2010 01:09:44 GMT

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


Mime
View raw message