HBase
  1. HBase
  2. HBASE-4331

Bypassing default actions in prePut fails sometimes with HTable client

    Details

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

      Description

      While testing some other scenario I found calling CoprocessorEnvironment.bypass() fails if all trailing puts in a batch are bypassed that way. By extension a single bypassed put will also fail.

      The problem is that the puts are removed from the batch in a way that does not align them with the result-status, and in addition the result is never marked as success.

      A possible fix is to just mark bypassed puts as SUCCESS and filter them in the following logic.
      (I also contemplated a new BYPASSED OperationStatusCode, but that turned out to be not necessary).

      1. 4331.txt
        2 kB
        Lars Hofhansl
      2. 4331-v2.txt
        7 kB
        Lars Hofhansl
      3. 4331-v3.txt
        7 kB
        Lars Hofhansl
      4. 4331-v4.txt
        8 kB
        Lars Hofhansl

        Activity

        Hide
        Hudson added a comment -

        Integrated in HBase-TRUNK #2196 (See https://builds.apache.org/job/HBase-TRUNK/2196/)
        HBASE-4331 Bypassing default actions in prePut fails sometimes with HTable client

        garyh :
        Files :

        • /hbase/trunk/CHANGES.txt
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
        • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/coprocessor/TestRegionObserverBypass.java
        Show
        Hudson added a comment - Integrated in HBase-TRUNK #2196 (See https://builds.apache.org/job/HBase-TRUNK/2196/ ) HBASE-4331 Bypassing default actions in prePut fails sometimes with HTable client garyh : Files : /hbase/trunk/CHANGES.txt /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java /hbase/trunk/src/test/java/org/apache/hadoop/hbase/coprocessor/TestRegionObserverBypass.java
        Hide
        Gary Helmling added a comment -

        Committed to trunk. Thanks for the patch Lars.

        Show
        Gary Helmling added a comment - Committed to trunk. Thanks for the patch Lars.
        Hide
        Lars Hofhansl added a comment -

        Just forgot to change the config property that points to the coprocessor class when I changed the name of the test.
        Using class.getName() now.

        Also added a few tests for other permutations.

        All coprocessor related test pass for me.

        Show
        Lars Hofhansl added a comment - Just forgot to change the config property that points to the coprocessor class when I changed the name of the test. Using class.getName() now. Also added a few tests for other permutations. All coprocessor related test pass for me.
        Hide
        Lars Hofhansl added a comment -

        I sync'd the latest trunk and now there seems to be additional checking on column families (a put with non-existent CF fails before it even gets to the coprocessor, I assume that's desired).

        But also with the latest changes my own test fails now.
        So it'll be a bit until I track that down.

        Show
        Lars Hofhansl added a comment - I sync'd the latest trunk and now there seems to be additional checking on column families (a put with non-existent CF fails before it even gets to the coprocessor, I assume that's desired). But also with the latest changes my own test fails now. So it'll be a bit until I track that down.
        Hide
        Gary Helmling added a comment -

        No, another patch here is fine. I'm +1 with that change, assuming tests pass.

        Show
        Gary Helmling added a comment - No, another patch here is fine. I'm +1 with that change, assuming tests pass.
        Hide
        Lars Hofhansl added a comment -

        Thanks Gary. I will attach a new patch in a few minutes. Would you prefer a review on review board?

        Show
        Lars Hofhansl added a comment - Thanks Gary. I will attach a new patch in a few minutes. Would you prefer a review on review board?
        Hide
        Gary Helmling added a comment -

        Lars,

        Looks good. My only comment would be to rename the new TestRegionObserver class to something like TestRegionObserverBypass, since that's what it's actually testing and we already have TestRegionObserverInterface.

        Show
        Gary Helmling added a comment - Lars, Looks good. My only comment would be to rename the new TestRegionObserver class to something like TestRegionObserverBypass, since that's what it's actually testing and we already have TestRegionObserverInterface.
        Hide
        Gary Helmling added a comment -

        Hi Lars,

        I took a quick look and it seemed good. I like the additional tests as well.

        I just want to take some time to review the batch put handling in a little more detail. Will dig in tonight.

        Show
        Gary Helmling added a comment - Hi Lars, I took a quick look and it seemed good. I like the additional tests as well. I just want to take some time to review the batch put handling in a little more detail. Will dig in tonight.
        Hide
        Lars Hofhansl added a comment -

        What's the general feeling about this change?

        Show
        Lars Hofhansl added a comment - What's the general feeling about this change?
        Hide
        Lars Hofhansl added a comment -

        Same patch, just with a few more comment.

        Andrew, Gary, or Mingie, I think you'd be the most qualified to look at this.

        Show
        Lars Hofhansl added a comment - Same patch, just with a few more comment. Andrew, Gary, or Mingie, I think you'd be the most qualified to look at this.
        Hide
        Lars Hofhansl added a comment -

        Forgot to add the test to the previous patch.

        Show
        Lars Hofhansl added a comment - Forgot to add the test to the previous patch.
        Hide
        Lars Hofhansl added a comment -

        Patch to fix the issue.
        Folks more familiar with the preprocessor code: Please have a careful look.

        Show
        Lars Hofhansl added a comment - Patch to fix the issue. Folks more familiar with the preprocessor code: Please have a careful look.

          People

          • Assignee:
            Lars Hofhansl
            Reporter:
            Lars Hofhansl
          • Votes:
            0 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development