From fb6e498b32e48aa606ef5427013fd84452cc762f Mon Sep 17 00:00:00 2001 From: Pankaj Date: Mon, 7 Dec 2020 23:00:48 +0530 Subject: [PATCH] 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 Signed-off-by Anoop Sam John Signed-off-by: Duo Zhang --- .../hbase/constraint/ConstraintProcessor.java | 18 ++---- .../regionserver/RegionCoprocessorHost.java | 17 ++++-- .../security/access/AccessController.java | 7 --- .../visibility/VisibilityController.java | 7 --- .../TestRegionCoprocessorHost.java | 57 +++++++++++++++++-- 5 files changed, 69 insertions(+), 37 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 index 6aa5d977b67..b0a04c5044a 100644 --- 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.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 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 index 5ebf7e1c159..7ed23f695ec 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 @@ -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 @@ -275,11 +281,10 @@ public class RegionCoprocessorHost 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 index 3a6c3aae657..75bc73ccdcd 100644 --- 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 @@ -1848,13 +1848,6 @@ public class AccessController implements MasterCoprocessor, RegionCoprocessor, scannerOwners.remove(s); } - @Override - public boolean postScannerFilterRow(final ObserverContext 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/hbase-server/src/main/java/org/apache/hadoop/hbase/security/visibility/VisibilityController.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/security/visibility/VisibilityController.java index 37f25a83ea7..7c4b7abb8bf 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/security/visibility/VisibilityController.java +++ b/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 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/hbase-server/src/test/java/org/apache/hadoop/hbase/coprocessor/TestRegionCoprocessorHost.java b/hbase-server/src/test/java/org/apache/hadoop/hbase/coprocessor/TestRegionCoprocessorHost.java index 423a412f75c..b0188d9b7ce 100644 --- a/hbase-server/src/test/java/org/apache/hadoop/hbase/coprocessor/TestRegionCoprocessorHost.java +++ b/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) - .setCoprocessor(SimpleRegionObserver.class.getName()).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(); + } 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 getRegionObserver() { + return Optional.of(this); + } + } }