hbase-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From st...@apache.org
Subject svn commit: r1042045 - in /hbase/branches/0.90: ./ src/main/java/org/apache/hadoop/hbase/master/ src/main/java/org/apache/hadoop/hbase/regionserver/ src/main/java/org/apache/hadoop/hbase/regionserver/handler/ src/main/java/org/apache/hadoop/hbase/zooke...
Date Fri, 03 Dec 2010 22:53:04 GMT
Author: stack
Date: Fri Dec  3 22:53:04 2010
New Revision: 1042045

URL: http://svn.apache.org/viewvc?rev=1042045&view=rev
Log:
HBASE-3298 Regionserver can close during a split causing double assignment

Modified:
    hbase/branches/0.90/CHANGES.txt
    hbase/branches/0.90/src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java
    hbase/branches/0.90/src/main/java/org/apache/hadoop/hbase/master/HMaster.java
    hbase/branches/0.90/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
    hbase/branches/0.90/src/main/java/org/apache/hadoop/hbase/regionserver/SplitTransaction.java
    hbase/branches/0.90/src/main/java/org/apache/hadoop/hbase/regionserver/handler/CloseRegionHandler.java
    hbase/branches/0.90/src/main/java/org/apache/hadoop/hbase/zookeeper/ZKAssign.java

Modified: hbase/branches/0.90/CHANGES.txt
URL: http://svn.apache.org/viewvc/hbase/branches/0.90/CHANGES.txt?rev=1042045&r1=1042044&r2=1042045&view=diff
==============================================================================
--- hbase/branches/0.90/CHANGES.txt (original)
+++ hbase/branches/0.90/CHANGES.txt Fri Dec  3 22:53:04 2010
@@ -723,6 +723,7 @@ Release 0.90.0 - Unreleased
                assigning/unassigning region
    HBASE-3296  Newly created table ends up disabled instead of assigned
    HBASE-3304  Get spurious master fails during bootup
+   HBASE-3298  Regionserver can close during a split causing double assignment
 
 
   IMPROVEMENTS

Modified: hbase/branches/0.90/src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java
URL: http://svn.apache.org/viewvc/hbase/branches/0.90/src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java?rev=1042045&r1=1042044&r2=1042045&view=diff
==============================================================================
--- hbase/branches/0.90/src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java
(original)
+++ hbase/branches/0.90/src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java
Fri Dec  3 22:53:04 2010
@@ -25,6 +25,7 @@ import java.io.IOException;
 import java.net.ConnectException;
 import java.util.ArrayList;
 import java.util.HashMap;
+import java.util.HashSet;
 import java.util.Iterator;
 import java.util.List;
 import java.util.Map;
@@ -33,7 +34,6 @@ import java.util.Set;
 import java.util.SortedMap;
 import java.util.TreeMap;
 import java.util.TreeSet;
-import java.util.concurrent.ConcurrentNavigableMap;
 import java.util.concurrent.ConcurrentSkipListMap;
 import java.util.concurrent.atomic.AtomicInteger;
 
@@ -108,11 +108,9 @@ public class AssignmentManager extends Z
   /** Plans for region movement. Key is the encoded version of a region name*/
   // TODO: When do plans get cleaned out?  Ever? In server open and in server
   // shutdown processing -- St.Ack
-  // TODO: Better to just synchronize access around regionPlans?  I think that
-  //       would be better than a concurrent structure since we do more than
-  //       one operation at a time -- jgray
-  final ConcurrentNavigableMap<String, RegionPlan> regionPlans =
-    new ConcurrentSkipListMap<String, RegionPlan>();
+  // All access to this Map must be synchronized.
+  final NavigableMap<String, RegionPlan> regionPlans =
+    new TreeMap<String, RegionPlan>();
 
   private final ZKTable zkTable;
 
@@ -538,7 +536,7 @@ public class AssignmentManager extends Z
       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);
   }
