Mailing-List: contact commits-help@hbase.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@hbase.apache.org
Content-Type: text/plain; charset="utf-8"
MIME-Version: 1.0
Content-Transfer-Encoding: 7bit
Subject: svn commit: r1170302 - in
 /hbase/branches/0.90/src/main/java/org/apache/hadoop/hbase:
 master/CatalogJanitor.java master/handler/ServerShutdownHandler.java
 regionserver/SplitTransaction.java
Date: Tue, 13 Sep 2011 19:11:50 -0000
To: commits@hbase.apache.org
From: stack@apache.org
Message-Id: <20110913191150.80BCD238888F@eris.apache.org>

Author: stack
Date: Tue Sep 13 19:11:49 2011
New Revision: 1170302

URL: http://svn.apache.org/viewvc?rev=1170302&view=rev
Log:
HBASE-4238 CatalogJanitor can clear a daughter that split before processing its parent

Modified:
    hbase/branches/0.90/src/main/java/org/apache/hadoop/hbase/master/CatalogJanitor.java
    hbase/branches/0.90/src/main/java/org/apache/hadoop/hbase/master/handler/ServerShutdownHandler.java
    hbase/branches/0.90/src/main/java/org/apache/hadoop/hbase/regionserver/SplitTransaction.java

Modified: hbase/branches/0.90/src/main/java/org/apache/hadoop/hbase/master/CatalogJanitor.java
URL: http://svn.apache.org/viewvc/hbase/branches/0.90/src/main/java/org/apache/hadoop/hbase/master/CatalogJanitor.java?rev=1170302&r1=1170301&r2=1170302&view=diff
==============================================================================
--- hbase/branches/0.90/src/main/java/org/apache/hadoop/hbase/master/CatalogJanitor.java (original)
+++ hbase/branches/0.90/src/main/java/org/apache/hadoop/hbase/master/CatalogJanitor.java Tue Sep 13 19:11:49 2011
@@ -102,26 +102,7 @@ class CatalogJanitor extends Chore {
     // Keep Map of found split parents.  There are candidates for cleanup.
     // Use a comparator that has split parents come before its daughters.
     final Map<HRegionInfo, Result> splitParents =
-      new TreeMap<HRegionInfo, Result>(new Comparator<HRegionInfo> () {
-        @Override
-        public int compare(HRegionInfo left, HRegionInfo right) {
-          // This comparator differs from the one HRegionInfo in that it sorts
-          // parent before daughters.
-          if (left == null) return -1;
-          if (right == null) return 1;
-          // Same table name.
-          int result = Bytes.compareTo(left.getTableDesc().getName(),
-            right.getTableDesc().getName());
-          if (result != 0) return result;
-          // Compare start keys.
-          result = Bytes.compareTo(left.getStartKey(), right.getStartKey());
-          if (result != 0) return result;
-          // Compare end keys.
-          result = Bytes.compareTo(left.getEndKey(), right.getEndKey());
-          if (result != 0) return -result; // Flip the result so parent comes first.
-          return result;
-        }
-      });
+      new TreeMap<HRegionInfo, Result>(new SplitParentFirstComparator());
     // This visitor collects split parents and counts rows in the .META. table
     MetaReader.Visitor visitor = new MetaReader.Visitor() {
       @Override
@@ -152,6 +133,31 @@ class CatalogJanitor extends Chore {
   }
 
   /**
+   * Compare HRegionInfos in a way that has split parents sort BEFORE their
+   * daughters.
+   */
+  static class SplitParentFirstComparator implements Comparator<HRegionInfo> {
+    @Override
+    public int compare(HRegionInfo left, HRegionInfo right) {
+      // This comparator differs from the one HRegionInfo in that it sorts
+      // parent before daughters.
+      if (left == null) return -1;
+      if (right == null) return 1;
+      // Same table name.
+      int result = Bytes.compareTo(left.getTableDesc().getName(),
+        right.getTableDesc().getName());
+      if (result != 0) return result;
+      // Compare start keys.
+      result = Bytes.compareTo(left.getStartKey(), right.getStartKey());
+      if (result != 0) return result;
+      // Compare end keys.
+      result = Bytes.compareTo(left.getEndKey(), right.getEndKey());
+      if (result != 0) return -result; // Flip the result so parent comes first.
+      return result;
+    }
+  }
+
+  /**
    * Get HRegionInfo from passed Map of row values.
    * @param result Map to do lookup in.
    * @return Null if not found (and logs fact that expected COL_REGIONINFO
@@ -187,7 +193,7 @@ class CatalogJanitor extends Chore {
       checkDaughter(parent, rowContent, HConstants.SPLITA_QUALIFIER);
     Pair<Boolean, Boolean> b =
       checkDaughter(parent, rowContent, HConstants.SPLITB_QUALIFIER);
-    if ((a.getFirst() && !a.getSecond()) && (b.getFirst() && !b.getSecond())) {
+    if (hasNoReferences(a) && hasNoReferences(b)) {
       LOG.debug("Deleting region " + parent.getRegionNameAsString() +
         " because daughter splits no longer hold references");
       // This latter regionOffline should not be necessary but is done for now
@@ -206,7 +212,16 @@ class CatalogJanitor extends Chore {
     return result;
   }
 
-  
+  /**
+   * @param p A pair where the first boolean says whether or not the daughter
+   * region directory exists in the filesystem and then the second boolean says
+   * whether the daughter has references to the parent.
+   * @return True the passed <code>p</code> signifies no references.
+   */
+  private boolean hasNoReferences(final Pair<Boolean, Boolean> p) {
+    return !p.getFirst() || !p.getSecond();
+  }
+
   /**
    * See if the passed daughter has references in the filesystem to the parent
    * and if not, remove the note of daughter region in the parent row: its

Modified: hbase/branches/0.90/src/main/java/org/apache/hadoop/hbase/master/handler/ServerShutdownHandler.java
URL: http://svn.apache.org/viewvc/hbase/branches/0.90/src/main/java/org/apache/hadoop/hbase/master/handler/ServerShutdownHandler.java?rev=1170302&r1=1170301&r2=1170302&view=diff
==============================================================================
--- hbase/branches/0.90/src/main/java/org/apache/hadoop/hbase/master/handler/ServerShutdownHandler.java (original)
+++ hbase/branches/0.90/src/main/java/org/apache/hadoop/hbase/master/handler/ServerShutdownHandler.java Tue Sep 13 19:11:49 2011
@@ -276,6 +276,11 @@ public class ServerShutdownHandler exten
     if (isDaughterMissing(catalogTracker, daughter)) {
       LOG.info("Fixup; missing daughter " + daughter.getRegionNameAsString());
       MetaEditor.addDaughter(catalogTracker, daughter, null);
+
+      // TODO: Log WARN if the regiondir does not exist in the fs.  If its not
+      // there then something wonky about the split -- things will keep going
+      // but could be missing references to parent region.
+
       // And assign it.
       assignmentManager.assign(daughter, true);
     } else {

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=1170302&r1=1170301&r2=1170302&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 Tue Sep 13 19:11:49 2011
@@ -255,17 +255,28 @@ class SplitTransaction {
     HRegion b = createDaughterRegion(this.hri_b, this.parent.flushRequester);
 
     // Edit parent in meta.  Offlines parent region and adds splita and splitb.
+    // TODO: This can 'fail' by timing out against .META. but the edits could
+    // be applied anyways over on the server.  There is no way to tell for sure.
+    // We could try and get the edits again subsequent to their application
+    // whether we fail or not but that could fail too.  We should probably move
+    // the PONR to here before the edits go in but could mean we'd abort the
+    // regionserver when we didn't need to; i.e. the edits did not make it in.
     if (!testing) {
       MetaEditor.offlineParentInMeta(server.getCatalogTracker(),
         this.parent.getRegionInfo(), a.getRegionInfo(), b.getRegionInfo());
     }
 
     // This is the point of no return.  Adding subsequent edits to .META. as we
-    // do below when we do the daugther opens adding each to .META. can fail in
+    // do below when we do the daughter opens adding each to .META. can fail in
     // various interesting ways the most interesting of which is a timeout
-    // BUT the edits all go through (See HBASE-3872).  IF we reach the POWR
+    // BUT the edits all go through (See HBASE-3872).  IF we reach the PONR
     // then subsequent failures need to crash out this regionserver; the
     // server shutdown processing should be able to fix-up the incomplete split.
+    // The offlined parent will have the daughters as extra columns.  If
+    // we leave the daughter regions in place and do not remove them when we
+    // crash out, then they will have their references to the parent in place
+    // still and the server shutdown fixup of .META. will point to these
+    // regions.
     this.journal.add(JournalEntry.PONR);
     // Open daughters in parallel.
     DaughterOpener aOpener = new DaughterOpener(server, services, a);
@@ -607,7 +618,9 @@ class SplitTransaction {
 
       case PONR:
         // We got to the point-of-no-return so we need to just abort. Return
-        // immediately.
+        // immediately.  Do not clean up created daughter regions.  They need
+        // to be in place so we don't delete the parent region mistakenly.
+        // See HBASE-3872.
         return false;
 
       default: