You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@hbase.apache.org by Gary Helmling <gh...@gmail.com> on 2010/12/17 01:26:20 UTC
Review Request: HBASE-3260: Add explicit lifecycle methods to Coprocessor
interface
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://review.cloudera.org/r/1306/
-----------------------------------------------------------
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
Re: Review Request: HBASE-3260: Add explicit lifecycle methods to Coprocessor interface
Posted by Andrew Purtell <ap...@apache.org>.
No need to repost I think, +1 to commit with fixups unless there's an objection.
Best regards,
- Andy
--- On Thu, 12/16/10, Gary Helmling <gh...@gmail.com> wrote:
> From: Gary Helmling <gh...@gmail.com>
> Subject: Re: Review Request: HBASE-3260: Add explicit lifecycle methods to Coprocessor interface
> To: "Andrew Purtell" <ap...@apache.org>, stack@duboce.net
> Cc: "Gary Helmling" <gh...@gmail.com>, jiraposter@review.hbase.org, dev@hbase.apache.org
> Date: Thursday, December 16, 2010, 5:39 PM
>
>
> > On 2010-12-16 17:09:44, Andrew Purtell wrote:
> > >
> src/main/java/org/apache/hadoop/hbase/coprocessor/BaseEndpointCoprocessor.java,
> line 66
> > > <http://review.cloudera.org/r/1306/diff/1/?file=18379#file18379line66>
> > >
> > > What are these arguments
> about?
>
> Those are:
> - String protocol
> - long clientVersion
>
> from org.apache.hadoop.ipc.VersionedProtocol.
>
> Will fix these up.
>
>
> > On 2010-12-16 17:09:44, Andrew Purtell wrote:
> > >
> src/main/java/org/apache/hadoop/hbase/regionserver/CoprocessorHost.java,
> line 289
> > > <http://review.cloudera.org/r/1306/diff/1/?file=18383#file18383line289>
> > >
> > > Should be a WARN?
>
> Yeah, agree. Will fix.
>
>
> > On 2010-12-16 17:09:44, Andrew Purtell wrote:
> > >
> src/main/java/org/apache/hadoop/hbase/regionserver/CoprocessorHost.java,
> line 305
> > > <http://review.cloudera.org/r/1306/diff/1/?file=18383#file18383line305>
> > >
> > > Should be a WARN?
>
> Yeah, will fix.
>
>
> > On 2010-12-16 17:09:44, Andrew Purtell wrote:
> > >
> src/main/java/org/apache/hadoop/hbase/regionserver/CoprocessorHost.java,
> line 385
> > > <http://review.cloudera.org/r/1306/diff/1/?file=18383#file18383line385>
> > >
> > > 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.
>
> Ok will change this to a volatile boolean and repost.
>
>
> - Gary
>
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply,
> visit:
> http://review.cloudera.org/r/1306/#review2102
> -----------------------------------------------------------
>
>
> 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
> >
> >
>
>
Re: Review Request: HBASE-3260: Add explicit lifecycle methods to
Coprocessor interface
Posted by Gary Helmling <gh...@gmail.com>.
> On 2010-12-16 17:09:44, Andrew Purtell wrote:
> > src/main/java/org/apache/hadoop/hbase/coprocessor/BaseEndpointCoprocessor.java, line 66
> > <http://review.cloudera.org/r/1306/diff/1/?file=18379#file18379line66>
> >
> > What are these arguments about?
Those are:
- String protocol
- long clientVersion
from org.apache.hadoop.ipc.VersionedProtocol.
Will fix these up.
> On 2010-12-16 17:09:44, Andrew Purtell wrote:
> > src/main/java/org/apache/hadoop/hbase/regionserver/CoprocessorHost.java, line 289
> > <http://review.cloudera.org/r/1306/diff/1/?file=18383#file18383line289>
> >
> > Should be a WARN?
Yeah, agree. Will fix.
> On 2010-12-16 17:09:44, Andrew Purtell wrote:
> > src/main/java/org/apache/hadoop/hbase/regionserver/CoprocessorHost.java, line 305
> > <http://review.cloudera.org/r/1306/diff/1/?file=18383#file18383line305>
> >
> > Should be a WARN?
Yeah, will fix.
> On 2010-12-16 17:09:44, Andrew Purtell wrote:
> > src/main/java/org/apache/hadoop/hbase/regionserver/CoprocessorHost.java, line 385
> > <http://review.cloudera.org/r/1306/diff/1/?file=18383#file18383line385>
> >
> > 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.
Ok will change this to a volatile boolean and repost.
- Gary
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://review.cloudera.org/r/1306/#review2102
-----------------------------------------------------------
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
>
>
Re: Review Request: HBASE-3260: Add explicit lifecycle methods to
Coprocessor interface
Posted by Andrew Purtell <ap...@apache.org>.
-----------------------------------------------------------
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
>
>