You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@hbase.apache.org by Andrew Purtell <ap...@apache.org> on 2010/12/15 03:16:22 UTC
Review Request: Allow Observers to completely override base function
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://review.cloudera.org/r/1295/
-----------------------------------------------------------
Review request for hbase, Jonathan Gray and Mingjie Lai.
Summary
-------
Currently an observer can act as a filter or translator but cannot stop a subsequent call down to the base method for get, put, delete, etc. This patch allows observers to 1) keep any subsequently chained observer from executing, or 2) prevent default behavior from executing. This latter option allows a preXXX hook to completely reimplement something.
I also found and fixed some logic bugs in coprocessor framework integration in HRegion.
I will squelch the added extraneous whitespace upon commit.
This addresses bug HBASE-3348.
http://issues.apache.org/jira/browse/HBASE-3348
Diffs
-----
src/main/java/org/apache/hadoop/hbase/coprocessor/BaseRegionObserverCoprocessor.java 134ed2f
src/main/java/org/apache/hadoop/hbase/coprocessor/CoprocessorEnvironment.java 654b179
src/main/java/org/apache/hadoop/hbase/coprocessor/RegionObserver.java 10dfff4
src/main/java/org/apache/hadoop/hbase/regionserver/CoprocessorHost.java c57ca0c
src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java cf9cad0
src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java 8248f5f
src/test/java/org/apache/hadoop/hbase/coprocessor/SimpleRegionObserver.java 345790f
src/test/java/org/apache/hadoop/hbase/coprocessor/TestRegionObserverStacking.java 9ef3562
Diff: http://review.cloudera.org/r/1295/diff
Testing
-------
All coprocessor unit tests pass. No failures of other unit tests observed that might be related to these changes.
Thanks,
Andrew
Re: Review Request: Allow Observers to completely override base function
Posted by Andrew Purtell <ap...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://review.cloudera.org/r/1295/#review2066
-----------------------------------------------------------
src/main/java/org/apache/hadoop/hbase/coprocessor/BaseRegionObserverCoprocessor.java
<http://review.cloudera.org/r/1295/#comment6485>
No need to return something. We are assuming the 'result' object is sufficiently modifiable by its methods. An empty result is passed in to be populated.
src/main/java/org/apache/hadoop/hbase/coprocessor/BaseRegionObserverCoprocessor.java
<http://review.cloudera.org/r/1295/#comment6486>
We assume the Get is fully modifiable via its methods. Replacing the query with something else is not a problem: The coprocessor can do just about anything from the preXXX hooks, populate the Result object with data, then tell the framework to return without invoking the base function.
src/main/java/org/apache/hadoop/hbase/coprocessor/RegionObserver.java
<http://review.cloudera.org/r/1295/#comment6487>
Ok
src/main/java/org/apache/hadoop/hbase/coprocessor/RegionObserver.java
<http://review.cloudera.org/r/1295/#comment6488>
Yes, if you play with the result but don't call e.bypass() then there is no impact. To change the result of an increment you need to use postIncrement.
src/main/java/org/apache/hadoop/hbase/regionserver/CoprocessorHost.java
<http://review.cloudera.org/r/1295/#comment6489>
I'll add a note to the javadoc.
- Andrew
On 2010-12-14 18:16:21, Andrew Purtell wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://review.cloudera.org/r/1295/
> -----------------------------------------------------------
>
> (Updated 2010-12-14 18:16:21)
>
>
> Review request for hbase, Jonathan Gray and Mingjie Lai.
>
>
> Summary
> -------
>
> Currently an observer can act as a filter or translator but cannot stop a subsequent call down to the base method for get, put, delete, etc. This patch allows observers to 1) keep any subsequently chained observer from executing, or 2) prevent default behavior from executing. This latter option allows a preXXX hook to completely reimplement something.
>
> I also found and fixed some logic bugs in coprocessor framework integration in HRegion.
>
> I will squelch the added extraneous whitespace upon commit.
>
>
> This addresses bug HBASE-3348.
> http://issues.apache.org/jira/browse/HBASE-3348
>
>
> Diffs
> -----
>
> src/main/java/org/apache/hadoop/hbase/coprocessor/BaseRegionObserverCoprocessor.java 134ed2f
> src/main/java/org/apache/hadoop/hbase/coprocessor/CoprocessorEnvironment.java 654b179
> src/main/java/org/apache/hadoop/hbase/coprocessor/RegionObserver.java 10dfff4
> src/main/java/org/apache/hadoop/hbase/regionserver/CoprocessorHost.java c57ca0c
> src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java cf9cad0
> src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java 8248f5f
> src/test/java/org/apache/hadoop/hbase/coprocessor/SimpleRegionObserver.java 345790f
> src/test/java/org/apache/hadoop/hbase/coprocessor/TestRegionObserverStacking.java 9ef3562
>
> Diff: http://review.cloudera.org/r/1295/diff
>
>
> Testing
> -------
>
> All coprocessor unit tests pass. No failures of other unit tests observed that might be related to these changes.
>
>
> Thanks,
>
> Andrew
>
>
Re: Review Request: Allow Observers to completely override base function
Posted by Ted Dunning <td...@maprtech.com>.
Indeed, if you look at the implementation of AtomicInteger, it is mostly
just a volatile int.
On Tue, Dec 14, 2010 at 6:36 PM, Ryan Rawson <ry...@gmail.com> wrote:
> src/main/java/org/apache/hadoop/hbase/regionserver/CoprocessorHost.java
> <http://review.cloudera.org/r/1295/#comment6479>
>
> unless you need CAS semantics, you can just use volatile here. We are
> over-using the Atomic* stuff sometimes.
Re: Review Request: Allow Observers to completely override base function
Posted by Ryan Rawson <ry...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://review.cloudera.org/r/1295/#review2063
-----------------------------------------------------------
src/main/java/org/apache/hadoop/hbase/coprocessor/BaseRegionObserverCoprocessor.java
<http://review.cloudera.org/r/1295/#comment6478>
presumably a co-processor could modify the Get object to implement policy? Another consideration is replacing the Get query with an alternate query, for example we have InternalGet subclasses for additional functionality, I'm just winging this though.
src/main/java/org/apache/hadoop/hbase/regionserver/CoprocessorHost.java
<http://review.cloudera.org/r/1295/#comment6479>
unless you need CAS semantics, you can just use volatile here. We are over-using the Atomic* stuff sometimes.
- Ryan
On 2010-12-14 18:16:21, Andrew Purtell wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://review.cloudera.org/r/1295/
> -----------------------------------------------------------
>
> (Updated 2010-12-14 18:16:21)
>
>
> Review request for hbase, Jonathan Gray and Mingjie Lai.
>
>
> Summary
> -------
>
> Currently an observer can act as a filter or translator but cannot stop a subsequent call down to the base method for get, put, delete, etc. This patch allows observers to 1) keep any subsequently chained observer from executing, or 2) prevent default behavior from executing. This latter option allows a preXXX hook to completely reimplement something.
>
> I also found and fixed some logic bugs in coprocessor framework integration in HRegion.
>
> I will squelch the added extraneous whitespace upon commit.
>
>
> This addresses bug HBASE-3348.
> http://issues.apache.org/jira/browse/HBASE-3348
>
>
> Diffs
> -----
>
> src/main/java/org/apache/hadoop/hbase/coprocessor/BaseRegionObserverCoprocessor.java 134ed2f
> src/main/java/org/apache/hadoop/hbase/coprocessor/CoprocessorEnvironment.java 654b179
> src/main/java/org/apache/hadoop/hbase/coprocessor/RegionObserver.java 10dfff4
> src/main/java/org/apache/hadoop/hbase/regionserver/CoprocessorHost.java c57ca0c
> src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java cf9cad0
> src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java 8248f5f
> src/test/java/org/apache/hadoop/hbase/coprocessor/SimpleRegionObserver.java 345790f
> src/test/java/org/apache/hadoop/hbase/coprocessor/TestRegionObserverStacking.java 9ef3562
>
> Diff: http://review.cloudera.org/r/1295/diff
>
>
> Testing
> -------
>
> All coprocessor unit tests pass. No failures of other unit tests observed that might be related to these changes.
>
>
> Thanks,
>
> Andrew
>
>
Re: Review Request: Allow Observers to completely override base function
Posted by Ryan Rawson <ry...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://review.cloudera.org/r/1295/#review2064
-----------------------------------------------------------
Ship it!
- Ryan
On 2010-12-14 18:16:21, Andrew Purtell wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://review.cloudera.org/r/1295/
> -----------------------------------------------------------
>
> (Updated 2010-12-14 18:16:21)
>
>
> Review request for hbase, Jonathan Gray and Mingjie Lai.
>
>
> Summary
> -------
>
> Currently an observer can act as a filter or translator but cannot stop a subsequent call down to the base method for get, put, delete, etc. This patch allows observers to 1) keep any subsequently chained observer from executing, or 2) prevent default behavior from executing. This latter option allows a preXXX hook to completely reimplement something.
>
> I also found and fixed some logic bugs in coprocessor framework integration in HRegion.
>
> I will squelch the added extraneous whitespace upon commit.
>
>
> This addresses bug HBASE-3348.
> http://issues.apache.org/jira/browse/HBASE-3348
>
>
> Diffs
> -----
>
> src/main/java/org/apache/hadoop/hbase/coprocessor/BaseRegionObserverCoprocessor.java 134ed2f
> src/main/java/org/apache/hadoop/hbase/coprocessor/CoprocessorEnvironment.java 654b179
> src/main/java/org/apache/hadoop/hbase/coprocessor/RegionObserver.java 10dfff4
> src/main/java/org/apache/hadoop/hbase/regionserver/CoprocessorHost.java c57ca0c
> src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java cf9cad0
> src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java 8248f5f
> src/test/java/org/apache/hadoop/hbase/coprocessor/SimpleRegionObserver.java 345790f
> src/test/java/org/apache/hadoop/hbase/coprocessor/TestRegionObserverStacking.java 9ef3562
>
> Diff: http://review.cloudera.org/r/1295/diff
>
>
> Testing
> -------
>
> All coprocessor unit tests pass. No failures of other unit tests observed that might be related to these changes.
>
>
> Thanks,
>
> Andrew
>
>
Re: Review Request: Allow Observers to completely override base function
Posted by Jonathan Gray <jg...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://review.cloudera.org/r/1295/#review2065
-----------------------------------------------------------
Ship it!
Looks great. Thanks Andy!
src/main/java/org/apache/hadoop/hbase/coprocessor/BaseRegionObserverCoprocessor.java
<http://review.cloudera.org/r/1295/#comment6480>
We had discussed these returning the return type but we'll pass in an empty result instead?
src/main/java/org/apache/hadoop/hbase/coprocessor/BaseRegionObserverCoprocessor.java
<http://review.cloudera.org/r/1295/#comment6481>
so here we have to because it's a primitive. so is there a reason not to do this elsewhere on the pre methods?
Or, this is for chaining, that's why... got it
src/main/java/org/apache/hadoop/hbase/coprocessor/RegionObserver.java
<http://review.cloudera.org/r/1295/#comment6482>
there shouldn't be @return javadoc now, right?
I think it would be noting in the javadoc that you can modify the result that is passed in though.
src/main/java/org/apache/hadoop/hbase/coprocessor/RegionObserver.java
<http://review.cloudera.org/r/1295/#comment6483>
if i don't do bypass/complete, and i play with this Result, what is the impact? do we just drop this if the flags don't get set?
src/main/java/org/apache/hadoop/hbase/regionserver/CoprocessorHost.java
<http://review.cloudera.org/r/1295/#comment6484>
looks like we drop the result. makes sense. not sure if we should add something to some doc somewhere but it's clear to me now.
- Jonathan
On 2010-12-14 18:16:21, Andrew Purtell wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://review.cloudera.org/r/1295/
> -----------------------------------------------------------
>
> (Updated 2010-12-14 18:16:21)
>
>
> Review request for hbase, Jonathan Gray and Mingjie Lai.
>
>
> Summary
> -------
>
> Currently an observer can act as a filter or translator but cannot stop a subsequent call down to the base method for get, put, delete, etc. This patch allows observers to 1) keep any subsequently chained observer from executing, or 2) prevent default behavior from executing. This latter option allows a preXXX hook to completely reimplement something.
>
> I also found and fixed some logic bugs in coprocessor framework integration in HRegion.
>
> I will squelch the added extraneous whitespace upon commit.
>
>
> This addresses bug HBASE-3348.
> http://issues.apache.org/jira/browse/HBASE-3348
>
>
> Diffs
> -----
>
> src/main/java/org/apache/hadoop/hbase/coprocessor/BaseRegionObserverCoprocessor.java 134ed2f
> src/main/java/org/apache/hadoop/hbase/coprocessor/CoprocessorEnvironment.java 654b179
> src/main/java/org/apache/hadoop/hbase/coprocessor/RegionObserver.java 10dfff4
> src/main/java/org/apache/hadoop/hbase/regionserver/CoprocessorHost.java c57ca0c
> src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java cf9cad0
> src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java 8248f5f
> src/test/java/org/apache/hadoop/hbase/coprocessor/SimpleRegionObserver.java 345790f
> src/test/java/org/apache/hadoop/hbase/coprocessor/TestRegionObserverStacking.java 9ef3562
>
> Diff: http://review.cloudera.org/r/1295/diff
>
>
> Testing
> -------
>
> All coprocessor unit tests pass. No failures of other unit tests observed that might be related to these changes.
>
>
> Thanks,
>
> Andrew
>
>