@@ -557,18 +555,23 @@ public class AssignmentManager extends Z
    * @param hsi
    */
   private void updateTimers(final HServerInfo hsi) {
-    // This loop could be expensive
-    for (Map.Entry<String, RegionPlan> 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());
-          }
-        }
+    // This loop could be expensive.
+    // First make a copy of current regionPlan rather than hold sync while
+    // looping because holding sync can cause deadlock.  Its ok in this loop
+    // if the Map we're going against is a little stale
+    Map<String, RegionPlan> copy = new HashMap<String, RegionPlan>();
+    synchronized(this.regionPlans) {
+      copy.putAll(this.regionPlans);
+    }
+    for (Map.Entry<String, RegionPlan> e: copy.entrySet()) {
+      if (!e.getValue().getDestination().equals(hsi)) continue;
+      RegionState rs = null;
+      synchronized (this.regionsInTransition) {
+        rs = this.regionsInTransition.get(e.getKey());
+      }
+      if (rs == null) continue;
+      synchronized (rs) {
+        rs.update(rs.getState());
       }
     }
   }
@@ -586,6 +589,8 @@ public class AssignmentManager extends Z
         this.regionsInTransition.notifyAll();
       }
     }
+    // remove the region plan as well just in case.
+    clearRegionPlan(regionInfo);
     setOffline(regionInfo);
   }
 
@@ -681,6 +686,7 @@ public class AssignmentManager extends Z
       final List<HRegionInfo> regions) {
     LOG.debug("Bulk assigning " + regions.size() + " region(s) to " +
       destination.getServerName());
+
     List<RegionState> states = new ArrayList<RegionState>(regions.size());
     synchronized (this.regionsInTransition) {
       for (HRegionInfo region: regions) {
@@ -941,23 +947,28 @@ public class AssignmentManager extends Z
     if (servers.isEmpty()) return null;
     RegionPlan randomPlan = new RegionPlan(state.getRegion(), null,
       LoadBalancer.randomAssignment(servers));
+    boolean newPlan = false;
+    RegionPlan existingPlan = null;
     synchronized (this.regionPlans) {
-      RegionPlan existingPlan = this.regionPlans.get(encodedName);
+      existingPlan = this.regionPlans.get(encodedName);
       if (existingPlan == null || forceNewPlan ||
           existingPlan.getDestination().equals(serverToExclude)) {
-        LOG.debug("No previous transition plan was found (or we are ignoring " +
-            "an existing plan) for " + state.getRegion().getRegionNameAsString()
-            + " so generated a random one; " + randomPlan + "; " +
-            serverManager.countOfRegionServers() +
-            " (online=" + serverManager.getOnlineServers().size() +
-            ", exclude=" + serverToExclude + ") available servers");
+        newPlan = true;
         this.regionPlans.put(encodedName, randomPlan);
+      }
+    }
+    if (newPlan) {
+      LOG.debug("No previous transition plan was found (or we are ignoring " +
+        "an existing plan) for " + state.getRegion().getRegionNameAsString() +
+        " so generated a random one; " + randomPlan + "; " +
+        serverManager.countOfRegionServers() +
+        " (online=" + serverManager.getOnlineServers().size() +
+        ", exclude=" + serverToExclude + ") available servers");
         return randomPlan;
       }
       LOG.debug("Using pre-existing plan for region " +
-          state.getRegion().getRegionNameAsString() + "; plan=" + existingPlan);
+        state.getRegion().getRegionNameAsString() + "; plan=" + existingPlan);
       return existingPlan;
-    }
   }
 
   /**
@@ -1384,15 +1395,15 @@ public class AssignmentManager extends Z
         }
       }
     }
-    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) {
     synchronized (this.regionPlans) {
-      this.regionPlans.remove(encodedRegionName);
+      this.regionPlans.remove(region.getEncodedName());
     }
   }
 
@@ -1646,6 +1657,24 @@ public class AssignmentManager extends Z
   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 into regionOffline method because it is
+    // called from various locations.
+    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);
 
@@ -1706,7 +1735,9 @@ public class AssignmentManager extends Z
    * @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());
   }
 

Modified: hbase/branches/0.90/src/main/java/org/apache/hadoop/hbase/master/HMaster.java
URL: http://svn.apache.org/viewvc/hbase/branches/0.90/src/main/java/org/apache/hadoop/hbase/master/HMaster.java?rev=1042045&r1=1042044&r2=1042045&view=diff
==============================================================================
--- hbase/branches/0.90/src/main/java/org/apache/hadoop/hbase/master/HMaster.java (original)
+++ hbase/branches/0.90/src/main/java/org/apache/hadoop/hbase/master/HMaster.java Fri Dec
 3 22:53:04 2010
@@ -710,7 +710,7 @@ implements HMasterInterface, HMasterRegi
     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 {

Modified: hbase/branches/0.90/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
URL: http://svn.apache.org/viewvc/hbase/branches/0.90/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java?rev=1042045&r1=1042044&r2=1042045&view=diff
==============================================================================
--- hbase/branches/0.90/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java (original)
+++ hbase/branches/0.90/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java Fri
Dec  3 22:53:04 2010
@@ -179,6 +179,9 @@ public class HRegion implements HeapSize
   private volatile boolean forceMajorCompaction = false;
   private Pair<Long,Long> lastCompactInfo = null;
 
+  // Used to ensure only one thread closes region at a time.
+  private final Object closeLock = new Object();
+
   /*
    * Data structure of write state flags used coordinating flushes,
    * compactions and closes.
@@ -479,7 +482,15 @@ public class HRegion implements HeapSize
    *
    * @throws IOException e
    */
