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 <ghelml...@gmail.com> wrote:
> From: Gary Helmling <ghelml...@gmail.com>
> Subject: Re: Review Request: HBASE-3260: Add explicit lifecycle methods to
> Coprocessor interface
> To: "Andrew Purtell" <apurt...@apache.org>, st...@duboce.net
> Cc: "Gary Helmling" <ghelml...@gmail.com>, jirapos...@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
> >
> >
>
>
