From 13ee2128519a59000714736c32e53c8ff9f5181e Mon Sep 17 00:00:00 2001 From: Michael Stack Date: Mon, 4 Dec 2017 22:29:27 -0800 Subject: [PATCH] HBASE-19384 Results returned by preAppend hook in a coprocessor are replaced with null from other coprocessor even on bypass If 'bypass' is set by a Coprocessor, skip out on calling any subsequent Coprocessors that might be chained to a bypassable method. This patch restores some of the 'complete' behavior removed by HBASE-19123 only 'bypass' now triggers 'complete'. --- .../hadoop/hbase/coprocessor/CoprocessorHost.java | 5 ++ .../hadoop/hbase/coprocessor/ObserverContext.java | 5 ++ .../hadoop/hbase/coprocessor/RegionObserver.java | 46 ++++++++++------ .../coprocessor/TestRegionObserverBypass.java | 62 +++++++++++++++++++++- 4 files changed, 102 insertions(+), 16 deletions(-) diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/coprocessor/CoprocessorHost.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/coprocessor/CoprocessorHost.java index 10e569b34a..f2d9b2a877 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/coprocessor/CoprocessorHost.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/coprocessor/CoprocessorHost.java @@ -678,6 +678,11 @@ public abstract class CoprocessorHost { * that the replacement for the bypassed code takes care of all necessary * skipped concerns. Because those concerns can change at any point, such an * assumption is never safe.

+ *

As of hbase2, when bypass has been set, we will NOT call any Coprocessors follow the + * bypassing Coprocessor; we cut short the processing and return the bypassing Coprocessors + * response (this used be a separate 'complete' option that has been folded into the + * 'bypass' in hbase2.

*/ void bypass(); + /** * Returns the active user for the coprocessor call. If an explicit {@code User} instance was * provided to the constructor, that will be returned, otherwise if we are in the context of an diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/coprocessor/RegionObserver.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/coprocessor/RegionObserver.java index 7ac0a7e7ae..e036441083 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/coprocessor/RegionObserver.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/coprocessor/RegionObserver.java @@ -220,6 +220,7 @@ public interface RegionObserver { * of candidates. If you remove all the candidates then the compaction will be canceled. *

Supports Coprocessor 'bypass' -- 'bypass' is how this method indicates that it changed * the passed in candidates. + * If 'bypass' is set, we skip out on calling any subsequent chained coprocessors. * @param c the environment provided by the region server * @param store the store where compaction is being requested * @param candidates the store files currently available for compaction @@ -309,7 +310,8 @@ public interface RegionObserver { /** * Called before the client performs a Get *

- * Call CoprocessorEnvironment#bypass to skip default actions + * Call CoprocessorEnvironment#bypass to skip default actions. + * If 'bypass' is set, we skip out on calling any subsequent chained coprocessors. * @param c the environment provided by the region server * @param get the Get request * @param result The result to return to the client if default processing @@ -334,7 +336,8 @@ public interface RegionObserver { /** * Called before the client tests for existence using a Get. *

- * Call CoprocessorEnvironment#bypass to skip default actions + * Call CoprocessorEnvironment#bypass to skip default actions. + * If 'bypass' is set, we skip out on calling any subsequent chained coprocessors. * @param c the environment provided by the region server * @param get the Get request * @param exists the result returned by the region server @@ -360,7 +363,8 @@ public interface RegionObserver { /** * Called before the client stores a value. *

- * Call CoprocessorEnvironment#bypass to skip default actions + * Call CoprocessorEnvironment#bypass to skip default actions. + * If 'bypass' is set, we skip out on calling any subsequent chained coprocessors. *

* Note: Do not retain references to any Cells in 'put' beyond the life of this invocation. * If need a Cell reference for later use, copy the cell and use that. @@ -388,7 +392,8 @@ public interface RegionObserver { /** * Called before the client deletes a value. *

- * Call CoprocessorEnvironment#bypass to skip default actions + * Call CoprocessorEnvironment#bypass to skip default actions. + * If 'bypass' is set, we skip out on calling any subsequent chained coprocessors. *

* Note: Do not retain references to any Cells in 'delete' beyond the life of this invocation. * If need a Cell reference for later use, copy the cell and use that. @@ -403,7 +408,8 @@ public interface RegionObserver { /** * Called before the server updates the timestamp for version delete with latest timestamp. *

- * Call CoprocessorEnvironment#bypass to skip default actions + * Call CoprocessorEnvironment#bypass to skip default actions. + * If 'bypass' is set, we skip out on calling any subsequent chained coprocessors. * @param c the environment provided by the region server * @param mutation - the parent mutation associated with this delete cell * @param cell - The deleteColumn with latest version cell @@ -495,7 +501,8 @@ public interface RegionObserver { /** * Called before checkAndPut. *

- * Call CoprocessorEnvironment#bypass to skip default actions + * Call CoprocessorEnvironment#bypass to skip default actions. + * If 'bypass' is set, we skip out on calling any subsequent chained coprocessors. *

* Note: Do not retain references to any Cells in 'put' beyond the life of this invocation. * If need a Cell reference for later use, copy the cell and use that. @@ -523,7 +530,8 @@ public interface RegionObserver { * Row will be locked for longer time. Trying to acquire lock on another row, within this, * can lead to potential deadlock. *

- * Call CoprocessorEnvironment#bypass to skip default actions + * Call CoprocessorEnvironment#bypass to skip default actions. + * If 'bypass' is set, we skip out on calling any subsequent chained coprocessors. *

* Note: Do not retain references to any Cells in 'put' beyond the life of this invocation. * If need a Cell reference for later use, copy the cell and use that. @@ -568,7 +576,8 @@ public interface RegionObserver { /** * Called before checkAndDelete. *

- * Call CoprocessorEnvironment#bypass to skip default actions + * Call CoprocessorEnvironment#bypass to skip default actions. + * If 'bypass' is set, we skip out on calling any subsequent chained coprocessors. *

* Note: Do not retain references to any Cells in 'delete' beyond the life of this invocation. * If need a Cell reference for later use, copy the cell and use that. @@ -595,7 +604,8 @@ public interface RegionObserver { * Row will be locked for longer time. Trying to acquire lock on another row, within this, * can lead to potential deadlock. *

- * Call CoprocessorEnvironment#bypass to skip default actions + * Call CoprocessorEnvironment#bypass to skip default actions. + * If 'bypass' is set, we skip out on calling any subsequent chained coprocessors. *

* Note: Do not retain references to any Cells in 'delete' beyond the life of this invocation. * If need a Cell reference for later use, copy the cell and use that. @@ -639,7 +649,8 @@ public interface RegionObserver { /** * Called before Append. *

- * Call CoprocessorEnvironment#bypass to skip default actions + * Call CoprocessorEnvironment#bypass to skip default actions. + * If 'bypass' is set, we skip out on calling any subsequent chained coprocessors. *

* Note: Do not retain references to any Cells in 'append' beyond the life of this invocation. * If need a Cell reference for later use, copy the cell and use that. @@ -659,7 +670,8 @@ public interface RegionObserver { * Row will be locked for longer time. Trying to acquire lock on another row, within this, * can lead to potential deadlock. *

- * Call CoprocessorEnvironment#bypass to skip default actions + * Call CoprocessorEnvironment#bypass to skip default actions. + * If 'bypass' is set, we skip out on calling any subsequent chained coprocessors. *

* Note: Do not retain references to any Cells in 'append' beyond the life of this invocation. * If need a Cell reference for later use, copy the cell and use that. @@ -690,7 +702,8 @@ public interface RegionObserver { /** * Called before Increment. *

- * Call CoprocessorEnvironment#bypass to skip default actions + * Call CoprocessorEnvironment#bypass to skip default actions. + * If 'bypass' is set, we skip out on calling any subsequent chained coprocessors. *

* Note: Do not retain references to any Cells in 'increment' beyond the life of this invocation. * If need a Cell reference for later use, copy the cell and use that. @@ -710,7 +723,8 @@ public interface RegionObserver { * Row will be locked for longer time. Trying to acquire lock on another row, within this, * can lead to potential deadlock. *

- * Call CoprocessorEnvironment#bypass to skip default actions + * Call CoprocessorEnvironment#bypass to skip default actions. + * If 'bypass' is set, we skip out on calling any subsequent chained coprocessors. *

* Note: Do not retain references to any Cells in 'increment' beyond the life of this invocation. * If need a Cell reference for later use, copy the cell and use that. @@ -772,7 +786,8 @@ public interface RegionObserver { /** * Called before the client asks for the next row on a scanner. *

- * Call CoprocessorEnvironment#bypass to skip default actions + * Call CoprocessorEnvironment#bypass to skip default actions. + * If 'bypass' is set, we skip out on calling any subsequent chained coprocessors. *

* Note: Do not retain references to any Cells returned by scanner, beyond the life of this * invocation. If need a Cell reference for later use, copy the cell and use that. @@ -836,7 +851,8 @@ public interface RegionObserver { /** * Called before the client closes a scanner. *

- * Call CoprocessorEnvironment#bypass to skip default actions + * Call CoprocessorEnvironment#bypass to skip default actions. + * If 'bypass' is set, we skip out on calling any subsequent chained coprocessors. * @param c the environment provided by the region server * @param s the scanner */ diff --git a/hbase-server/src/test/java/org/apache/hadoop/hbase/coprocessor/TestRegionObserverBypass.java b/hbase-server/src/test/java/org/apache/hadoop/hbase/coprocessor/TestRegionObserverBypass.java index 050ea36fde..ad45ef630f 100644 --- a/hbase-server/src/test/java/org/apache/hadoop/hbase/coprocessor/TestRegionObserverBypass.java +++ b/hbase-server/src/test/java/org/apache/hadoop/hbase/coprocessor/TestRegionObserverBypass.java @@ -25,6 +25,7 @@ import java.util.ArrayList; import java.util.List; import java.util.Map; import java.util.Optional; +import java.util.concurrent.atomic.AtomicInteger; import org.apache.hadoop.conf.Configuration; import org.apache.hadoop.hbase.Cell; @@ -63,9 +64,12 @@ public class TestRegionObserverBypass { @BeforeClass public static void setUpBeforeClass() throws Exception { + // Stack up three coprocessors just so I can check bypass skips subsequent calls. Configuration conf = HBaseConfiguration.create(); conf.setStrings(CoprocessorHost.USER_REGION_COPROCESSOR_CONF_KEY, - TestCoprocessor.class.getName()); + new String [] {TestCoprocessor.class.getName(), + TestCoprocessor2.class.getName(), + TestCoprocessor3.class.getName()}); util = new HBaseTestingUtility(conf); util.startMiniCluster(); } @@ -85,6 +89,8 @@ public class TestRegionObserverBypass { admin.deleteTable(tableName); } util.createTable(tableName, new byte[][] {dummy, test}); + TestCoprocessor.PREPUT_BYPASSES.set(0); + TestCoprocessor.PREPUT_INVOCATIONS.set(0); } /** @@ -104,6 +110,7 @@ public class TestRegionObserverBypass { /** * Test various multiput operations. + * If the column family is 'test', then bypass is invoked. * @throws Exception */ @Test @@ -205,7 +212,48 @@ public class TestRegionObserverBypass { t.delete(d); } + /** + * Test that when bypass is called, we skip out calling any other coprocessors stacked up method, + * in this case, a prePut. + * If the column family is 'test', then bypass is invoked. + */ + @Test + public void testBypassAlsoCompletes() throws IOException { + //ensure that server time increments every time we do an operation, otherwise + //previous deletes will eclipse successive puts having the same timestamp + EnvironmentEdgeManagerTestHelper.injectEdge(new IncrementingEnvironmentEdge()); + + Table t = util.getConnection().getTable(tableName); + List puts = new ArrayList<>(); + Put p = new Put(row1); + p.addColumn(dummy, dummy, dummy); + puts.add(p); + p = new Put(row2); + p.addColumn(test, dummy, dummy); + puts.add(p); + p = new Put(row3); + p.addColumn(test, dummy, dummy); + puts.add(p); + t.put(puts); + // Ensure expected result. + checkRowAndDelete(t,row1,1); + checkRowAndDelete(t,row2,0); + checkRowAndDelete(t,row3,0); + // We have three Coprocessors stacked up on the prePut. See the beforeClass setup. We did three + // puts above two of which bypassed. A bypass means do not call the other coprocessors in the + // stack so for the two 'test' calls in the above, we should not have call through to all all + // three coprocessors in the chain. So we should have: + // 3 invocations for first put + 1 invocation + 1 bypass for second put + 1 invocation + + // 1 bypass for the last put. Assert. + assertEquals("Total CP invocation count", 5, TestCoprocessor.PREPUT_INVOCATIONS.get()); + assertEquals("Total CP bypasses", 2, TestCoprocessor.PREPUT_BYPASSES.get()); + } + + public static class TestCoprocessor implements RegionCoprocessor, RegionObserver { + static AtomicInteger PREPUT_INVOCATIONS = new AtomicInteger(0); + static AtomicInteger PREPUT_BYPASSES = new AtomicInteger(0); + @Override public Optional getRegionObserver() { return Optional.of(this); @@ -215,10 +263,22 @@ public class TestRegionObserverBypass { public void prePut(final ObserverContext e, final Put put, final WALEdit edit, final Durability durability) throws IOException { + PREPUT_INVOCATIONS.incrementAndGet(); Map> familyMap = put.getFamilyCellMap(); if (familyMap.containsKey(test)) { + PREPUT_BYPASSES.incrementAndGet(); e.bypass(); } } } + + /** + * Calls through to TestCoprocessor. + */ + public static class TestCoprocessor2 extends TestRegionObserverBypass.TestCoprocessor {} + + /** + * Calls through to TestCoprocessor. + */ + public static class TestCoprocessor3 extends TestRegionObserverBypass.TestCoprocessor {} } -- 2.11.0 (Apple Git-81)