HBASE-25277 postScannerFilterRow impacts Scan performance a lot in HBase 2.x (#2675)

* HBASE-25277 postScannerFilterRow impacts Scan performance a lot in HBase 2.x

1. Added a check for Object class in RegionCoprocessorHost to avoid wrong initialization of hasCustomPostScannerFilterRow
2. Removed dummy implementation of postScannerFilterRow from AccessController, VisibilityController & ConstraintProcessor (which are not required currently)

Signed-off-by Ramkrishna S Vasudevan <ramkrishna@apache.org>
Signed-off-by Anoop Sam John <anoopsamjohn@apache.org>
Signed-off-by: Duo Zhang <zhangduo@apache.org>

hbase-server/src/main/java/org/apache/hadoop/hbase/constraint/ConstraintProcessor.java

@@ -22,20 +22,19 @@ import java.util.ArrayList;
 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 class ConstraintProcessor implements RegionCoprocessor, RegionObserver {
     }
     // 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;
-  }
 }

hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/RegionCoprocessorHost.java

@@ -79,7 +79,6 @@ import org.apache.hadoop.hbase.wal.WALKey;
 import org.apache.yetus.audience.InterfaceAudience;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
-
 import org.apache.hbase.thirdparty.com.google.protobuf.Message;
 import org.apache.hbase.thirdparty.com.google.protobuf.Service;
 import org.apache.hbase.thirdparty.org.apache.commons.collections4.map.AbstractReferenceMap;
@@ -102,6 +101,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
@@ -276,10 +282,9 @@ public class RegionCoprocessorHost
       if (env.getInstance() instanceof RegionObserver) {
         Class<?> clazz = env.getInstance().getClass();
         for (;;) {
-          if (clazz == null) {
-            // we must have directly implemented RegionObserver
-            hasCustomPostScannerFilterRow = true;
-            break out;
+          if (clazz == Object.class) {
+            // we dont need to look postScannerFilterRow into Object class
+            break; // break the inner loop
           }
           try {
             clazz.getDeclaredMethod("postScannerFilterRow", ObserverContext.class,

hbase-server/src/main/java/org/apache/hadoop/hbase/security/access/AccessController.java

@@ -1848,13 +1848,6 @@ public class AccessController implements MasterCoprocessor, RegionCoprocessor,
     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

hbase-server/src/main/java/org/apache/hadoop/hbase/security/visibility/VisibilityController.java

@@ -678,13 +678,6 @@ public class VisibilityController implements MasterCoprocessor, RegionCoprocesso
     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,

hbase-server/src/test/java/org/apache/hadoop/hbase/coprocessor/TestRegionCoprocessorHost.java

@@ -22,11 +22,14 @@ import static org.apache.hadoop.hbase.coprocessor.CoprocessorHost.REGION_COPROCE
 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.CellComparator;
 import org.apache.hadoop.hbase.HBaseClassTestRule;
@@ -58,7 +61,6 @@ import org.junit.Rule;
 import org.junit.Test;
 import org.junit.experimental.categories.Category;
 import org.junit.rules.TestName;
-import java.io.IOException;
 
 @Category({SmallTests.class})
 public class TestRegionCoprocessorHost {
@@ -79,19 +81,36 @@ public class TestRegionCoprocessorHost {
 
   @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();
-    // config a same coprocessor with system coprocessor
-    TableDescriptor tableDesc = TableDescriptorBuilder.newBuilder(tableName)
+    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();
+    }
     region = mock(HRegion.class);
     when(region.getRegionInfo()).thenReturn(regionInfo);
     when(region.getTableDescriptor()).thenReturn(tableDesc);
     rsServices = mock(RegionServerServices.class);
   }
+
   @Test
   public void testLoadDuplicateCoprocessor() throws Exception {
     conf.setBoolean(SKIP_LOAD_DUPLICATE_TABLE_COPROCESSOR, true);
@@ -158,6 +177,27 @@ public class TestRegionCoprocessorHost {
     verifyScanInfo(newScanInfo);
   }
 
+  @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());
+  }
+
   private void verifyScanInfo(ScanInfo newScanInfo) {
     assertEquals(KeepDeletedCells.TRUE, newScanInfo.getKeepDeletedCells());
     assertEquals(MAX_VERSIONS, newScanInfo.getMaxVersions());
@@ -175,4 +215,13 @@ public class TestRegionCoprocessorHost {
       CellComparator.getInstance(), true);
   }
 
+  /*
+   * Simple region coprocessor which doesn't override postScannerFilterRow
+   */
+  public static class TempRegionObserver implements RegionCoprocessor, RegionObserver {
+    @Override
+    public Optional<RegionObserver> getRegionObserver() {
+      return Optional.of(this);
+    }
+  }
 }