HBase
  1. HBase
  2. HBASE-2000 Coprocessors
  3. HBASE-3348

Allow Observers to completely override base function

    Details

    • Type: Sub-task Sub-task
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 0.92.0
    • Component/s: Coprocessors
    • Labels:
      None
    • Hadoop Flags:
      Reviewed

      Description

      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 means an observer cannot completely override the base function. To deal with this we can:

      • Change the preXXX methods to return the same type as the postXXX methods, the same return type of the base method.
      • Extend Coprocessor.Environment with methods that get/set a "should continue" flag.

      The framework should check the "should continue" flag before calling the base method. If not, just return what was returned by the preXXX method.

      1. HBASE-3348.patch
        84 kB
        Andrew Purtell

        Activity

        Andrew Purtell created issue -
        Andrew Purtell made changes -
        Field Original Value New Value
        Description 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 means an observer cannot completely override the base function. To deal with this we can:

        - Change the preXXX methods to return the same type as the postXXX methods, the same return type of the base method.

        - Extend {{Coprocessor.Environment}} with methods that get/set a "should continue" flag.
        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 means an observer cannot completely override the base function. To deal with this we can:

        - Change the preXXX methods to return the same type as the postXXX methods, the same return type of the base method.

        - Extend {{Coprocessor.Environment}} with methods that get/set a "should continue" flag.

        The framework should check the "should continue" flag before calling the base method. If not, just return what was returned by the preXXX method.
        Andrew Purtell made changes -
        Assignee Andrew Purtell [ apurtell ]
        Andrew Purtell made changes -
        Status Open [ 1 ] In Progress [ 3 ]
        Hide
        Andrew Purtell added a comment -

        I had a patch ready but it is failing some tests, digging in.

        Show
        Andrew Purtell added a comment - I had a patch ready but it is failing some tests, digging in.
        Hide
        Andrew Purtell added a comment -

        Patch contains API changes and fixes some logic bugs I found with coprocessor integration in HRegion. Not quite ready yet, will put up a new patch on RB then.

        Show
        Andrew Purtell added a comment - Patch contains API changes and fixes some logic bugs I found with coprocessor integration in HRegion. Not quite ready yet, will put up a new patch on RB then.
        Andrew Purtell made changes -
        Attachment HBASE-3348.patch [ 12466246 ]
        Hide
        HBase Review Board added a comment -

        Message from: "Andrew Purtell" <apurtell@apache.org>

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

        Show
        HBase Review Board added a comment - Message from: "Andrew Purtell" <apurtell@apache.org> ----------------------------------------------------------- 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
        Hide
        HBase Review Board added a comment -

        Message from: "Ryan Rawson" <ryanobjc@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
        Show
        HBase Review Board added a comment - Message from: "Ryan Rawson" <ryanobjc@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
        Hide
        HBase Review Board added a comment -

        Message from: "Ryan Rawson" <ryanobjc@gmail.com>

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        http://review.cloudera.org/r/1295/#review2064
        -----------------------------------------------------------

        Ship it!

        • Ryan
        Show
        HBase Review Board added a comment - Message from: "Ryan Rawson" <ryanobjc@gmail.com> ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://review.cloudera.org/r/1295/#review2064 ----------------------------------------------------------- Ship it! Ryan
        Hide
        HBase Review Board added a comment -

        Message from: "Jonathan Gray" <jgray@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
        Show
        HBase Review Board added a comment - Message from: "Jonathan Gray" <jgray@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
        Hide
        HBase Review Board added a comment -

        Message from: "Andrew Purtell" <apurtell@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
        Show
        HBase Review Board added a comment - Message from: "Andrew Purtell" <apurtell@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
        Andrew Purtell made changes -
        Attachment HBASE-3348.patch [ 12466246 ]
        Andrew Purtell made changes -
        Attachment HBASE-3348.patch [ 12466303 ]
        Hide
        Andrew Purtell added a comment -

        Committed the updated patch. Thanks for the reviews Ryan and Jon. Reopen if more needed here.

        Show
        Andrew Purtell added a comment - Committed the updated patch. Thanks for the reviews Ryan and Jon. Reopen if more needed here.
        Andrew Purtell made changes -
        Status In Progress [ 3 ] Resolved [ 5 ]
        Hadoop Flags [Reviewed]
        Resolution Fixed [ 1 ]
        Hide
        Hudson added a comment -

        Integrated in HBase-TRUNK #1697 (See https://hudson.apache.org/hudson/job/HBase-TRUNK/1697/)

        Show
        Hudson added a comment - Integrated in HBase-TRUNK #1697 (See https://hudson.apache.org/hudson/job/HBase-TRUNK/1697/ )
        Todd Lipcon made changes -
        Component/s coprocessors [ 12314191 ]

          People

          • Assignee:
            Andrew Purtell
            Reporter:
            Andrew Purtell
          • Votes:
            0 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development