-  public List<StoreFile> close(final boolean abort)
+  public List<StoreFile> 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<StoreFile> doClose(final boolean abort)
   throws IOException {
     if (isClosed()) {
       LOG.warn("Region " + this + " already closed");

Modified: hbase/branches/0.90/src/main/java/org/apache/hadoop/hbase/regionserver/SplitTransaction.java
URL: http://svn.apache.org/viewvc/hbase/branches/0.90/src/main/java/org/apache/hadoop/hbase/regionserver/SplitTransaction.java?rev=1042045&r1=1042044&r2=1042045&view=diff
==============================================================================
--- hbase/branches/0.90/src/main/java/org/apache/hadoop/hbase/regionserver/SplitTransaction.java
(original)
+++ hbase/branches/0.90/src/main/java/org/apache/hadoop/hbase/regionserver/SplitTransaction.java
Fri Dec  3 22:53:04 2010
@@ -195,6 +195,15 @@ class SplitTransaction {
     this.journal.add(JournalEntry.CREATE_SPLIT_DIR);
 
     List<StoreFile> 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
+      // process 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) {

Modified: hbase/branches/0.90/src/main/java/org/apache/hadoop/hbase/regionserver/handler/CloseRegionHandler.java
URL: http://svn.apache.org/viewvc/hbase/branches/0.90/src/main/java/org/apache/hadoop/hbase/regionserver/handler/CloseRegionHandler.java?rev=1042045&r1=1042044&r2=1042045&view=diff
==============================================================================
--- hbase/branches/0.90/src/main/java/org/apache/hadoop/hbase/regionserver/handler/CloseRegionHandler.java
(original)
+++ hbase/branches/0.90/src/main/java/org/apache/hadoop/hbase/regionserver/handler/CloseRegionHandler.java
Fri Dec  3 22:53:04 2010
@@ -114,11 +114,18 @@ public class CloseRegionHandler extends 
     // Close the region
     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);
+      // a timeout if this is long-running, need to spin up a thread?
+      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);
+        regionInfo.getRegionNameAsString() + ", still finishing close", e);
     }
 
     this.rsServices.removeFromOnlineRegions(regionInfo.getEncodedName());
@@ -164,12 +171,12 @@ public class CloseRegionHandler extends 
     try {
       if ((expectedVersion = ZKAssign.createNodeClosing(
           server.getZooKeeper(), regionInfo, server.getServerName())) == FAILED) {
-        LOG.warn("Error creating node in CLOSING state, aborting close of "
-            + regionInfo.getRegionNameAsString());
+        LOG.warn("Error creating node in CLOSING state, aborting close of " +
+          regionInfo.getRegionNameAsString());
       }
     } catch (KeeperException e) {
-      LOG.warn("Error creating node in CLOSING state, aborting close of "
-          + regionInfo.getRegionNameAsString());
+      LOG.warn("Error creating node in CLOSING state, aborting close of " +
+        regionInfo.getRegionNameAsString());
     }
     return expectedVersion;
   }

Modified: hbase/branches/0.90/src/main/java/org/apache/hadoop/hbase/zookeeper/ZKAssign.java
URL: http://svn.apache.org/viewvc/hbase/branches/0.90/src/main/java/org/apache/hadoop/hbase/zookeeper/ZKAssign.java?rev=1042045&r1=1042044&r2=1042045&view=diff
==============================================================================
--- hbase/branches/0.90/src/main/java/org/apache/hadoop/hbase/zookeeper/ZKAssign.java (original)
+++ hbase/branches/0.90/src/main/java/org/apache/hadoop/hbase/zookeeper/ZKAssign.java Fri
Dec  3 22:53:04 2010
@@ -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());



Mime
View raw message