From cd779e6f67307c5a0d9fa04ed45d1f7e8a765dda Mon Sep 17 00:00:00 2001 From: Michael Stack Date: Mon, 11 Dec 2017 22:19:31 -0800 Subject: [PATCH] HBASE-18946 Stochastic load balancer assigns replica regions to the same RS Added new bulk assign createRoundRobinAssignProcedure to complement the existing createAssignProcedure. The former asks the balancer for target servers to set into the created AssignProcedures. The latter sets no target server into AssignProcedure. When no target server is specified, we make effort at assign-time at trying to deploy the region to its old location if there was one. The new round robin assign procedure creator does not do this. Use the new round robin method on table create or reenabling offline regions. Use the old assign in ServerCrashProcedure or in EnableTable so there is a chance we retain locality. Bulk preassigning passing all to-be-assigned to the balancer in one go is good for ensuring good distribution especially when read replicas in the mix. The old assign was single-assign scoped so region replicas could end up on the same server. M hbase-server/src/main/java/org/apache/hadoop/hbase/master/assignment/AssignProcedure.java Cleanup around forceNewPlan. Was confusing. Added a Comparator to sort AssignProcedures so meta and system tables come ahead of user-space tables. M hbase-server/src/main/java/org/apache/hadoop/hbase/master/assignment/AssignmentManager.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/assignment/AssignmentManager.java Remove the forceNewPlan argument on createAssignProcedure. Didn't make sense given we were creating a new AssignProcedure; the arg had no effect. (createRoundRobinAssignProcedures) Recast to feed all regions to the balancer in bulk and to sort the return so meta and system tables take precedence. Miscellaneous fixes including keeping the Master around until all RegionServers are down, documentation on how assignment retention works, etc. --- .../hadoop/hbase/procedure2/ProcedureExecutor.java | 3 + .../org/apache/hadoop/hbase/master/HMaster.java | 11 +- .../hadoop/hbase/master/MasterRpcServices.java | 2 +- .../apache/hadoop/hbase/master/ServerManager.java | 13 +- .../hbase/master/assignment/AssignProcedure.java | 46 ++++- .../hbase/master/assignment/AssignmentManager.java | 223 ++++++++++++--------- .../master/assignment/MoveRegionProcedure.java | 4 +- .../assignment/RegionTransitionProcedure.java | 7 +- .../master/procedure/CloneSnapshotProcedure.java | 2 +- .../master/procedure/CreateTableProcedure.java | 2 +- .../master/procedure/EnableTableProcedure.java | 3 +- .../master/procedure/RecoverMetaProcedure.java | 2 +- .../master/procedure/ServerCrashProcedure.java | 7 +- .../master/procedure/TruncateTableProcedure.java | 2 +- .../hadoop/hbase/regionserver/HRegionServer.java | 7 +- .../hadoop/hbase/regionserver/RSRpcServices.java | 9 +- .../master/assignment/TestAssignmentManager.java | 24 +-- .../hbase/master/snapshot/TestAssignProcedure.java | 90 +++++++++ .../regionserver/TestRSKilledWhenInitializing.java | 6 +- .../TestRegionMergeTransactionOnCluster.java | 2 +- .../TestRegionReplicasWithRestartScenarios.java | 164 +++++++++++++++ .../TestSplitTransactionOnCluster.java | 2 +- 22 files changed, 481 insertions(+), 150 deletions(-) create mode 100644 hbase-server/src/test/java/org/apache/hadoop/hbase/master/snapshot/TestAssignProcedure.java create mode 100644 hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestRegionReplicasWithRestartScenarios.java diff --git a/hbase-procedure/src/main/java/org/apache/hadoop/hbase/procedure2/ProcedureExecutor.java b/hbase-procedure/src/main/java/org/apache/hadoop/hbase/procedure2/ProcedureExecutor.java index 976ad79c68..ac0487165e 100644 --- a/hbase-procedure/src/main/java/org/apache/hadoop/hbase/procedure2/ProcedureExecutor.java +++ b/hbase-procedure/src/main/java/org/apache/hadoop/hbase/procedure2/ProcedureExecutor.java @@ -880,6 +880,9 @@ public class ProcedureExecutor { public void submitProcedures(final Procedure[] procs) { Preconditions.checkArgument(lastProcId.get() >= 0); Preconditions.checkArgument(isRunning(), "executor not running"); + if (procs == null || procs.length <= 0) { + return; + } // Prepare procedure for (int i = 0; i < procs.length; ++i) { diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/HMaster.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/HMaster.java index 1c576209e8..c14cf7ef89 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/HMaster.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/HMaster.java @@ -761,10 +761,7 @@ public class HMaster extends HRegionServer implements MasterServices { /* * We are active master now... go initialize components we need to run. - * Note, there may be dross in zk from previous runs; it'll get addressed - * below after we determine if cluster startup or failover. */ - status.setStatus("Initializing Master file system"); this.masterActiveTime = System.currentTimeMillis(); @@ -1173,12 +1170,6 @@ public class HMaster extends HRegionServer implements MasterServices { super.stopServiceThreads(); stopChores(); - // Wait for all the remaining region servers to report in IFF we were - // running a cluster shutdown AND we were NOT aborting. - if (!isAborted() && this.serverManager != null && - this.serverManager.isClusterShutdown()) { - this.serverManager.letRegionServersShutdown(); - } if (LOG.isDebugEnabled()) { LOG.debug("Stopping service threads"); } @@ -3531,4 +3522,4 @@ public class HMaster extends HRegionServer implements MasterServices { public SpaceQuotaSnapshotNotifier getSpaceQuotaSnapshotNotifier() { return this.spaceQuotaSnapshotNotifier; } -} \ No newline at end of file +} diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/MasterRpcServices.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/MasterRpcServices.java index ce85b66cb4..10f5299e6d 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/MasterRpcServices.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/MasterRpcServices.java @@ -513,7 +513,7 @@ public class MasterRpcServices extends RSRpcServices master.cpHost.preAssign(regionInfo); } LOG.info(master.getClientIdAuditPrefix() + " assign " + regionInfo.getRegionNameAsString()); - master.getAssignmentManager().assign(regionInfo, true); + master.getAssignmentManager().assign(regionInfo); if (master.cpHost != null) { master.cpHost.postAssign(regionInfo); } diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/ServerManager.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/ServerManager.java index 78661d253e..27d08aa864 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/ServerManager.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/ServerManager.java @@ -321,8 +321,7 @@ public class ServerManager { * @param sl the server load on the server * @return true if the server is recorded, otherwise, false */ - boolean checkAndRecordNewServer( - final ServerName serverName, final ServerLoad sl) { + boolean checkAndRecordNewServer(final ServerName serverName, final ServerLoad sl) { ServerName existingServer = null; synchronized (this.onlineServers) { existingServer = findServerWithSameHostnamePortWithLock(serverName); @@ -343,7 +342,8 @@ public class ServerManager { // Note that we assume that same ts means same server, and don't expire in that case. // TODO: ts can theoretically collide due to clock shifts, so this is a bit hacky. - if (existingServer != null && (existingServer.getStartcode() < serverName.getStartcode())) { + if (existingServer != null && + (existingServer.getStartcode() < serverName.getStartcode())) { LOG.info("Triggering server recovery; existingServer " + existingServer + " looks stale, new server:" + serverName); expireServer(existingServer); @@ -857,7 +857,7 @@ public class ServerManager { for (ServerListener listener: this.listeners) { listener.waiting(); } - while (!this.master.isStopped() && count < maxToStart && + while (!this.master.isStopped() && !isClusterShutdown() && count < maxToStart && ((lastCountChange + interval) > now || timeout > slept || count < minToStart)) { // Log some info at every interval time or if there is a change if (oldCount != count || lastLogTime + interval < now) { @@ -882,6 +882,10 @@ public class ServerManager { lastCountChange = now; } } + // Did we exit the loop because cluster is going down? + if (isClusterShutdown()) { + this.master.stop("Cluster shutdown"); + } LOG.info("Finished wait on RegionServer count=" + count + "; waited=" + slept + "ms," + " expected min=" + minToStart + " server(s), max=" + getStrForMax(maxToStart) + " server(s),"+ " master is "+ (this.master.isStopped() ? "stopped.": "running")); @@ -955,7 +959,6 @@ public class ServerManager { String statusStr = "Cluster shutdown requested of master=" + this.master.getServerName(); LOG.info(statusStr); this.clusterShutdown.set(true); - this.master.stop(statusStr); } public boolean isClusterShutdown() { diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/assignment/AssignProcedure.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/assignment/AssignProcedure.java index 33e04fb9e6..5555062f3b 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/assignment/AssignProcedure.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/assignment/AssignProcedure.java @@ -1,4 +1,4 @@ -/** +/* * * Licensed to the Apache Software Foundation (ASF) under one * or more contributor license agreements. See the NOTICE file @@ -20,6 +20,7 @@ package org.apache.hadoop.hbase.master.assignment; import java.io.IOException; +import java.util.Comparator; import org.apache.commons.logging.Log; import org.apache.commons.logging.LogFactory; @@ -73,6 +74,9 @@ import org.apache.hadoop.hbase.shaded.protobuf.generated.RegionServerStatusProto public class AssignProcedure extends RegionTransitionProcedure { private static final Log LOG = LogFactory.getLog(AssignProcedure.class); + /** + * Set to true when we need recalibrate -- choose a new target -- because original assign failed. + */ private boolean forceNewPlan = false; /** @@ -84,24 +88,24 @@ public class AssignProcedure extends RegionTransitionProcedure { */ protected volatile ServerName targetServer; + /** + * Comparator that will sort AssignProcedures so meta assigns come first, then system table + * assigns and finally user space assigns. + */ + public static final CompareAssignProcedure COMPARATOR = new CompareAssignProcedure(); + public AssignProcedure() { // Required by the Procedure framework to create the procedure on replay super(); } public AssignProcedure(final RegionInfo regionInfo) { - this(regionInfo, false); - } - - public AssignProcedure(final RegionInfo regionInfo, final boolean forceNewPlan) { super(regionInfo); - this.forceNewPlan = forceNewPlan; this.targetServer = null; } public AssignProcedure(final RegionInfo regionInfo, final ServerName destinationServer) { super(regionInfo); - this.forceNewPlan = false; this.targetServer = destinationServer; } @@ -361,4 +365,32 @@ public class AssignProcedure extends RegionTransitionProcedure { protected ProcedureMetrics getProcedureMetrics(MasterProcedureEnv env) { return env.getAssignmentManager().getAssignmentManagerMetrics().getAssignProcMetrics(); } + + /** + * Sort AssignProcedures such that meta and system assigns come first before user-space assigns. + * Have to do it this way w/ distinct Comparator because Procedure is already Comparable on + * 'Env'(?). + */ + public static class CompareAssignProcedure implements Comparator { + @Override + public int compare(AssignProcedure left, AssignProcedure right) { + if (left.getRegionInfo().isMetaRegion()) { + if (right.getRegionInfo().isMetaRegion()) { + return RegionInfo.COMPARATOR.compare(left.getRegionInfo(), right.getRegionInfo()); + } + return -1; + } else if (left.getRegionInfo().isMetaRegion()) { + return +1; + } + if (left.getRegionInfo().getTable().isSystemTable()) { + if (right.getRegionInfo().getTable().isSystemTable()) { + return RegionInfo.COMPARATOR.compare(left.getRegionInfo(), right.getRegionInfo()); + } + return -1; + } else if (left.getRegionInfo().getTable().isSystemTable()) { + return +1; + } + return RegionInfo.COMPARATOR.compare(left.getRegionInfo(), right.getRegionInfo()); + } + } } diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/assignment/AssignmentManager.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/assignment/AssignmentManager.java index c7902d2f0c..23ebf582b7 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/assignment/AssignmentManager.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/assignment/AssignmentManager.java @@ -1,4 +1,4 @@ -/** +/* * 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 @@ -20,6 +20,7 @@ package org.apache.hadoop.hbase.master.assignment; import java.io.IOException; import java.util.ArrayList; +import java.util.Arrays; import java.util.Collection; import java.util.Collections; import java.util.HashMap; @@ -464,7 +465,7 @@ public class AssignmentManager implements ServerListener { proc = createAssignProcedure(metaRegionInfo, serverName); } else { LOG.debug("Assigning " + metaRegionInfo.getRegionNameAsString()); - proc = createAssignProcedure(metaRegionInfo, false); + proc = createAssignProcedure(metaRegionInfo); } ProcedureSyncWait.submitAndWaitProcedure(master.getMasterProcedureExecutor(), proc); } @@ -526,11 +527,7 @@ public class AssignmentManager implements ServerListener { } public void assign(final RegionInfo regionInfo) throws IOException { - assign(regionInfo, true); - } - - public void assign(final RegionInfo regionInfo, final boolean forceNewPlan) throws IOException { - AssignProcedure proc = createAssignProcedure(regionInfo, forceNewPlan); + AssignProcedure proc = createAssignProcedure(regionInfo); ProcedureSyncWait.submitAndWaitProcedure(master.getMasterProcedureExecutor(), proc); } @@ -605,23 +602,84 @@ public class AssignmentManager implements ServerListener { // RegionTransition procedures helpers // ============================================================================================ - public AssignProcedure[] createAssignProcedures(final Collection regionInfo) { - return createAssignProcedures(regionInfo, false); + /** + * Create round-robin assigns. Use on table creation to distribute out regions across cluster. + * @return AssignProcedures made out of the passed in hris and a call + * to the balancer to populate the assigns with targets chosen using round-robin (default + * balancer scheme). If at assign-time, the target chosen is no longer up, thats fine, + * the AssignProcedure will ask the balancer for a new target, and so on. + */ + public AssignProcedure[] createRoundRobinAssignProcedures(final List hris) { + if (hris.isEmpty()) { + return null; + } + try { + // Ask the balancer to assign our regions. Pass the regions en masse. The balancer can do + // a better job if it has all the assignments in the one lump. + Map> assignments = getBalancer().roundRobinAssignment(hris, + this.master.getServerManager().createDestinationServersList(null)); + // Return mid-method! + return createAssignProcedures(assignments, hris.size()); + } catch (HBaseIOException hioe) { + LOG.warn("Failed roundRobinAssignment", hioe); + } + // If an error above, fall-through to this simpler assign. Last resort. + return createAssignProcedures(hris); } - public AssignProcedure[] createAssignProcedures(final Collection regionInfo, - final boolean forceNewPlan) { - if (regionInfo.isEmpty()) return null; - final AssignProcedure[] procs = new AssignProcedure[regionInfo.size()]; + /** + * Create an array of AssignProcedures w/o specifying a target server. + * If no target server, at assign time, we will try to use the former location of the region + * if one exists. This is how we 'retain' the old location across a server restart. + * Used by {@link ServerCrashProcedure} assigning regions on a server that has crashed (SCP is + * also used across a cluster-restart just-in-case to ensure we do cleanup of any old WALs or + * server processes). + */ + public AssignProcedure[] createAssignProcedures(final List hris) { + if (hris.isEmpty()) { + return null; + } int index = 0; - for (RegionInfo hri: regionInfo) { - procs[index++] = createAssignProcedure(hri, forceNewPlan); + AssignProcedure [] procedures = new AssignProcedure[hris.size()]; + for (RegionInfo hri : hris) { + // Sort the procedures so meta and system regions are first in the returned array. + procedures[index++] = createAssignProcedure(hri); } - return procs; + if (procedures.length > 1) { + // Sort the procedures so meta and system regions are first in the returned array. + Arrays.sort(procedures, AssignProcedure.COMPARATOR); + } + return procedures; + } + + // Make this static for the method below where we use it typing the AssignProcedure array we + // return as result. + private static final AssignProcedure [] ASSIGN_PROCEDURE_ARRAY_TYPE = new AssignProcedure[] {}; + + /** + * @param assignments Map of assignments from which we produce an array of AssignProcedures. + * @param size Count of assignments to make (the caller may know the total count) + * @return Assignments made from the passed in assignments + */ + private AssignProcedure[] createAssignProcedures(Map> assignments, + int size) { + List procedures = new ArrayList<>(size > 0? size: 8/*Arbitrary*/); + for (Map.Entry> e: assignments.entrySet()) { + for (RegionInfo ri: e.getValue()) { + AssignProcedure ap = createAssignProcedure(ri, e.getKey()); + ap.setOwner(getProcedureEnvironment().getRequestUser().getShortName()); + procedures.add(ap); + } + } + if (procedures.size() > 1) { + // Sort the procedures so meta and system regions are first in the returned array. + procedures.sort(AssignProcedure.COMPARATOR); + } + return procedures.toArray(ASSIGN_PROCEDURE_ARRAY_TYPE); } // Needed for the following method so it can type the created Array we return - private static final UnassignProcedure [] UNASSIGNED_PROCEDURE_FOR_TYPE_INFO = + private static final UnassignProcedure [] UNASSIGN_PROCEDURE_ARRAY_TYPE = new UnassignProcedure[0]; UnassignProcedure[] createUnassignProcedures(final Collection nodes) { @@ -634,7 +692,7 @@ public class AssignmentManager implements ServerListener { assert node.getRegionLocation() != null: node.toString(); procs.add(createUnassignProcedure(node.getRegionInfo(), node.getRegionLocation(), false)); } - return procs.toArray(UNASSIGNED_PROCEDURE_FOR_TYPE_INFO); + return procs.toArray(UNASSIGN_PROCEDURE_ARRAY_TYPE); } public MoveRegionProcedure[] createReopenProcedures(final Collection regionInfo) { @@ -649,14 +707,6 @@ public class AssignmentManager implements ServerListener { } /** - * Called by things like EnableTableProcedure to get a list of AssignProcedure - * to assign the regions of the table. - */ - public AssignProcedure[] createAssignProcedures(final TableName tableName) { - return createAssignProcedures(regionStates.getRegionsOfTable(tableName)); - } - - /** * Called by things like DisableTableProcedure to get a list of UnassignProcedure * to unassign the regions of the table. */ @@ -664,17 +714,8 @@ public class AssignmentManager implements ServerListener { return createUnassignProcedures(regionStates.getTableRegionStateNodes(tableName)); } - /** - * Called by things like ModifyColumnFamilyProcedure to get a list of MoveRegionProcedure - * to reopen the regions of the table. - */ - public MoveRegionProcedure[] createReopenProcedures(final TableName tableName) { - return createReopenProcedures(regionStates.getRegionsOfTable(tableName)); - } - - public AssignProcedure createAssignProcedure(final RegionInfo regionInfo, - final boolean forceNewPlan) { - AssignProcedure proc = new AssignProcedure(regionInfo, forceNewPlan); + public AssignProcedure createAssignProcedure(final RegionInfo regionInfo) { + AssignProcedure proc = new AssignProcedure(regionInfo); proc.setOwner(getProcedureEnvironment().getRequestUser().getShortName()); return proc; } @@ -686,7 +727,7 @@ public class AssignmentManager implements ServerListener { return proc; } - public UnassignProcedure createUnassignProcedure(final RegionInfo regionInfo, + UnassignProcedure createUnassignProcedure(final RegionInfo regionInfo, final ServerName destinationServer, final boolean force) { // If destinationServer is null, figure it. ServerName sn = destinationServer != null? destinationServer: @@ -1157,19 +1198,18 @@ public class AssignmentManager implements ServerListener { public void joinCluster() throws IOException { final long startTime = System.currentTimeMillis(); - LOG.info("Joining the cluster..."); + LOG.info("Joining cluster..."); // Scan hbase:meta to build list of existing regions, servers, and assignment loadMeta(); for (int i = 0; master.getServerManager().countOfRegionServers() < 1; ++i) { - LOG.info("waiting for RS to join"); + LOG.info("Waiting for RegionServers to join; current count=" + + master.getServerManager().countOfRegionServers()); Threads.sleep(250); } - LOG.info("RS joined. Num RS = " + master.getServerManager().countOfRegionServers()); + LOG.info("Number of RegionServers=" + master.getServerManager().countOfRegionServers()); - // This method will assign all user regions if a clean server startup or - // it will reconstruct master state and cleanup any leftovers from previous master process. boolean failover = processofflineServersWithOnlineRegions(); // Start the RIT chore @@ -1220,54 +1260,74 @@ public class AssignmentManager implements ServerListener { wakeMetaLoadedEvent(); } - // TODO: the assumption here is that if RSs are crashing while we are executing this - // they will be handled by the SSH that are put in the ServerManager "queue". - // we can integrate this a bit better. + /** + * Look at what is in meta and the list of servers that have checked in and make reconciliation. + * We cannot tell definitively the difference between a clean shutdown and a cluster that has + * been crashed down. At this stage of a Master startup, they look the same: they have the + * same state in hbase:meta. We could do detective work probing ZK and the FS for old WALs to + * split but SCP does this already so just let it do its job. + *

>The profiles of clean shutdown and cluster crash-down are the same because on clean + * shutdown currently, we do not update hbase:meta with region close state (In AMv2, region + * state is kept in hbse:meta). Usually the master runs all region transitions as of AMv2 but on + * cluster controlled shutdown, the RegionServers close all their regions only reporting the + * final change to the Master. Currently this report is ignored. Later we could take it and + * update as many regions as we can before hbase:meta goes down or have the master run the + * close of all regions out on the cluster but we may never be able to achieve the proper state on + * all regions (at least not w/o lots of painful manipulations and waiting) so clean shutdown + * might not be possible especially on big clusters.... And clean shutdown will take time. Given + * this current state of affairs, we just run ServerCrashProcedure in both cases. It will always + * do the right thing. + * @return True if for sure this is a failover where a Master is starting up into an already + * running cluster. + */ + // The assumption here is that if RSs are crashing while we are executing this + // they will be handled by the SSH that are put in the ServerManager deadservers "queue". private boolean processofflineServersWithOnlineRegions() { - boolean failover = !master.getServerManager().getDeadServers().isEmpty(); - - final Set offlineServersWithOnlineRegions = new HashSet(); - final ArrayList regionsToAssign = new ArrayList(); - long st, et; - - st = System.currentTimeMillis(); + boolean deadServers = !master.getServerManager().getDeadServers().isEmpty(); + final Set offlineServersWithOnlineRegions = new HashSet<>(); + int size = regionStates.getRegionStateNodes().size(); + final List offlineRegionsToAssign = new ArrayList<>(size); + long startTime = System.currentTimeMillis(); + // If deadservers then its a failover, else, we are not sure yet. + boolean failover = deadServers; for (RegionStateNode regionNode: regionStates.getRegionStateNodes()) { + // Region State can be OPEN even if we did controlled cluster shutdown; Master does not close + // the regions in this case. The RegionServer does the close so hbase:meta is state in + // hbase:meta is not updated -- Master does all updates -- and is left with OPEN as region + // state in meta. How to tell difference between ordered shutdown and crashed-down cluster + // then? We can't. Not currently. Perhaps if we updated hbase:meta with CLOSED on ordered + // shutdown. This would slow shutdown though and not all edits would make it in anyways. + // TODO: Examine. + // Because we can't be sure it an ordered shutdown, we run ServerCrashProcedure always. + // ServerCrashProcedure will try to retain old deploy when it goes to assign. if (regionNode.getState() == State.OPEN) { final ServerName serverName = regionNode.getRegionLocation(); if (!master.getServerManager().isServerOnline(serverName)) { offlineServersWithOnlineRegions.add(serverName); + } else { + // Server is online. This a failover. Master is starting into already-running cluster. + failover = true; } } else if (regionNode.getState() == State.OFFLINE) { if (isTableEnabled(regionNode.getTable())) { - regionsToAssign.add(regionNode.getRegionInfo()); + offlineRegionsToAssign.add(regionNode.getRegionInfo()); } } } - et = System.currentTimeMillis(); - LOG.info("[STEP-1] " + StringUtils.humanTimeDiff(et - st)); - - // kill servers with online regions - st = System.currentTimeMillis(); + // Kill servers with online regions just-in-case. Runs ServerCrashProcedure. for (ServerName serverName: offlineServersWithOnlineRegions) { if (!master.getServerManager().isServerOnline(serverName)) { - LOG.info("KILL RS hosting regions but not online " + serverName + - " (master=" + master.getServerName() + ")"); + LOG.info("KILL RegionServer=" + serverName + " hosting regions but not online."); killRegionServer(serverName); } } - et = System.currentTimeMillis(); - LOG.info("[STEP-2] " + StringUtils.humanTimeDiff(et - st)); - setFailoverCleanupDone(true); - // assign offline regions - st = System.currentTimeMillis(); - for (RegionInfo regionInfo: getOrderedRegions(regionsToAssign)) { - master.getMasterProcedureExecutor().submitProcedure( - createAssignProcedure(regionInfo, false)); + // Assign offline regions. Uses round-robin. + if (offlineRegionsToAssign.size() > 0) { + master.getMasterProcedureExecutor().submitProcedures(master.getAssignmentManager(). + createRoundRobinAssignProcedures(offlineRegionsToAssign)); } - et = System.currentTimeMillis(); - LOG.info("[STEP-3] " + StringUtils.humanTimeDiff(et - st)); return failover; } @@ -1369,27 +1429,6 @@ public class AssignmentManager implements ServerListener { return new Pair(ritCount, states.size()); } - /** - * Used when assign regions, this method will put system regions in - * front of user regions - * @param regions - * @return A list of regions with system regions at front - */ - public List getOrderedRegions( - final List regions) { - if (regions == null) return Collections.emptyList(); - - List systemList = new ArrayList<>(); - List userList = new ArrayList<>(); - for (RegionInfo hri : regions) { - if (hri.getTable().isSystemTable()) systemList.add(hri); - else userList.add(hri); - } - // Append userList to systemList - systemList.addAll(userList); - return systemList; - } - // ============================================================================================ // TODO: Region State In Transition // ============================================================================================ diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/assignment/MoveRegionProcedure.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/assignment/MoveRegionProcedure.java index 4caed2895d..5940f2fe31 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/assignment/MoveRegionProcedure.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/assignment/MoveRegionProcedure.java @@ -1,4 +1,4 @@ -/** +/* * * Licensed to the Apache Software Foundation (ASF) under one * or more contributor license agreements. See the NOTICE file @@ -71,7 +71,7 @@ public class MoveRegionProcedure extends AbstractStateMachineRegionProcedure futureA = submitProcedure(am.createAssignProcedure(hri, false)); + final Future futureA = submitProcedure(am.createAssignProcedure(hri)); // wait first assign waitOnFuture(futureA); @@ -396,7 +396,7 @@ public class TestAssignmentManager { // Second should be a noop. We should recognize region is already OPEN internally // and skip out doing nothing. // wait second assign - final Future futureB = submitProcedure(am.createAssignProcedure(hri, false)); + final Future futureB = submitProcedure(am.createAssignProcedure(hri)); waitOnFuture(futureB); am.getRegionStates().isRegionInState(hri, State.OPEN); // TODO: What else can we do to ensure just a noop. @@ -419,7 +419,7 @@ public class TestAssignmentManager { rsDispatcher.setMockRsExecutor(new GoodRsExecutor()); // assign the region first - waitOnFuture(submitProcedure(am.createAssignProcedure(hri, false))); + waitOnFuture(submitProcedure(am.createAssignProcedure(hri))); final Future futureA = submitProcedure(am.createUnassignProcedure(hri, null, false)); @@ -516,7 +516,7 @@ public class TestAssignmentManager { private AssignProcedure createAndSubmitAssign(TableName tableName, int regionId) { RegionInfo hri = createRegionInfo(tableName, regionId); - AssignProcedure proc = am.createAssignProcedure(hri, false); + AssignProcedure proc = am.createAssignProcedure(hri); master.getMasterProcedureExecutor().submitProcedure(proc); return proc; } diff --git a/hbase-server/src/test/java/org/apache/hadoop/hbase/master/snapshot/TestAssignProcedure.java b/hbase-server/src/test/java/org/apache/hadoop/hbase/master/snapshot/TestAssignProcedure.java new file mode 100644 index 0000000000..ccf88de69b --- /dev/null +++ b/hbase-server/src/test/java/org/apache/hadoop/hbase/master/snapshot/TestAssignProcedure.java @@ -0,0 +1,90 @@ +/* + * Copyright The Apache Software Foundation + * + * 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.master.snapshot; + +import java.util.ArrayList; +import java.util.List; + +import org.apache.hadoop.hbase.CategoryBasedTimeout; +import org.apache.hadoop.hbase.TableName; +import org.apache.hadoop.hbase.client.RegionInfo; +import org.apache.hadoop.hbase.client.RegionInfoBuilder; +import org.apache.hadoop.hbase.master.assignment.AssignProcedure; +import org.apache.hadoop.hbase.testclassification.RegionServerTests; +import org.apache.hadoop.hbase.testclassification.SmallTests; +import org.apache.hadoop.hbase.util.Bytes; +import org.junit.Rule; +import org.junit.Test; +import org.junit.experimental.categories.Category; +import org.junit.rules.TestName; +import org.junit.rules.TestRule; + +import static junit.framework.TestCase.assertTrue; + + +@Category({RegionServerTests.class, SmallTests.class}) +public class TestAssignProcedure { + @Rule public TestName name = new TestName(); + @Rule public final TestRule timeout = CategoryBasedTimeout.builder(). + withTimeout(this.getClass()). + withLookingForStuckThread(true). + build(); + + @Test + public void testSimpleComparator() { + List procedures = new ArrayList(); + RegionInfo user1 = RegionInfoBuilder.newBuilder(TableName.valueOf("user_space1")).build(); + procedures.add(new AssignProcedure(user1)); + RegionInfo user2 = RegionInfoBuilder.newBuilder(TableName.valueOf("user_space2")).build(); + procedures.add(new AssignProcedure(RegionInfoBuilder.FIRST_META_REGIONINFO)); + procedures.add(new AssignProcedure(user2)); + RegionInfo system = RegionInfoBuilder.newBuilder(TableName.NAMESPACE_TABLE_NAME).build(); + procedures.add(new AssignProcedure(system)); + procedures.sort(AssignProcedure.COMPARATOR); + assertTrue(procedures.get(0).isMeta()); + assertTrue(procedures.get(1).getRegionInfo().getTable().equals(TableName.NAMESPACE_TABLE_NAME)); + } + + @Test + public void testComparatorWithMetas() { + List procedures = new ArrayList(); + RegionInfo user1 = RegionInfoBuilder.newBuilder(TableName.valueOf("user_space1")).build(); + procedures.add(new AssignProcedure(user1)); + RegionInfo meta2 = RegionInfoBuilder.newBuilder(TableName.META_TABLE_NAME). + setStartKey(Bytes.toBytes("002")).build(); + procedures.add(new AssignProcedure(meta2)); + RegionInfo meta1 = RegionInfoBuilder.newBuilder(TableName.META_TABLE_NAME). + setStartKey(Bytes.toBytes("001")).build(); + procedures.add(new AssignProcedure(meta1)); + procedures.add(new AssignProcedure(RegionInfoBuilder.FIRST_META_REGIONINFO)); + RegionInfo meta0 = RegionInfoBuilder.newBuilder(TableName.META_TABLE_NAME). + setStartKey(Bytes.toBytes("000")).build(); + procedures.add(new AssignProcedure(meta0)); + RegionInfo user2 = RegionInfoBuilder.newBuilder(TableName.valueOf("user_space2")).build(); + procedures.add(new AssignProcedure(user2)); + RegionInfo system = RegionInfoBuilder.newBuilder(TableName.NAMESPACE_TABLE_NAME).build(); + procedures.add(new AssignProcedure(system)); + procedures.sort(AssignProcedure.COMPARATOR); + assertTrue(procedures.get(0).getRegionInfo().equals(RegionInfoBuilder.FIRST_META_REGIONINFO)); + assertTrue(procedures.get(1).getRegionInfo().equals(meta0)); + assertTrue(procedures.get(2).getRegionInfo().equals(meta1)); + assertTrue(procedures.get(3).getRegionInfo().equals(meta2)); + assertTrue(procedures.get(4).getRegionInfo().getTable().equals(TableName.NAMESPACE_TABLE_NAME)); + } +} diff --git a/hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestRSKilledWhenInitializing.java b/hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestRSKilledWhenInitializing.java index bf5787f3d9..d7be4021ac 100644 --- a/hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestRSKilledWhenInitializing.java +++ b/hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestRSKilledWhenInitializing.java @@ -60,7 +60,7 @@ import org.apache.hadoop.hbase.shaded.protobuf.generated.RegionServerStatusProto * from list of online regions. See HBASE-9593. */ @Category({RegionServerTests.class, MediumTests.class}) -public class TestRSKilledWhenInitializing { +@Ignore("See HBASE-19515") public class TestRSKilledWhenInitializing { private static final Log LOG = LogFactory.getLog(TestRSKilledWhenInitializing.class); @Rule public TestName testName = new TestName(); @Rule public final TestRule timeout = CategoryBasedTimeout.builder(). @@ -141,6 +141,10 @@ public class TestRSKilledWhenInitializing { LOG.info("Move " + hri.getEncodedName() + " to " + killedRS.get()); master.getMaster().move(hri.getEncodedNameAsBytes(), Bytes.toBytes(killedRS.get().toString())); + + // TODO: This test could do more to verify fix. It could create a table + // and do round-robin assign. It should fail if zombie RS. HBASE-19515. + // Wait until the RS no longer shows as registered in Master. while (onlineServersList.size() > (NUM_RS + 1)) { Thread.sleep(100); diff --git a/hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestRegionMergeTransactionOnCluster.java b/hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestRegionMergeTransactionOnCluster.java index 035fb9e7f0..ede9764fd8 100644 --- a/hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestRegionMergeTransactionOnCluster.java +++ b/hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestRegionMergeTransactionOnCluster.java @@ -153,7 +153,7 @@ public class TestRegionMergeTransactionOnCluster { RegionStates regionStates = am.getRegionStates(); // We should not be able to assign it again - am.assign(hri, true); + am.assign(hri); assertFalse("Merged region can't be assigned", regionStates.isRegionInTransition(hri)); diff --git a/hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestRegionReplicasWithRestartScenarios.java b/hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestRegionReplicasWithRestartScenarios.java new file mode 100644 index 0000000000..1646a66a04 --- /dev/null +++ b/hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestRegionReplicasWithRestartScenarios.java @@ -0,0 +1,164 @@ +/* + * 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.assertEquals; +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertTrue; + +import java.io.IOException; +import java.util.ArrayList; +import java.util.Collection; + +import org.apache.commons.logging.Log; +import org.apache.commons.logging.LogFactory; +import org.apache.hadoop.conf.Configuration; +import org.apache.hadoop.hbase.CategoryBasedTimeout; +import org.apache.hadoop.hbase.HBaseTestingUtility; +import org.apache.hadoop.hbase.HConstants; +import org.apache.hadoop.hbase.TableName; +import org.apache.hadoop.hbase.client.RegionInfo; +import org.apache.hadoop.hbase.client.RegionReplicaUtil; +import org.apache.hadoop.hbase.client.Table; +import org.apache.hadoop.hbase.client.TableDescriptorBuilder; +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.RegionSplitter; +import org.apache.hadoop.hdfs.DFSConfigKeys; +import org.junit.After; +import org.junit.AfterClass; +import org.junit.Before; +import org.junit.BeforeClass; +import org.junit.Rule; +import org.junit.Test; +import org.junit.experimental.categories.Category; +import org.junit.rules.TestName; +import org.junit.rules.TestRule; + +@Category({RegionServerTests.class, MediumTests.class}) +public class TestRegionReplicasWithRestartScenarios { + private static final Log LOG = LogFactory.getLog(TestRegionReplicasWithRestartScenarios.class); + @Rule public TestName name = new TestName(); + @Rule public final TestRule timeout = CategoryBasedTimeout.builder(). + withTimeout(this.getClass()). + withLookingForStuckThread(true). + build(); + + private static final int NB_SERVERS = 3; + private Table table; + + private static final HBaseTestingUtility HTU = new HBaseTestingUtility(); + private static final byte[] f = HConstants.CATALOG_FAMILY; + + @BeforeClass + public static void beforeClass() throws Exception { + // Reduce the hdfs block size and prefetch to trigger the file-link reopen + // when the file is moved to archive (e.g. compaction) + HTU.getConfiguration().setInt(DFSConfigKeys.DFS_BLOCK_SIZE_KEY, 8192); + HTU.getConfiguration().setInt(DFSConfigKeys.DFS_CLIENT_READ_PREFETCH_SIZE_KEY, 1); + HTU.getConfiguration().setInt(HConstants.HREGION_MEMSTORE_FLUSH_SIZE, 128 * 1024 * 1024); + HTU.getConfiguration().setInt(">hbase.master.wait.on.regionservers.mintostart", 3); + HTU.startMiniCluster(NB_SERVERS); + } + + @Before + public void before() throws IOException { + TableName tableName = TableName.valueOf(this.name.getMethodName()); + // Create table then get the single region for our new table. + this.table = createTableDirectlyFromHTD(tableName); + } + + @After + public void after() throws IOException { + this.table.close(); + } + + private static Table createTableDirectlyFromHTD(final TableName tableName) throws IOException { + TableDescriptorBuilder builder = TableDescriptorBuilder.newBuilder(tableName); + builder.setRegionReplication(3); + return HTU.createTable(builder.build(), new byte[][] { f }, getSplits(20), + new Configuration(HTU.getConfiguration())); + } + + private static byte[][] getSplits(int numRegions) { + RegionSplitter.UniformSplit split = new RegionSplitter.UniformSplit(); + split.setFirstRow(Bytes.toBytes(0L)); + split.setLastRow(Bytes.toBytes(Long.MAX_VALUE)); + return split.split(numRegions); + } + + @AfterClass + public static void afterClass() throws Exception { + HRegionServer.TEST_SKIP_REPORTING_TRANSITION = false; + HTU.shutdownMiniCluster(); + } + + private HRegionServer getRS() { + return HTU.getMiniHBaseCluster().getRegionServer(0); + } + + private HRegionServer getSecondaryRS() { + return HTU.getMiniHBaseCluster().getRegionServer(1); + } + + private HRegionServer getTertiaryRS() { + return HTU.getMiniHBaseCluster().getRegionServer(2); + } + + @Test + public void testRegionReplicasCreated() throws Exception { + Collection onlineRegions = getRS().getOnlineRegionsLocalContext(); + boolean res = checkDuplicates(onlineRegions); + assertFalse(res); + Collection onlineRegions2 = getSecondaryRS().getOnlineRegionsLocalContext(); + res = checkDuplicates(onlineRegions2); + assertFalse(res); + Collection onlineRegions3 = getTertiaryRS().getOnlineRegionsLocalContext(); + checkDuplicates(onlineRegions3); + assertFalse(res); + int totalRegions = onlineRegions.size() + onlineRegions2.size() + onlineRegions3.size(); + assertEquals(62, totalRegions); + } + + private boolean checkDuplicates(Collection onlineRegions3) throws Exception { + ArrayList copyOfRegion = new ArrayList(onlineRegions3); + for (Region region : copyOfRegion) { + RegionInfo regionInfo = region.getRegionInfo(); + RegionInfo regionInfoForReplica = + RegionReplicaUtil.getRegionInfoForDefaultReplica(regionInfo); + int i = 0; + for (Region actualRegion : onlineRegions3) { + if (regionInfoForReplica.equals( + RegionReplicaUtil.getRegionInfoForDefaultReplica(actualRegion.getRegionInfo()))) { + i++; + if (i > 1) { + LOG.info("Duplicate found " + actualRegion.getRegionInfo() + " " + + region.getRegionInfo()); + assertTrue(Bytes.equals(region.getRegionInfo().getStartKey(), + actualRegion.getRegionInfo().getStartKey())); + assertTrue(Bytes.equals(region.getRegionInfo().getEndKey(), + actualRegion.getRegionInfo().getEndKey())); + return true; + } + } + } + } + return false; + } +} diff --git a/hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestSplitTransactionOnCluster.java b/hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestSplitTransactionOnCluster.java index 8924454ee4..92833fde61 100644 --- a/hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestSplitTransactionOnCluster.java +++ b/hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestSplitTransactionOnCluster.java @@ -716,7 +716,7 @@ public class TestSplitTransactionOnCluster { assertTrue(regionStates.isRegionInState(daughters.get(1).getRegionInfo(), State.OPEN)); // We should not be able to assign it again - am.assign(hri, true); + am.assign(hri); assertFalse("Split region can't be assigned", regionStates.isRegionInTransition(hri)); assertTrue(regionStates.isRegionInState(hri, State.SPLIT)); -- 2.11.0 (Apple Git-81)