From 6309c090b520e9b2b1a7b6e7be11a2066b587202 Mon Sep 17 00:00:00 2001 From: Andrew Purtell Date: Thu, 6 May 2021 09:13:33 -0700 Subject: [PATCH] HBASE-25854 Remove redundant AM in-memory state changes in CatalogJanitor (#3234) In CatalogJanitor we schedule GCRegionProcedure to clean up both filesystem and in-memory state after a split, and GCMultipleMergedRegionsProcedure to do the same for merges. Both of these procedures clean up in-memory state, but CatalogJanitor also does this redundantly just after scheduling the procedures. The cleanup should be done in only one place. Presumably we are using the procedures to do it in a principled way. Remove the redundancy in CatalogJanitor and fix any follow on issues, like test failures. Signed-off-by: Duo Zhang Signed-off-by: Michael Stack Signed-off-by: Viraj Jasani --- .../hbase/master/janitor/CatalogJanitor.java | 10 --------- .../TestCatalogJanitorInMemoryStates.java | 21 +++++++++++++++++++ 2 files changed, 21 insertions(+), 10 deletions(-) diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/janitor/CatalogJanitor.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/janitor/CatalogJanitor.java index 0914d53193a..fee218eb29b 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/janitor/CatalogJanitor.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/janitor/CatalogJanitor.java @@ -280,12 +280,6 @@ public class CatalogJanitor extends ScheduledChore { LOG.debug("Submitted procedure {} for merged region {}", mergeRegionProcedure, mergedRegion); } - // TODO: The above scheduled GCMultipleMergedRegionsProcedure does the below. - // Do we need this? - for (RegionInfo ri : parents) { - this.services.getAssignmentManager().getRegionStates().deleteRegion(ri); - this.services.getServerManager().removeRegion(ri); - } return true; } return false; @@ -356,10 +350,6 @@ public class CatalogJanitor extends ScheduledChore { if (LOG.isDebugEnabled()) { LOG.debug("Submitted procedure {} for split parent {}", gcRegionProcedure, parent); } - // Remove from in-memory states - // TODO: The above scheduled GCRegionProcedure does the below. Do we need this? - services.getAssignmentManager().getRegionStates().deleteRegion(parent); - services.getServerManager().removeRegion(parent); return true; } else { if (LOG.isDebugEnabled()) { diff --git a/hbase-server/src/test/java/org/apache/hadoop/hbase/master/janitor/TestCatalogJanitorInMemoryStates.java b/hbase-server/src/test/java/org/apache/hadoop/hbase/master/janitor/TestCatalogJanitorInMemoryStates.java index 07b3ca79078..10bf0d631ae 100644 --- a/hbase-server/src/test/java/org/apache/hadoop/hbase/master/janitor/TestCatalogJanitorInMemoryStates.java +++ b/hbase-server/src/test/java/org/apache/hadoop/hbase/master/janitor/TestCatalogJanitorInMemoryStates.java @@ -33,6 +33,7 @@ import org.apache.hadoop.hbase.MetaMockingUtil; import org.apache.hadoop.hbase.MetaTableAccessor; import org.apache.hadoop.hbase.TableName; import org.apache.hadoop.hbase.TableNameTestRule; +import org.apache.hadoop.hbase.Waiter; import org.apache.hadoop.hbase.client.Admin; import org.apache.hadoop.hbase.client.Connection; import org.apache.hadoop.hbase.client.ConnectionFactory; @@ -44,6 +45,10 @@ import org.apache.hadoop.hbase.client.Table; import org.apache.hadoop.hbase.master.HMaster; import org.apache.hadoop.hbase.master.ServerManager; import org.apache.hadoop.hbase.master.assignment.AssignmentManager; +import org.apache.hadoop.hbase.master.assignment.GCRegionProcedure; +import org.apache.hadoop.hbase.master.procedure.MasterProcedureEnv; +import org.apache.hadoop.hbase.procedure2.Procedure; +import org.apache.hadoop.hbase.procedure2.ProcedureExecutor; import org.apache.hadoop.hbase.testclassification.MasterTests; import org.apache.hadoop.hbase.testclassification.MediumTests; import org.apache.hadoop.hbase.util.Bytes; @@ -120,6 +125,22 @@ public class TestCatalogJanitorInMemoryStates { Result r = MetaMockingUtil.getMetaTableRowResult(parent.getRegion(), null, daughters.get(0).getRegion(), daughters.get(1).getRegion()); CatalogJanitor.cleanParent(master, parent.getRegion(), r); + + // wait for procedures to complete + Waiter.waitFor(TEST_UTIL.getConfiguration(), 10 * 1000, new Waiter.Predicate() { + @Override + public boolean evaluate() throws Exception { + ProcedureExecutor pe = master.getMasterProcedureExecutor(); + for (Procedure proc: pe.getProcedures()) { + if (proc.getClass().isAssignableFrom(GCRegionProcedure.class) && + proc.isFinished()) { + return true; + } + } + return false; + } + }); + assertFalse("Parent region should have been removed from RegionStates", am.getRegionStates().isRegionInRegionStates(parent.getRegion())); assertFalse("Parent region should have been removed from ServerManager",