HBASE-25277 postScannerFilterRow impacts Scan performance a lot in HB…

…ase 2.x
apache · Dec 10, 2020 · 0d0c610 · 0d0c610
1 parent 245b439
commit 0d0c610
Show file tree

Hide file tree

Showing 5 changed files with 89 additions and 32 deletions.
diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/constraint/ConstraintProcessor.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/constraint/ConstraintProcessor.java
@@ -22,20 +22,19 @@
 import java.util.List;
 import java.util.Optional;
 
-import org.apache.yetus.audience.InterfaceAudience;
-import org.slf4j.Logger;
-import org.slf4j.LoggerFactory;
-import org.apache.hadoop.hbase.Cell;
 import org.apache.hadoop.hbase.CoprocessorEnvironment;
-import org.apache.hadoop.hbase.client.Put;
 import org.apache.hadoop.hbase.client.Durability;
+import org.apache.hadoop.hbase.client.Put;
 import org.apache.hadoop.hbase.client.TableDescriptor;
 import org.apache.hadoop.hbase.coprocessor.ObserverContext;
 import org.apache.hadoop.hbase.coprocessor.RegionCoprocessor;
 import org.apache.hadoop.hbase.coprocessor.RegionCoprocessorEnvironment;
 import org.apache.hadoop.hbase.coprocessor.RegionObserver;
-import org.apache.hadoop.hbase.regionserver.InternalScanner;
 import org.apache.hadoop.hbase.wal.WALEdit;
