From 0742f803d8db543727ab147f9c9cd6197a5a6e30 Mon Sep 17 00:00:00 2001 From: comnetwork Date: Tue, 8 Mar 2022 11:34:45 +0800 Subject: [PATCH 1/6] HBASE-26811 Secondary replica may be disabled for read forever --- .../hbase/client/TableDescriptorBuilder.java | 10 +- ...tRegionReplicaWaitForPrimaryFlushConf.java | 120 ++++++++++++++++++ 2 files changed, 126 insertions(+), 4 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 0128458645cb..1abd1d1cb4c5 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 @@ -1335,11 +1335,13 @@ public boolean hasRegionMemStoreReplication() { * @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 + ModifyableTableDescriptor returnDesc = setValue(REGION_MEMSTORE_REPLICATION_KEY, Boolean.toString(memstoreReplication)); + // 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 - return setValue(REGION_REPLICA_WAIT_FOR_PRIMARY_FLUSH_CONF_KEY, - Boolean.toString(memstoreReplication)); + if(!memstoreReplication) { + return setValue(REGION_REPLICA_WAIT_FOR_PRIMARY_FLUSH_CONF_KEY, Boolean.toString(false)); + } + return returnDesc; } public ModifyableTableDescriptor setPriority(int priority) { 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 000000000000..d7c56e8b64a8 --- /dev/null +++ b/hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestRegionReplicaWaitForPrimaryFlushConf.java @@ -0,0 +1,120 @@ +/** + * 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.assertNotNull; +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.HBaseTestingUtil; +import org.apache.hadoop.hbase.StartTestingClusterOption; +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.regionserver.Region.Operation; +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(TestReplicateToReplica.class); + + private static byte[] FAMILY = Bytes.toBytes("family"); + + private TableName tableName; + + @Rule + public final TableNameTestRule name = new TableNameTestRule(); + private static final HBaseTestingUtil HTU = new HBaseTestingUtil(); + + @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(StartTestingClusterOption.builder().numRegionServers(2).build()); + + } + + @AfterClass + public static void tearDownAfterClass() throws Exception { + HTU.shutdownMiniCluster(); + } + + @Test + public void test() 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(); + assertTrue(regionAndRegionServers.get(replicaId) == null); + 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(); + assertTrue( + !ServerRegionReplicaUtil.isRegionReplicaWaitForPrimaryFlushEnabled(secondaryRegion.conf)); + secondaryRegion.startRegionOperation(Operation.SCAN); + secondaryRegion.closeRegionOperation(Operation.SCAN); + + } + +} From 8a522aa9b2754a6194d434640983d46a3ac5da61 Mon Sep 17 00:00:00 2001 From: comnetwork Date: Tue, 8 Mar 2022 18:33:34 +0800 Subject: [PATCH 2/6] modify test --- .../hbase/client/TableDescriptorBuilder.java | 2 +- .../hadoop/hbase/regionserver/HRegion.java | 7 +++++ ...tRegionReplicaWaitForPrimaryFlushConf.java | 26 +++++++++++-------- 3 files changed, 23 insertions(+), 12 deletions(-) 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 1abd1d1cb4c5..a57bacdbb83c 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 @@ -1338,7 +1338,7 @@ public ModifyableTableDescriptor setRegionMemStoreReplication(boolean memstoreRe ModifyableTableDescriptor returnDesc = setValue(REGION_MEMSTORE_REPLICATION_KEY, Boolean.toString(memstoreReplication)); // 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 - if(!memstoreReplication) { + if (!memstoreReplication) { return setValue(REGION_REPLICA_WAIT_FOR_PRIMARY_FLUSH_CONF_KEY, Boolean.toString(false)); } return returnDesc; 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 01eb4cc5107b..ea2fe0d5ddae 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 @@ -23,6 +23,7 @@ import static org.apache.hadoop.hbase.trace.HBaseSemanticAttributes.ROW_LOCK_READ_LOCK_KEY; import static org.apache.hadoop.hbase.util.ConcurrentMapUtils.computeIfAbsent; +import com.google.errorprone.annotations.RestrictedApi; import edu.umd.cs.findbugs.annotations.Nullable; import io.opentelemetry.api.trace.Span; import java.io.EOFException; @@ -8743,4 +8744,10 @@ public void addReadRequestsCount(long readRequestsCount) { public void addWriteRequestsCount(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/test/java/org/apache/hadoop/hbase/regionserver/TestRegionReplicaWaitForPrimaryFlushConf.java b/hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestRegionReplicaWaitForPrimaryFlushConf.java index d7c56e8b64a8..efbd73aed43b 100644 --- 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 @@ -17,7 +17,9 @@ */ 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; @@ -34,7 +36,6 @@ 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.regionserver.Region.Operation; import org.apache.hadoop.hbase.testclassification.MediumTests; import org.apache.hadoop.hbase.testclassification.RegionServerTests; import org.apache.hadoop.hbase.util.Bytes; @@ -51,9 +52,9 @@ public class TestRegionReplicaWaitForPrimaryFlushConf { @ClassRule public static final HBaseClassTestRule CLASS_RULE = - HBaseClassTestRule.forClass(TestReplicateToReplica.class); + HBaseClassTestRule.forClass(TestRegionReplicaWaitForPrimaryFlushConf.class); - private static byte[] FAMILY = Bytes.toBytes("family"); + private static final byte[] FAMILY = Bytes.toBytes("family_test"); private TableName tableName; @@ -75,8 +76,14 @@ public static void tearDownAfterClass() throws Exception { HTU.shutdownMiniCluster(); } + /** + * This test is for 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. + */ @Test - public void test() throws Exception { + public void testSecondaryReplicaReadEnabled() throws Exception { tableName = name.getTableName(); TableDescriptor tableDescriptor = TableDescriptorBuilder.newBuilder(tableName) .setRegionReplication(2).setColumnFamily(ColumnFamilyDescriptorBuilder.of(FAMILY)) @@ -91,7 +98,7 @@ public void test() throws Exception { List onlineRegions = rs.getRegions(tableName); for (HRegion region : onlineRegions) { int replicaId = region.getRegionInfo().getReplicaId(); - assertTrue(regionAndRegionServers.get(replicaId) == null); + assertNull(regionAndRegionServers.get(replicaId)); regionAndRegionServers.set(replicaId, new Pair(region, rs)); } } @@ -108,13 +115,10 @@ public void test() throws Exception { } catch (NullPointerException e) { assertTrue(e != null); } - HRegion secondaryRegion = regionAndRegionServers.get(1).getFirst(); - assertTrue( - !ServerRegionReplicaUtil.isRegionReplicaWaitForPrimaryFlushEnabled(secondaryRegion.conf)); - secondaryRegion.startRegionOperation(Operation.SCAN); - secondaryRegion.closeRegionOperation(Operation.SCAN); - + assertFalse( + ServerRegionReplicaUtil.isRegionReplicaWaitForPrimaryFlushEnabled(secondaryRegion.conf)); + assertTrue(secondaryRegion.isReadsEnabled()); } } From 9ef203154eb71b4e1a148c6d5511dd19c1cc1d35 Mon Sep 17 00:00:00 2001 From: comnetwork Date: Tue, 8 Mar 2022 18:39:06 +0800 Subject: [PATCH 3/6] fix checkstyle --- .../org/apache/hadoop/hbase/client/TableDescriptorBuilder.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) 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 a57bacdbb83c..d2183627fa3b 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 @@ -1335,7 +1335,8 @@ public boolean hasRegionMemStoreReplication() { * @return the modifyable TD */ public ModifyableTableDescriptor setRegionMemStoreReplication(boolean memstoreReplication) { - ModifyableTableDescriptor returnDesc = setValue(REGION_MEMSTORE_REPLICATION_KEY, Boolean.toString(memstoreReplication)); + ModifyableTableDescriptor returnDesc = + setValue(REGION_MEMSTORE_REPLICATION_KEY, Boolean.toString(memstoreReplication)); // 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 if (!memstoreReplication) { From 2ed5125da60b88f2e2fa8c0f435ed7f9beec3a79 Mon Sep 17 00:00:00 2001 From: comnetwork Date: Thu, 10 Mar 2022 10:51:44 +0800 Subject: [PATCH 4/6] move the logic --- .../hadoop/hbase/client/TableDescriptorBuilder.java | 9 +-------- .../apache/hadoop/hbase/regionserver/HRegionServer.java | 8 +++++++- 2 files changed, 8 insertions(+), 9 deletions(-) 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 d2183627fa3b..7dbd98650af3 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 @@ -1335,14 +1335,7 @@ public boolean hasRegionMemStoreReplication() { * @return the modifyable TD */ public ModifyableTableDescriptor setRegionMemStoreReplication(boolean memstoreReplication) { - ModifyableTableDescriptor returnDesc = - setValue(REGION_MEMSTORE_REPLICATION_KEY, Boolean.toString(memstoreReplication)); - // 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 - if (!memstoreReplication) { - return setValue(REGION_REPLICA_WAIT_FOR_PRIMARY_FLUSH_CONF_KEY, Boolean.toString(false)); - } - return returnDesc; + 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/HRegionServer.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java index 1517c86aff1f..16116ba7a137 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 @@ -2230,7 +2230,13 @@ private void triggerFlushInPrimaryRegion(final HRegion region) { } 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; } From bc4d2ac52153f2b1d241c93cd4a2dd8ecbb30a59 Mon Sep 17 00:00:00 2001 From: comnetwork Date: Thu, 10 Mar 2022 12:45:41 +0800 Subject: [PATCH 5/6] move the logic --- .../hbase/regionserver/HRegionServer.java | 19 ++++++++++++------- 1 file changed, 12 insertions(+), 7 deletions(-) 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 16116ba7a137..88b1a56a5ded 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 @@ -2230,13 +2230,18 @@ private void triggerFlushInPrimaryRegion(final HRegion region) { } TableName tn = region.getTableDescriptor().getTableName(); if (!ServerRegionReplicaUtil.isRegionReplicaReplicationEnabled(region.conf, tn) || - !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()) { + !ServerRegionReplicaUtil.isRegionReplicaWaitForPrimaryFlushEnabled(region.conf)) { + region.setReadsEnabled(true); + return; + } + + if (!region.getTableDescriptor().hasRegionMemStoreReplication() + && 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.setReadsEnabled(true); return; } From 3e1bc6aa9bf74132fe91b758091c183b9d113bd8 Mon Sep 17 00:00:00 2001 From: comnetwork Date: Fri, 11 Mar 2022 14:07:02 +0800 Subject: [PATCH 6/6] move the logic --- .../hbase/regionserver/HRegionServer.java | 19 +++++++------------ 1 file changed, 7 insertions(+), 12 deletions(-) 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 88b1a56a5ded..16116ba7a137 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 @@ -2230,18 +2230,13 @@ private void triggerFlushInPrimaryRegion(final HRegion region) { } TableName tn = region.getTableDescriptor().getTableName(); if (!ServerRegionReplicaUtil.isRegionReplicaReplicationEnabled(region.conf, tn) || - !ServerRegionReplicaUtil.isRegionReplicaWaitForPrimaryFlushEnabled(region.conf)) { - region.setReadsEnabled(true); - return; - } - - if (!region.getTableDescriptor().hasRegionMemStoreReplication() - && 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 + !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; }