From 3ed778a9ff4d947f85d6abe65dcebf46af239dc1 Mon Sep 17 00:00:00 2001 From: Michael Stack Date: Tue, 24 Oct 2017 21:33:44 -0700 Subject: [PATCH] HBASE-18770 Remove bypass method in ObserverContext and implement the 'bypass' logic case by case First cut. See RegionObserver on how it does bypass by method. --- .../hadoop/hbase/coprocessor/CoprocessorHost.java | 31 +++--- .../hadoop/hbase/coprocessor/ObserverContext.java | 7 -- .../hbase/coprocessor/ObserverContextImpl.java | 19 ---- .../hadoop/hbase/coprocessor/RegionObserver.java | 117 +++++++++++++-------- .../hbase/regionserver/RegionCoprocessorHost.java | 4 +- 5 files changed, 85 insertions(+), 93 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 c785b0b594..21970a45d2 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 @@ -36,14 +36,12 @@ import java.util.function.Function; import org.apache.commons.logging.Log; import org.apache.commons.logging.LogFactory; import org.apache.yetus.audience.InterfaceAudience; -import org.apache.yetus.audience.InterfaceStability; import org.apache.hadoop.conf.Configuration; import org.apache.hadoop.fs.Path; import org.apache.hadoop.hbase.Abortable; import org.apache.hadoop.hbase.Coprocessor; import org.apache.hadoop.hbase.CoprocessorEnvironment; import org.apache.hadoop.hbase.DoNotRetryIOException; -import org.apache.hadoop.hbase.HBaseInterfaceAudience; import org.apache.hadoop.hbase.HConstants; import org.apache.hadoop.hbase.ipc.RpcServer; import org.apache.hadoop.hbase.security.User; @@ -631,28 +629,25 @@ public abstract class CoprocessorHost R execOperationWithResult(final boolean ifBypass, final R defaultValue, final ObserverOperationWithResult observerOperation) throws IOException { if (observerOperation == null) { return ifBypass ? null : defaultValue; - } else { - observerOperation.setResult(defaultValue); - boolean bypass = execOperation(true, observerOperation); - R result = observerOperation.getResult(); - return bypass == ifBypass ? result : null; } + observerOperation.setResult(defaultValue); + execOperation(true, observerOperation); + return observerOperation.getResult(); } - protected boolean execOperation(final ObserverOperation observerOperation) + protected void execOperation(final ObserverOperation observerOperation) throws IOException { - return execOperation(true, observerOperation); + execOperation(true, observerOperation); } - protected boolean execOperation(final boolean earlyExit, + protected void execOperation(final boolean earlyExit, final ObserverOperation observerOperation) throws IOException { - if (observerOperation == null) return false; - boolean bypass = false; + if (observerOperation == null) return; List envs = coprocEnvironments.get(); for (E env : envs) { observerOperation.prepare(env); @@ -666,13 +661,12 @@ public abstract class CoprocessorHost boolean execShutdown(final ObserverOperation observerOperation) + protected void execShutdown(final ObserverOperation observerOperation) throws IOException { - if (observerOperation == null) return false; + if (observerOperation == null) return; boolean bypass = false; List envs = coprocEnvironments.get(); // Iterate the coprocessors and execute ObserverOperation's call() @@ -706,7 +700,6 @@ public abstract class CoprocessorHost { E getEnvironment(); /** - * Call to indicate that the current coprocessor's return value should be - * used in place of the normal HBase obtained value. - */ - void bypass(); - - /** * Call to indicate that additional coprocessors further down the execution * chain do not need to be invoked. Implies that this coprocessor's response * is definitive. @@ -60,5 +54,4 @@ public interface ObserverContext { * context. */ Optional getCaller(); - } diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/coprocessor/ObserverContextImpl.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/coprocessor/ObserverContextImpl.java index ff829564b6..b1eeabd8ed 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/coprocessor/ObserverContextImpl.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/coprocessor/ObserverContextImpl.java @@ -21,11 +21,9 @@ import java.util.Optional; import com.google.common.annotations.VisibleForTesting; import org.apache.hadoop.hbase.CoprocessorEnvironment; -import org.apache.hadoop.hbase.HBaseInterfaceAudience; import org.apache.hadoop.hbase.ipc.RpcServer; import org.apache.hadoop.hbase.security.User; import org.apache.yetus.audience.InterfaceAudience; -import org.apache.yetus.audience.InterfaceStability; /** * This is the only implementation of {@link ObserverContext}, which serves as the interface for @@ -34,7 +32,6 @@ import org.apache.yetus.audience.InterfaceStability; @InterfaceAudience.Private public class ObserverContextImpl implements ObserverContext { private E env; - private boolean bypass; private boolean complete; private final User caller; @@ -50,27 +47,11 @@ public class ObserverContextImpl implements Ob this.env = env; } - public void bypass() { - bypass = true; - } - public void complete() { complete = true; } /** - * @return {@code true}, if {@link ObserverContext#bypass()} was called by one of the loaded - * coprocessors, {@code false} otherwise. - */ - public boolean shouldBypass() { - if (bypass) { - bypass = false; - return true; - } - return false; - } - - /** * @return {@code true}, if {@link ObserverContext#complete()} was called by one of the loaded * coprocessors, {@code false} otherwise. */ 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 5c89149fa4..6a21effe6a 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 @@ -22,6 +22,7 @@ package org.apache.hadoop.hbase.coprocessor; import java.io.IOException; import java.util.List; import java.util.Map; +import java.util.Optional; import org.apache.hadoop.fs.FileSystem; import org.apache.hadoop.fs.Path; @@ -129,8 +130,8 @@ public interface RegionObserver { /** * Called before a Store's memstore is flushed to disk. * @param c the environment provided by the region server - * @param store the store where compaction is being requested - * @param scanner the scanner over existing data used in the store file + * @param store the store where the flush is being requested + * @param scanner the scanner over the MemStore used in the store * @return the scanner to use during compaction. Should not be {@code null} * unless the implementation is writing new store files on its own. */ @@ -234,8 +235,6 @@ public interface RegionObserver { /** * Called before the client performs a Get *

- * Call CoprocessorEnvironment#bypass to skip default actions - *

* Call CoprocessorEnvironment#complete to skip any subsequent chained * coprocessors * @param c the environment provided by the region server @@ -243,9 +242,13 @@ public interface RegionObserver { * @param result The result to return to the client if default processing * is bypassed. Can be modified. Will not be used if default processing * is not bypassed. + * @return True is we are to bypass further processing and use the result + * instead (Note Metrics will be incremented as though the operation was not bypassed). */ - default void preGetOp(ObserverContext c, Get get, List result) - throws IOException {} + default boolean preGetOp(ObserverContext c, Get get, + List result) throws IOException { + return false; + } /** * Called after the client performs a Get @@ -271,12 +274,13 @@ public interface RegionObserver { * coprocessors * @param c the environment provided by the region server * @param get the Get request - * @param exists the result returned by the region server - * @return the value to return to the client if bypassing default processing + * @return True or False if we are to bypass further processing and use this result else return + * null to continue processing (Note Metrics will be incremented as though the operation was not + * bypassed). */ - default boolean preExists(ObserverContext c, Get get, - boolean exists) throws IOException { - return exists; + default Optional preExists(ObserverContext c, Get get, + List result) throws IOException { + return Optional.empty(); } /** @@ -308,9 +312,13 @@ public interface RegionObserver { * @param put The Put object * @param edit The WALEdit object that will be written to the wal * @param durability Persistence guarantee for this Put + * @return True if we are to bypass further processing (Note Metrics will be incremented as though + * the operation was not bypassed). */ - default void prePut(ObserverContext c, Put put, WALEdit edit, - Durability durability) throws IOException {} + default boolean prePut(ObserverContext c, Put put, WALEdit edit, + Durability durability) throws IOException { + return false; + } /** * Called after the client stores a value. @@ -342,9 +350,13 @@ public interface RegionObserver { * @param delete The Delete object * @param edit The WALEdit object for the wal * @param durability Persistence guarantee for this Delete + * @return True if we are to bypass further processing (Note Metrics will be incremented as though + * the operation was not bypassed). */ - default void preDelete(ObserverContext c, Delete delete, - WALEdit edit, Durability durability) throws IOException {} + default boolean preDelete(ObserverContext c, Delete delete, + WALEdit edit, Durability durability) throws IOException { + return false; + } /** * Called before the server updates the timestamp for version delete with latest timestamp. @@ -358,9 +370,13 @@ public interface RegionObserver { * @param byteNow - timestamp bytes * @param get - the get formed using the current cell's row. Note that the get does not specify * the family and qualifier + * @return True if we are to bypass timestamp setting (presumed done by Coprocessor). */ - default void prePrepareTimeStampForDeleteVersion(ObserverContext c, - Mutation mutation, Cell cell, byte[] byteNow, Get get) throws IOException {} + // TODO: Should we allow bypass on this one? + default boolean prePrepareTimeStampForDeleteVersion(ObserverContext c, + Mutation mutation, Cell cell, byte[] byteNow, Get get) throws IOException { + return false; + } /** * Called after the client deletes a value. @@ -389,9 +405,13 @@ public interface RegionObserver { * If need a Cell reference for later use, copy the cell and use that. * @param c the environment provided by the region server * @param miniBatchOp batch of Mutations getting applied to region. + * @return True if we are to bypass further processing (Note Metrics will be incremented as though + * the operation was not bypassed). */ - default void preBatchMutate(ObserverContext c, - MiniBatchOperationInProgress miniBatchOp) throws IOException {} + default boolean preBatchMutate(ObserverContext c, + MiniBatchOperationInProgress miniBatchOp) throws IOException { + return false; + } /** * This will be called after applying a batch of Mutations on a region. The Mutations are added to @@ -456,13 +476,14 @@ public interface RegionObserver { * @param comparator the comparator * @param put data to put if check succeeds * @param result - * @return the return value to return to client if bypassing default - * processing + * @return True or False if we are to bypass further processing and use this result else return + * null to continue processing (Note Metrics will be incremented as though the operation was not + * bypassed). */ - default boolean preCheckAndPut(ObserverContext c, byte[] row, - byte[] family, byte[] qualifier, CompareOperator op, ByteArrayComparable comparator, Put put, - boolean result) throws IOException { - return result; + default Optional preCheckAndPut(ObserverContext c, + byte[] row, byte[] family, byte[] qualifier, CompareOperator op, + ByteArrayComparable comparator, Put put, boolean result) throws IOException { + return Optional.empty(); } /** @@ -487,13 +508,14 @@ public interface RegionObserver { * @param comparator the comparator * @param put data to put if check succeeds * @param result - * @return the return value to return to client if bypassing default - * processing + * @return True or False if we are to bypass further processing and use this result else return + * null to continue processing (Note Metrics will be incremented as though the operation was not + * bypassed). */ - default boolean preCheckAndPutAfterRowLock(ObserverContext c, + default Optional preCheckAndPutAfterRowLock(ObserverContext c, byte[] row, byte[] family, byte[] qualifier, CompareOperator op, ByteArrayComparable comparator, Put put, boolean result) throws IOException { - return result; + return Optional.empty(); } /** @@ -538,12 +560,14 @@ public interface RegionObserver { * @param comparator the comparator * @param delete delete to commit if check succeeds * @param result - * @return the value to return to client if bypassing default processing + * @return True or False if we are to bypass further processing and use this result else return + * null to continue processing (Note Metrics will be incremented as though the operation was not + * bypassed). */ - default boolean preCheckAndDelete(ObserverContext c, byte[] row, - byte[] family, byte[] qualifier, CompareOperator op, ByteArrayComparable comparator, - Delete delete, boolean result) throws IOException { - return result; + default Optional preCheckAndDelete(ObserverContext c, + byte[] row, byte[] family, byte[] qualifier, CompareOperator op, + ByteArrayComparable comparator, Delete delete, boolean result) throws IOException { + return Optional.empty(); } /** @@ -568,12 +592,14 @@ public interface RegionObserver { * @param comparator the comparator * @param delete delete to commit if check succeeds * @param result - * @return the value to return to client if bypassing default processing + * @return True or False if we are to bypass further processing and use this result else return + * null to continue processing (Note Metrics will be incremented as though the operation was not + * bypassed). */ - default boolean preCheckAndDeleteAfterRowLock(ObserverContext c, + default Optional preCheckAndDeleteAfterRowLock(ObserverContext c, byte[] row, byte[] family, byte[] qualifier, CompareOperator op, ByteArrayComparable comparator, Delete delete, boolean result) throws IOException { - return result; + return Optional.empty(); } /** @@ -613,6 +639,7 @@ public interface RegionObserver { * @param c the environment provided by the region server * @param append Append object * @return result to return to the client if bypassing default processing + * (Note Metrics will be incremented as though the operation was not bypassed). */ default Result preAppend(ObserverContext c, Append append) throws IOException { @@ -636,6 +663,7 @@ public interface RegionObserver { * @param c the environment provided by the region server * @param append Append object * @return result to return to the client if bypassing default processing + * (Note Metrics will be incremented as though the operation was not bypassed). */ default Result preAppendAfterRowLock(ObserverContext c, Append append) throws IOException { @@ -673,6 +701,7 @@ public interface RegionObserver { * @param c the environment provided by the region server * @param increment increment object * @return result to return to the client if bypassing default processing + * (Note Metrics will be incremented as though the operation was not bypassed). */ default Result preIncrement(ObserverContext c, Increment increment) throws IOException { @@ -699,6 +728,7 @@ public interface RegionObserver { * increment object * @return result to return to the client if bypassing default processing * if an error occurred on the coprocessor + * (Note Metrics will be incremented as though the operation was not bypassed). */ default Result preIncrementAfterRowLock(ObserverContext c, Increment increment) throws IOException { @@ -739,6 +769,7 @@ public interface RegionObserver { * @return an RegionScanner instance to use instead of the base scanner if * overriding default behavior, null otherwise */ + // TODO: The return here is going away? default RegionScanner preScannerOpen(ObserverContext c, Scan scan, RegionScanner s) throws IOException { return s; @@ -779,7 +810,8 @@ public interface RegionObserver { * is not bypassed. * @param limit the maximum number of results to return * @param hasNext the 'has more' indication - * @return 'has more' indication that should be sent to client + * @return 'has more' indication that should be sent to client. This is bypass. This is how it + * is interpreted: return bypass == null ? false : bypass; */ default boolean preScannerNext(ObserverContext c, InternalScanner s, List result, int limit, boolean hasNext) throws IOException { @@ -799,7 +831,7 @@ public interface RegionObserver { * @param result the result to return to the client, can be modified * @param limit the maximum number of results to return * @param hasNext the 'has more' indication - * @return 'has more' indication that should be sent to client + * @return 'has more' indication that should be sent to client. */ default boolean postScannerNext(ObserverContext c, InternalScanner s, List result, int limit, boolean hasNext) throws IOException { @@ -858,8 +890,6 @@ public interface RegionObserver { /** * Called before replaying WALs for this region. - * Calling {@link org.apache.hadoop.hbase.coprocessor.ObserverContext#bypass()} has no - * effect in this hook. * @param ctx the environment provided by the region server * @param info the RegionInfo for this region * @param edits the file of recovered edits @@ -948,9 +978,6 @@ public interface RegionObserver { /** * Called before creation of Reader for a store file. - * Calling {@link org.apache.hadoop.hbase.coprocessor.ObserverContext#bypass()} has no - * effect in this hook. - * * @param ctx the environment provided by the region server * @param fs fileystem to read from * @param p path to the file @@ -998,8 +1025,6 @@ public interface RegionObserver { /** * Called after a new cell has been created during an increment operation, but before * it is committed to the WAL or memstore. - * Calling {@link org.apache.hadoop.hbase.coprocessor.ObserverContext#bypass()} has no - * effect in this hook. * @param ctx the environment provided by the region server * @param opType the operation type * @param mutation the current mutation diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/RegionCoprocessorHost.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/RegionCoprocessorHost.java index 735d7ba6ec..db7617ab31 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/RegionCoprocessorHost.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/RegionCoprocessorHost.java @@ -586,9 +586,9 @@ public class RegionCoprocessorHost * @return If {@code true}, skip the normal selection process and use the current list * @throws IOException */ - public boolean preCompactSelection(final HStore store, final List candidates, + public void preCompactSelection(final HStore store, final List candidates, final CompactionLifeCycleTracker tracker, final User user) throws IOException { - return execOperation(coprocEnvironments.isEmpty() ? null : new RegionObserverOperation(user) { + execOperation(coprocEnvironments.isEmpty() ? null : new RegionObserverOperation(user) { @Override public void call(RegionObserver observer) throws IOException { observer.preCompactSelection(this, store, candidates, tracker); -- 2.11.0 (Apple Git-81)