+import org.apache.yetus.audience.InterfaceAudience;
+
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
 
 /***
  * Processes multiple {@link Constraint Constraints} on a given table.
@@ -98,11 +97,4 @@ public void prePut(ObserverContext<RegionCoprocessorEnvironment> e, Put put,
     }
     // if we made it here, then the Put is valid
   }
-
-  @Override
-  public boolean postScannerFilterRow(final ObserverContext<RegionCoprocessorEnvironment> e,
-      final InternalScanner s, final Cell curRowCell, final boolean hasMore) throws IOException {
-    // 'default' in RegionObserver might do unnecessary copy for Off heap backed Cells.
-    return hasMore;
-  }
 }
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
@@ -105,6 +105,13 @@ public class RegionCoprocessorHost
   // optimization: no need to call postScannerFilterRow, if no coprocessor implements it
   private final boolean hasCustomPostScannerFilterRow;
 
+  /*
+   * Whether any configured CPs override postScannerFilterRow hook
+   */
+  public boolean hasCustomPostScannerFilterRow() {
+    return hasCustomPostScannerFilterRow;
+  }
+
   /**
    *
    * Encapsulation of the environment of each coprocessor
@@ -278,11 +285,10 @@ public RegionCoprocessorHost(final HRegion region,
     out: for (RegionCoprocessorEnvironment env: coprocEnvironments) {
       if (env.getInstance() instanceof RegionObserver) {
         Class<?> clazz = env.getInstance().getClass();
-        for(;;) {
-          if (clazz == null) {
-            // we must have directly implemented RegionObserver
-            hasCustomPostScannerFilterRow = true;
-            break out;
+        for (;;) {
+          if (clazz == Object.class) {
+            // we dont need to look postScannerFilterRow into Object class
+            break; // break the inner loop
           }
           try {
             clazz.getDeclaredMethod("postScannerFilterRow", ObserverContext.class,

diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/security/access/AccessController.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/security/access/AccessController.java
@@ -1877,13 +1877,6 @@ public void postScannerClose(final ObserverContext<RegionCoprocessorEnvironment>
     scannerOwners.remove(s);
   }
 
-  @Override
-  public boolean postScannerFilterRow(final ObserverContext<RegionCoprocessorEnvironment> e,
-      final InternalScanner s, final Cell curRowCell, final boolean hasMore) throws IOException {
-    // 'default' in RegionObserver might do unnecessary copy for Off heap backed Cells.
-    return hasMore;
-  }
-
   /**
    * Verify, when servicing an RPC, that the caller is the scanner owner.
    * If so, we assume that access control is correctly enforced based on

diff --git a/...erver/src/main/java/org/apache/hadoop/hbase/security/visibility/VisibilityController.java b/...erver/src/main/java/org/apache/hadoop/hbase/security/visibility/VisibilityController.java
@@ -741,13 +741,6 @@ private Cell createNewCellWithTags(Mutation mutation, Cell newCell) throws IOExc
     return PrivateCellUtil.createCell(newCell, tags);
   }
 
-  @Override
-  public boolean postScannerFilterRow(final ObserverContext<RegionCoprocessorEnvironment> e,
-      final InternalScanner s, final Cell curRowCell, final boolean hasMore) throws IOException {
-    // 'default' in RegionObserver might do unnecessary copy for Off heap backed Cells.
-    return hasMore;
-  }
-
   /****************************** VisibilityEndpoint service related methods ******************************/
   @Override
   public synchronized void addLabels(RpcController controller, VisibilityLabelsRequest request,

diff --git a/...e-server/src/test/java/org/apache/hadoop/hbase/coprocessor/TestRegionCoprocessorHost.java b/...e-server/src/test/java/org/apache/hadoop/hbase/coprocessor/TestRegionCoprocessorHost.java
@@ -22,9 +22,14 @@
 import static org.apache.hadoop.hbase.coprocessor.CoprocessorHost.SKIP_LOAD_DUPLICATE_TABLE_COPROCESSOR;
 import static org.apache.hadoop.hbase.coprocessor.CoprocessorHost.USER_COPROCESSORS_ENABLED_CONF_KEY;
 import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertTrue;
 import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.when;
 
+import java.io.IOException;
+import java.util.Optional;
+
 import org.apache.hadoop.conf.Configuration;
 import org.apache.hadoop.hbase.HBaseClassTestRule;
 import org.apache.hadoop.hbase.HBaseConfiguration;
@@ -37,9 +42,13 @@
 import org.apache.hadoop.hbase.regionserver.RegionCoprocessorHost;
 import org.apache.hadoop.hbase.regionserver.RegionServerServices;
 import org.apache.hadoop.hbase.testclassification.SmallTests;
+
+import org.junit.Before;
 import org.junit.ClassRule;
+import org.junit.Rule;
 import org.junit.Test;
 import org.junit.experimental.categories.Category;
+import org.junit.rules.TestName;
 
 @Category({SmallTests.class})
 public class TestRegionCoprocessorHost {
@@ -48,6 +57,39 @@ public class TestRegionCoprocessorHost {
   public static final HBaseClassTestRule CLASS_RULE =
       HBaseClassTestRule.forClass(TestRegionCoprocessorHost.class);
 
+  @Rule public final TestName name = new TestName();
+  private Configuration conf;
+  private RegionInfo regionInfo;
+  private HRegion region;
+  private RegionServerServices rsServices;
+
+  @Before public void setup() throws IOException {
+    init(null);
+  }
+
+  private void init(Boolean flag) throws IOException {
+    conf = HBaseConfiguration.create();
+    conf.setBoolean(COPROCESSORS_ENABLED_CONF_KEY, true);
+    conf.setBoolean(USER_COPROCESSORS_ENABLED_CONF_KEY, true);
+    TableName tableName = TableName.valueOf(name.getMethodName());
+    regionInfo = RegionInfoBuilder.newBuilder(tableName).build();
+    TableDescriptor tableDesc = null;
+    if (flag == null) {
+      // configure a coprocessor which override postScannerFilterRow
+      tableDesc = TableDescriptorBuilder.newBuilder(tableName)
+          .setCoprocessor(SimpleRegionObserver.class.getName()).build();
+    } else if (flag) {
+      // configure a coprocessor which don't override postScannerFilterRow
+      tableDesc = TableDescriptorBuilder.newBuilder(tableName)
+          .setCoprocessor(TempRegionObserver.class.getName()).build();
+    } else {
+      // configure two coprocessors, one don't override postScannerFilterRow but another one does
+      conf.set(REGION_COPROCESSOR_CONF_KEY, TempRegionObserver.class.getName());
+      tableDesc = TableDescriptorBuilder.newBuilder(tableName)
+          .setCoprocessor(SimpleRegionObserver.class.getName()).build();
+    }
+  }
+
   @Test
   public void testLoadDuplicateCoprocessor() throws Exception {
     Configuration conf = HBaseConfiguration.create();
@@ -74,4 +116,35 @@ public void testLoadDuplicateCoprocessor() throws Exception {
     // Two duplicate coprocessors loaded
     assertEquals(2, host.coprocEnvironments.size());
   }
+
+  @Test
+  public void testPostScannerFilterRow() throws IOException {
+    // By default SimpleRegionObserver is set as region coprocessor which implements
+    // postScannerFilterRow
+    RegionCoprocessorHost host = new RegionCoprocessorHost(region, rsServices, conf);
+    assertTrue("Region coprocessor implement postScannerFilterRow",
+        host.hasCustomPostScannerFilterRow());
+
+    // Set a region CP which doesn't implement postScannerFilterRow
+    init(true);
+    host = new RegionCoprocessorHost(region, rsServices, conf);
+    assertFalse("Region coprocessor implement postScannerFilterRow",
+        host.hasCustomPostScannerFilterRow());
+
+    // Set multiple region CPs, in which one implements postScannerFilterRow
+    init(false);
+    host = new RegionCoprocessorHost(region, rsServices, conf);
+    assertTrue("Region coprocessor doesn't implement postScannerFilterRow",
+        host.hasCustomPostScannerFilterRow());
+  }
+
+  /*
+   * Simple region coprocessor which doesn't override postScannerFilterRow
+   */
+  public static class TempRegionObserver implements RegionCoprocessor, RegionObserver {
+    @Override
+    public Optional<RegionObserver> getRegionObserver() {
+      return Optional.of(this);
+    }
+  }
 }