From d9a32f940545679e07c87a4e479024a2aa9be61c Mon Sep 17 00:00:00 2001 From: chenglei Date: Fri, 1 Apr 2022 11:53:33 +0800 Subject: [PATCH] HBASE-26811 Secondary replica may be disabled for read incorrectly forever (#4310) --- .../hbase/client/TableDescriptorBuilder.java | 6 +- .../hadoop/hbase/regionserver/HRegion.java | 8 ++ .../hbase/regionserver/HRegionServer.java | 8 +- ...tRegionReplicaWaitForPrimaryFlushConf.java | 125 ++++++++++++++++++ 4 files changed, 141 insertions(+), 6 deletions(-) create mode 100644 hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestRegionReplicaWaitForPrimaryFlushConf.java diff --git a/hbase-client/src/main/java/org/apache/hadoop/hbase/client/TableDescriptorBuilder.java b/hbase-client/src/main/java/org/apache/hadoop/hbase/client/TableDescriptorBuilder.java index 1f33b062a1e..e3795c868f6 100644 --- a/hbase-client/src/main/java/org/apache/hadoop/hbase/client/TableDescriptorBuilder.java +++ b/hbase-client/src/main/java/org/apache/hadoop/hbase/client/TableDescriptorBuilder.java @@ -1336,11 +1336,7 @@ public class TableDescriptorBuilder { * @return the modifyable TD */ public ModifyableTableDescriptor setRegionMemStoreReplication(boolean memstoreReplication) { - setValue(REGION_MEMSTORE_REPLICATION_KEY, Boolean.toString(memstoreReplication)); - // If the memstore replication is setup, we do not have to wait for observing a flush event - // from primary before starting to serve reads, because gaps from replication is not applicable - return setValue(REGION_REPLICA_WAIT_FOR_PRIMARY_FLUSH_CONF_KEY, - Boolean.toString(memstoreReplication)); + return setValue(REGION_MEMSTORE_REPLICATION_KEY, Boolean.toString(memstoreReplication)); } public ModifyableTableDescriptor setPriority(int priority) { diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java index dab7d14108c..de4b9ed1e1e 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java @@ -20,6 +20,8 @@ package org.apache.hadoop.hbase.regionserver; import static org.apache.hadoop.hbase.HConstants.REPLICATION_SCOPE_LOCAL; import static org.apache.hadoop.hbase.regionserver.HStoreFile.MAJOR_COMPACTION_KEY; import static org.apache.hadoop.hbase.util.ConcurrentMapUtils.computeIfAbsent; + +import com.google.errorprone.annotations.RestrictedApi; import edu.umd.cs.findbugs.annotations.Nullable; import java.io.EOFException; import java.io.FileNotFoundException; @@ -9422,4 +9424,10 @@ public class HRegion implements HeapSize, PropagatingConfigurationObserver, Regi public void setWriteRequestsCount(long writeRequestsCount) { this.writeRequestsCount.add(writeRequestsCount); } + + @RestrictedApi(explanation = "Should only be called in tests", link = "", + allowedOnPath = ".*/src/test/.*") + boolean isReadsEnabled() { + return this.writestate.readsEnabled; + } } diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java index c87a1e2f22b..8bdab44699e 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java @@ -2518,7 +2518,13 @@ public class HRegionServer extends Thread implements } TableName tn = region.getTableDescriptor().getTableName(); if (!ServerRegionReplicaUtil.isRegionReplicaReplicationEnabled(region.conf, tn) || - !ServerRegionReplicaUtil.isRegionReplicaWaitForPrimaryFlushEnabled(region.conf)) { + !ServerRegionReplicaUtil.isRegionReplicaWaitForPrimaryFlushEnabled(region.conf) || + // If the memstore replication not setup, we do not have to wait for observing a flush event + // from primary before starting to serve reads, because gaps from replication is not + // applicable,this logic is from + // TableDescriptorBuilder.ModifyableTableDescriptor.setRegionMemStoreReplication by + // HBASE-13063 + !region.getTableDescriptor().hasRegionMemStoreReplication()) { region.setReadsEnabled(true); return; } diff --git a/hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestRegionReplicaWaitForPrimaryFlushConf.java b/hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestRegionReplicaWaitForPrimaryFlushConf.java new file mode 100644 index 00000000000..566b2de7910 --- /dev/null +++ b/hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestRegionReplicaWaitForPrimaryFlushConf.java @@ -0,0 +1,125 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.hadoop.hbase.regionserver; + +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertNotNull; +import static org.junit.Assert.assertNull; +import static org.junit.Assert.assertTrue; +import static org.junit.Assert.fail; + +import java.util.ArrayList; +import java.util.Arrays; +import java.util.List; +import org.apache.hadoop.conf.Configuration; +import org.apache.hadoop.hbase.HBaseClassTestRule; +import org.apache.hadoop.hbase.HBaseTestingUtility; +import org.apache.hadoop.hbase.StartMiniClusterOption; +import org.apache.hadoop.hbase.TableName; +import org.apache.hadoop.hbase.TableNameTestRule; +import org.apache.hadoop.hbase.client.ColumnFamilyDescriptorBuilder; +import org.apache.hadoop.hbase.client.TableDescriptor; +import org.apache.hadoop.hbase.client.TableDescriptorBuilder; +import org.apache.hadoop.hbase.executor.ExecutorType; +import org.apache.hadoop.hbase.testclassification.MediumTests; +import org.apache.hadoop.hbase.testclassification.RegionServerTests; +import org.apache.hadoop.hbase.util.Bytes; +import org.apache.hadoop.hbase.util.Pair; +import org.apache.hadoop.hbase.util.ServerRegionReplicaUtil; +import org.junit.AfterClass; +import org.junit.BeforeClass; +import org.junit.ClassRule; +import org.junit.Rule; +import org.junit.Test; +import org.junit.experimental.categories.Category; + +@Category({ RegionServerTests.class, MediumTests.class }) +public class TestRegionReplicaWaitForPrimaryFlushConf { + @ClassRule + public static final HBaseClassTestRule CLASS_RULE = + HBaseClassTestRule.forClass(TestRegionReplicaWaitForPrimaryFlushConf.class); + + private static final byte[] FAMILY = Bytes.toBytes("family_test"); + + private TableName tableName; + + @Rule + public final TableNameTestRule name = new TableNameTestRule(); + private static final HBaseTestingUtility HTU = new HBaseTestingUtility(); + + @BeforeClass + public static void setUpBeforeClass() throws Exception { + Configuration conf = HTU.getConfiguration(); + conf.setBoolean(ServerRegionReplicaUtil.REGION_REPLICA_REPLICATION_CONF_KEY, true); + conf.setBoolean(ServerRegionReplicaUtil.REGION_REPLICA_WAIT_FOR_PRIMARY_FLUSH_CONF_KEY, false); + HTU.startMiniCluster(StartMiniClusterOption.builder().numRegionServers(2).build()); + + } + + @AfterClass + public static void tearDownAfterClass() throws Exception { + HTU.shutdownMiniCluster(); + } + + /** + * This test is for HBASE-26811,before HBASE-26811,when + * {@link ServerRegionReplicaUtil#REGION_REPLICA_WAIT_FOR_PRIMARY_FLUSH_CONF_KEY} is false and set + * {@link TableDescriptorBuilder#setRegionMemStoreReplication} to true explicitly,the secondary + * replica would be disabled for read after open,after HBASE-26811,the secondary replica would be + * enabled for read after open. + */ + @Test + public void testSecondaryReplicaReadEnabled() throws Exception { + tableName = name.getTableName(); + TableDescriptor tableDescriptor = TableDescriptorBuilder.newBuilder(tableName) + .setRegionReplication(2).setColumnFamily(ColumnFamilyDescriptorBuilder.of(FAMILY)) + .setRegionMemStoreReplication(true).build(); + HTU.getAdmin().createTable(tableDescriptor); + + final ArrayList> regionAndRegionServers = + new ArrayList>(Arrays.asList(null, null)); + + for (int i = 0; i < 2; i++) { + HRegionServer rs = HTU.getMiniHBaseCluster().getRegionServer(i); + List onlineRegions = rs.getRegions(tableName); + for (HRegion region : onlineRegions) { + int replicaId = region.getRegionInfo().getReplicaId(); + assertNull(regionAndRegionServers.get(replicaId)); + regionAndRegionServers.set(replicaId, new Pair(region, rs)); + } + } + for (Pair pair : regionAndRegionServers) { + assertNotNull(pair); + } + + HRegionServer secondaryRs = regionAndRegionServers.get(1).getSecond(); + + try { + secondaryRs.getExecutorService() + .getExecutorThreadPool(ExecutorType.RS_REGION_REPLICA_FLUSH_OPS); + fail(); + } catch (NullPointerException e) { + assertTrue(e != null); + } + HRegion secondaryRegion = regionAndRegionServers.get(1).getFirst(); + assertFalse( + ServerRegionReplicaUtil.isRegionReplicaWaitForPrimaryFlushEnabled(secondaryRegion.conf)); + assertTrue(secondaryRegion.isReadsEnabled()); + } + +}