diff --git a/src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java b/src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java index f4625f6..7e9ed96 100644 --- a/src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java +++ b/src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java @@ -538,7 +538,7 @@ public class AssignmentManager extends ZooKeeperListener { addToServers(serverInfo, regionInfo); } // Remove plan if one. - this.regionPlans.remove(regionInfo.getEncodedName()); + clearRegionPlan(regionInfo); // Update timers for all regions in transition going against this server. updateTimers(serverInfo); } @@ -558,15 +558,17 @@ public class AssignmentManager extends ZooKeeperListener { */ private void updateTimers(final HServerInfo hsi) { // This loop could be expensive - for (Map.Entry e: this.regionPlans.entrySet()) { - if (e.getValue().getDestination().equals(hsi)) { - RegionState rs = null; - synchronized (this.regionsInTransition) { - rs = this.regionsInTransition.get(e.getKey()); - } - if (rs != null) { - synchronized (rs) { - rs.update(rs.getState()); + synchronized (this.regionPlans) { + for (Map.Entry e: this.regionPlans.entrySet()) { + if (e.getValue().getDestination().equals(hsi)) { + RegionState rs = null; + synchronized (this.regionsInTransition) { + rs = this.regionsInTransition.get(e.getKey()); + } + if (rs != null) { + synchronized (rs) { + rs.update(rs.getState()); + } } } } @@ -586,6 +588,8 @@ public class AssignmentManager extends ZooKeeperListener { this.regionsInTransition.notifyAll(); } } + // remove the region plan as well just in case. + clearRegionPlan(regionInfo); setOffline(regionInfo); } @@ -681,6 +685,7 @@ public class AssignmentManager extends ZooKeeperListener { final List regions) { LOG.debug("Bulk assigning " + regions.size() + " region(s) to " + destination.getServerName()); + List states = new ArrayList(regions.size()); synchronized (this.regionsInTransition) { for (HRegionInfo region: regions) { @@ -1382,13 +1387,14 @@ public class AssignmentManager extends ZooKeeperListener { } } } - clearRegionPlan(hri.getEncodedName()); + clearRegionPlan(hri); } /** - * @param encodedRegionName Region whose plan we are to clear. + * @param region Region whose plan we are to clear. */ - void clearRegionPlan(final String encodedRegionName) { + void clearRegionPlan(final HRegionInfo region) { + final String encodedRegionName = region.getEncodedName(); synchronized (this.regionPlans) { this.regionPlans.remove(encodedRegionName); } @@ -1644,6 +1650,25 @@ public class AssignmentManager extends ZooKeeperListener { public void handleSplitReport(final HServerInfo hsi, final HRegionInfo parent, final HRegionInfo a, final HRegionInfo b) { regionOffline(parent); + // remove any CLOSING node, if exists, due to race between master & rs + // for close & split. Not putting int regionOffline due to multi use by + // other people. + try { + RegionTransitionData node = ZKAssign.getDataNoWatch(this.watcher, + parent.getEncodedName(), null); + + if (node != null) { + if(node.getEventType().equals(EventType.RS_ZK_REGION_CLOSING)) { + ZKAssign.deleteClosingNode(this.watcher, parent); + } else { + LOG.warn("Split report has RIT node (shouldnt have one): " + + parent + " node: " + node); + } + } + } catch (KeeperException e) { + LOG.warn("Exception while validating RIT during split report", e); + } + regionOnline(a, hsi); regionOnline(b, hsi); @@ -1704,7 +1729,9 @@ public class AssignmentManager extends ZooKeeperListener { * @param plan Plan to execute. */ void balance(final RegionPlan plan) { - this.regionPlans.put(plan.getRegionName(), plan); + synchronized (this.regionPlans) { + this.regionPlans.put(plan.getRegionName(), plan); + } unassign(plan.getRegionInfo()); } diff --git a/src/main/java/org/apache/hadoop/hbase/master/HMaster.java b/src/main/java/org/apache/hadoop/hbase/master/HMaster.java index 21609c9..0ba46b7 100644 --- a/src/main/java/org/apache/hadoop/hbase/master/HMaster.java +++ b/src/main/java/org/apache/hadoop/hbase/master/HMaster.java @@ -710,7 +710,7 @@ implements HMasterInterface, HMasterRegionInterface, MasterServices, Server { if (destServerName == null || destServerName.length == 0) { LOG.info("Passed destination servername is null/empty so " + "choosing a server at random"); - this.assignmentManager.clearRegionPlan(hri.getEncodedName()); + this.assignmentManager.clearRegionPlan(hri); // Unassign will reassign it elsewhere choosing random server. this.assignmentManager.unassign(hri); } else { diff --git a/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java b/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java index dcea027..8ecff87 100644 --- a/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java +++ b/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java @@ -465,6 +465,8 @@ public class HRegion implements HeapSize { // , Writable{ return close(false); } + private final Object closeLock = new Object(); + /** * Close down this HRegion. Flush the cache unless abort parameter is true, * Shut down each HStore, don't service any more calls. @@ -479,7 +481,16 @@ public class HRegion implements HeapSize { // , Writable{ * * @throws IOException e */ - public List close(final boolean abort) + public List close(final boolean abort) throws IOException { + // Only allow one thread to close at a time. Serialize them so dual + // threads attempting to close will run up against each other. + + synchronized (closeLock) { + return doClose(abort); + } + } + + private List doClose(final boolean abort) throws IOException { if (isClosed()) { LOG.warn("Region " + this + " already closed"); diff --git a/src/main/java/org/apache/hadoop/hbase/regionserver/SplitTransaction.java b/src/main/java/org/apache/hadoop/hbase/regionserver/SplitTransaction.java index 1bcde8c..26892db 100644 --- a/src/main/java/org/apache/hadoop/hbase/regionserver/SplitTransaction.java +++ b/src/main/java/org/apache/hadoop/hbase/regionserver/SplitTransaction.java @@ -195,6 +195,15 @@ class SplitTransaction { this.journal.add(JournalEntry.CREATE_SPLIT_DIR); List hstoreFilesToSplit = this.parent.close(false); + if (hstoreFilesToSplit == null) { + // the region was closed by a concurrent thread, we can't continue + // with the split, instead we must just abandon the split. If we + // reopen or split this could cause problems because the region has + // probably already been moved to a different server, or is in the + // progress of moving to a different server. + throw new IOException("Failed to close region: already closed by " + + "another thread"); + } this.journal.add(JournalEntry.CLOSED_PARENT_REGION); if (!testing) { diff --git a/src/main/java/org/apache/hadoop/hbase/regionserver/handler/CloseRegionHandler.java b/src/main/java/org/apache/hadoop/hbase/regionserver/handler/CloseRegionHandler.java index 6cba18a..908401c 100644 --- a/src/main/java/org/apache/hadoop/hbase/regionserver/handler/CloseRegionHandler.java +++ b/src/main/java/org/apache/hadoop/hbase/regionserver/handler/CloseRegionHandler.java @@ -115,7 +115,14 @@ public class CloseRegionHandler extends EventHandler { try { // TODO: If we need to keep updating CLOSING stamp to prevent against // a timeout if this is long-running, need to spin up a thread? - region.close(abort); + if (region.close(abort) == null) { + // This region got closed. Most likely due to a split. So instead + // of doing the setClosedState() below, let's just ignore and continue. + // The split message will clean up the master state. + LOG.warn("Can't close region: was already closed during close(): " + + regionInfo.getRegionNameAsString()); + return; + } } catch (IOException e) { LOG.error("Unrecoverable exception while closing region " + regionInfo.getRegionNameAsString() + ", still finishing close", e); @@ -173,4 +180,17 @@ public class CloseRegionHandler extends EventHandler { } return expectedVersion; } + + private void removeClosingState() { + try { + ZKAssign.deleteClosingNode(server.getZooKeeper(), + regionInfo); + } catch (KeeperException.NoNodeException e) { + LOG.warn("Error removing closing state node during split failure " + + "of region " + regionInfo, e); + } catch (KeeperException e) { + LOG.warn("Error removing closing state node during split failure " + + "of region " + regionInfo, e); + } + } } \ No newline at end of file diff --git a/src/main/java/org/apache/hadoop/hbase/zookeeper/ZKAssign.java b/src/main/java/org/apache/hadoop/hbase/zookeeper/ZKAssign.java index 96ced28..1ac083d 100644 --- a/src/main/java/org/apache/hadoop/hbase/zookeeper/ZKAssign.java +++ b/src/main/java/org/apache/hadoop/hbase/zookeeper/ZKAssign.java @@ -355,13 +355,14 @@ public class ZKAssign { * of the specified regions transition to being closed. * * @param zkw zk reference - * @param regionName closing region to be deleted from zk + * @param region closing region to be deleted from zk * @throws KeeperException if unexpected zookeeper exception * @throws KeeperException.NoNodeException if node does not exist */ public static boolean deleteClosingNode(ZooKeeperWatcher zkw, - String regionName) + HRegionInfo region) throws KeeperException, KeeperException.NoNodeException { + String regionName = region.getEncodedName(); return deleteNode(zkw, regionName, EventType.RS_ZK_REGION_CLOSING); } @@ -381,7 +382,7 @@ public class ZKAssign { * of the specified regions transition to being closed. * * @param zkw zk reference - * @param region region to be deleted from zk + * @param regionName region to be deleted from zk * @param expectedState state region must be in for delete to complete * @throws KeeperException if unexpected zookeeper exception * @throws KeeperException.NoNodeException if node does not exist @@ -467,9 +468,11 @@ public class ZKAssign { throws KeeperException, KeeperException.NodeExistsException { LOG.debug(zkw.prefix("Creating unassigned node for " + region.getEncodedName() + " in a CLOSING state")); + RegionTransitionData data = new RegionTransitionData( EventType.RS_ZK_REGION_CLOSING, region.getRegionName(), serverName); - synchronized(zkw.getNodes()) { + + synchronized (zkw.getNodes()) { String node = getNodeName(zkw, region.getEncodedName()); zkw.getNodes().add(node); return ZKUtil.createAndWatch(zkw, node, data.getBytes());