From 7c368fad1ba1f0db6e7a5a5a9154b3d21b37154d Mon Sep 17 00:00:00 2001 From: Andrew Purtell Date: Wed, 5 May 2021 09:52:43 -0700 Subject: [PATCH 1/2] HBASE-25854 Remove redundant AM in-memory state changes in CatalogJanitor 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. --- .../hadoop/hbase/master/janitor/CatalogJanitor.java | 10 ---------- 1 file changed, 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 0914d53193a1..fee218eb29b2 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 @@ private boolean cleanMergeRegion(final RegionInfo mergedRegion, List 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 @@ static boolean cleanParent(MasterServices services, RegionInfo parent, Result ro 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()) { From e7db51f20b756f779c289089a697e90861eafef4 Mon Sep 17 00:00:00 2001 From: Andrew Purtell Date: Wed, 5 May 2021 17:28:42 -0700 Subject: [PATCH 2/2] Fix TestCatalogJanitorInMemoryStates --- .../TestCatalogJanitorInMemoryStates.java | 21 +++++++++++++++++++ 1 file changed, 21 insertions(+) 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 07b3ca790784..10bf0d631ae2 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.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.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 void testInMemoryParentCleanup() 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",