HBASE-2656. HMaster.getRegionTableClosest should not return null for closed regions

git-svn-id: https://svn.apache.org/repos/asf/hbase/trunk@956685 13f79535-47bb-0310-9956-ffa450edef68

CHANGES.txt

@@ -398,6 +398,8 @@ Release 0.21.0 - Unreleased
    HBASE-2752  Don't retry forever when waiting on too many store files
    HBASE-2737  CME in ZKW introduced in HBASE-2694 (Karthik Ranganathan via JD)
    HBASE-2756  MetaScanner.metaScan doesn't take configurations
+   HBASE-2656  HMaster.getRegionTableClosest should not return null for closed
+               regions
 
   IMPROVEMENTS
    HBASE-1760  Cleanup TODOs in HTable

src/main/java/org/apache/hadoop/hbase/HRegionInfo.java

@@ -424,6 +424,15 @@ public class HRegionInfo extends VersionedWritable implements WritableComparable
     return firstKeyInRange && lastKeyInRange;
   }
   
+  /**
+   * Return true if the given row falls in this region.
+   */
+  public boolean containsRow(byte[] row) {
+    return Bytes.compareTo(row, startKey) >= 0 &&
+      (Bytes.compareTo(row, endKey) < 0 ||
+       Bytes.equals(endKey, HConstants.EMPTY_BYTE_ARRAY));
+  }
+
   /** @return the tableDesc */
   public HTableDescriptor getTableDesc(){
     return tableDesc;

src/main/java/org/apache/hadoop/hbase/client/MetaScanner.java

@@ -33,8 +33,11 @@ import java.io.IOException;
  * Scanner class that contains the <code>.META.</code> table scanning logic
  * and uses a Retryable scanner. Provided visitors will be called
  * for each row.
+ * 
+ * Although public visibility, this is not a public-facing API and may evolve in
+ * minor releases.
  */
-class MetaScanner {
+public class MetaScanner {
 
   /**
    * Scans the meta table and calls a visitor on each RowResult and uses a empty
@@ -160,7 +163,7 @@ class MetaScanner {
   /**
    * Visitor class called to process each row of the .META. table
    */
-  interface MetaScannerVisitor {
+  public interface MetaScannerVisitor {
     /**
      * Visitor method that accepts a RowResult and the meta region location.
      * Implementations can return false to stop the region's loop if it becomes

src/main/java/org/apache/hadoop/hbase/master/HMaster.java

@@ -33,6 +33,7 @@ import java.util.NavigableMap;
 import java.util.Set;
 import java.util.SortedMap;
 import java.util.concurrent.atomic.AtomicBoolean;
+import java.util.concurrent.atomic.AtomicReference;
 import java.util.concurrent.locks.Lock;
 import java.util.concurrent.locks.ReentrantLock;
 
@@ -62,6 +63,8 @@ import org.apache.hadoop.hbase.TableExistsException;
 import org.apache.hadoop.hbase.client.Get;
 import org.apache.hadoop.hbase.client.HBaseAdmin;
 import org.apache.hadoop.hbase.client.Result;
+import org.apache.hadoop.hbase.client.MetaScanner;
+import org.apache.hadoop.hbase.client.MetaScanner.MetaScannerVisitor;
 import org.apache.hadoop.hbase.client.Scan;
 import org.apache.hadoop.hbase.client.ServerConnection;
 import org.apache.hadoop.hbase.client.ServerConnectionManager;
@@ -97,6 +100,8 @@ import org.apache.zookeeper.Watcher;
 import org.apache.zookeeper.Watcher.Event.EventType;
 import org.apache.zookeeper.Watcher.Event.KeeperState;
 
+import com.google.common.collect.Lists;
+
 /**
  * HMaster is the "master server" for HBase. An HBase cluster has one active
  * master.  If many masters are started, all compete.  Whichever wins goes on to
@@ -838,99 +843,94 @@ public class HMaster extends Thread implements HMasterInterface,
     new ChangeTableState(this, tableName, false).process();
   }
 
-  // TODO: Redo so this method does not duplicate code with subsequent methods.
+  /**
+   * Get a list of the regions for a given table. The pairs may have
+   * null for their second element in the case that they are not
+   * currently deployed.
+   * TODO: Redo so this method does not duplicate code with subsequent methods.
+   */
   List<Pair<HRegionInfo,HServerAddress>> getTableRegions(
       final byte [] tableName)
   throws IOException {
-    List<Pair<HRegionInfo,HServerAddress>> result =
+    final ArrayList<Pair<HRegionInfo, HServerAddress>> result =
-      new ArrayList<Pair<HRegionInfo,HServerAddress>>();
+      Lists.newArrayList();
-    Set<MetaRegion> regions =
+    MetaScannerVisitor visitor =
-      this.regionManager.getMetaRegionsForTable(tableName);
+      new MetaScannerVisitor() {
-    byte [] firstRowInTable = Bytes.toBytes(Bytes.toString(tableName) + ",,");
+        @Override
-    for (MetaRegion m: regions) {
+        public boolean processRow(Result data) throws IOException {
-      byte [] metaRegionName = m.getRegionName();
-      HRegionInterface srvr =
-        this.connection.getHRegionConnection(m.getServer());
-      Scan scan = new Scan(firstRowInTable);
-      scan.addColumn(HConstants.CATALOG_FAMILY,
-          HConstants.REGIONINFO_QUALIFIER);
-      scan.addColumn(HConstants.CATALOG_FAMILY, HConstants.SERVER_QUALIFIER);
-      // TODO: Use caching.
-      long scannerid = srvr.openScanner(metaRegionName, scan);
-      try {
-        while (true) {
-          Result data = srvr.next(scannerid);
           if (data == null || data.size() <= 0)
-            break;
+            return true;
-          HRegionInfo info = Writables.getHRegionInfo(
+          Pair<HRegionInfo, HServerAddress> pair =
-              data.getValue(HConstants.CATALOG_FAMILY,
+            metaRowToRegionPair(data);
-                  HConstants.REGIONINFO_QUALIFIER));
+          if (pair == null) return false;
-          if (Bytes.equals(info.getTableDesc().getName(), tableName)) {
+          if (!Bytes.equals(pair.getFirst().getTableDesc().getName(),
-            final byte[] value = data.getValue(HConstants.CATALOG_FAMILY,
+                tableName)) {
-                HConstants.SERVER_QUALIFIER);
+            return false;
-            if (value != null && value.length > 0) {
-              HServerAddress server = new HServerAddress(Bytes.toString(value));
-              result.add(new Pair<HRegionInfo,HServerAddress>(info, server));
-            }
-          } else {
-            break;
           }
+          result.add(pair);
+          return true;
         }
-      } finally {
+    };
-        srvr.close(scannerid);
+
-      }
+    MetaScanner.metaScan(conf, visitor, tableName); 
-    }
     return result;
   }
   
-  Pair<HRegionInfo,HServerAddress> getTableRegionClosest(
+  private Pair<HRegionInfo, HServerAddress> metaRowToRegionPair(
+      Result data) throws IOException {
+    HRegionInfo info = Writables.getHRegionInfo(
+        data.getValue(HConstants.CATALOG_FAMILY,
+            HConstants.REGIONINFO_QUALIFIER));
+    final byte[] value = data.getValue(HConstants.CATALOG_FAMILY,
+        HConstants.SERVER_QUALIFIER);
+    if (value != null && value.length > 0) {
+      HServerAddress server = new HServerAddress(Bytes.toString(value));
+      return new Pair<HRegionInfo,HServerAddress>(info, server);
+    } else {
+      //undeployed
+      return new Pair<HRegionInfo, HServerAddress>(info, null);
+    }    
+  }
+
+  /**
+   * Return the region and current deployment for the region containing
+   * the given row. If the region cannot be found, returns null. If it
+   * is found, but not currently deployed, the second element of the pair
+   * may be null.
+   */
+  Pair<HRegionInfo,HServerAddress> getTableRegionForRow(
       final byte [] tableName, final byte [] rowKey)
   throws IOException {
-    Set<MetaRegion> regions =
+    final AtomicReference<Pair<HRegionInfo, HServerAddress>> result =
-      this.regionManager.getMetaRegionsForTable(tableName);
+      new AtomicReference<Pair<HRegionInfo, HServerAddress>>(null);
-    for (MetaRegion m: regions) {
+    
-      byte [] firstRowInTable = Bytes.toBytes(Bytes.toString(tableName) + ",,");
+    MetaScannerVisitor visitor =
-      byte [] metaRegionName = m.getRegionName();
+      new MetaScannerVisitor() {
-      HRegionInterface srvr = this.connection.getHRegionConnection(m.getServer());
+        @Override
-      Scan scan = new Scan(firstRowInTable);
+        public boolean processRow(Result data) throws IOException {
-      scan.addColumn(HConstants.CATALOG_FAMILY,
-          HConstants.REGIONINFO_QUALIFIER);
-      scan.addColumn(HConstants.CATALOG_FAMILY, HConstants.SERVER_QUALIFIER);
-      long scannerid = srvr.openScanner(metaRegionName, scan);
-      try {
-        while (true) {
-          Result data = srvr.next(scannerid);
           if (data == null || data.size() <= 0)
-            break;
+            return true;
-          HRegionInfo info = Writables.getHRegionInfo(
+          Pair<HRegionInfo, HServerAddress> pair =
-              data.getValue(HConstants.CATALOG_FAMILY,
+            metaRowToRegionPair(data);
-                  HConstants.REGIONINFO_QUALIFIER));
+          if (pair == null) return false;
-          if (Bytes.compareTo(info.getTableDesc().getName(), tableName) == 0) {
+          if (!Bytes.equals(pair.getFirst().getTableDesc().getName(),
-            if ((Bytes.compareTo(info.getStartKey(), rowKey) >= 0) &&
+                tableName)) {
-                (Bytes.compareTo(info.getEndKey(), rowKey) < 0)) {
+            return false;
-                final byte[] value = data.getValue(HConstants.CATALOG_FAMILY,
-                    HConstants.SERVER_QUALIFIER);
-                if (value != null && value.length > 0) {
-                  HServerAddress server =
-                    new HServerAddress(Bytes.toString(value));
-                  return new Pair<HRegionInfo,HServerAddress>(info, server);
-                }
-            }
-          } else {
-            break;
           }
+          result.set(pair);
+          return true;
         }
-      } finally {
+    };
-        srvr.close(scannerid);
+
-      }
+    MetaScanner.metaScan(conf, visitor, tableName, rowKey, 1);
-    }
+    return result.get();
-    return null;
   }
   
   Pair<HRegionInfo,HServerAddress> getTableRegionFromName(
       final byte [] regionName)
   throws IOException {
     byte [] tableName = HRegionInfo.parseRegionName(regionName)[0];
+    
     Set<MetaRegion> regions = regionManager.getMetaRegionsForTable(tableName);
     for (MetaRegion m: regions) {
       byte [] metaRegionName = m.getRegionName();
@@ -941,16 +941,7 @@ public class HMaster extends Thread implements HMasterInterface,
       get.addColumn(HConstants.CATALOG_FAMILY, HConstants.SERVER_QUALIFIER);
       Result data = srvr.get(metaRegionName, get);
       if(data == null || data.size() <= 0) continue;
-      HRegionInfo info = Writables.getHRegionInfo(
+      return metaRowToRegionPair(data);
-          data.getValue(HConstants.CATALOG_FAMILY,
-              HConstants.REGIONINFO_QUALIFIER));
-      final byte[] value = data.getValue(HConstants.CATALOG_FAMILY,
-          HConstants.SERVER_QUALIFIER);
-      if(value != null && value.length > 0) {
-        HServerAddress server =
-          new HServerAddress(Bytes.toString(value));
-        return new Pair<HRegionInfo,HServerAddress>(info, server);
-      }
     }
     return null;
   }
@@ -1008,16 +999,18 @@ public class HMaster extends Thread implements HMasterInterface,
           pair = getTableRegionFromName(regionName);
         } else {
           byte [] rowKey = ((ImmutableBytesWritable)args[0]).get();
-          pair = getTableRegionClosest(tableName, rowKey);
+          pair = getTableRegionForRow(tableName, rowKey);
         }
-        if (pair != null) {
+        if (pair != null && pair.getSecond() != null) {
           this.regionManager.startAction(pair.getFirst().getRegionName(),
             pair.getFirst(), pair.getSecond(), op);
         }
       } else {
-        for (Pair<HRegionInfo,HServerAddress> pair: getTableRegions(tableName))
+        for (Pair<HRegionInfo,HServerAddress> pair: getTableRegions(tableName)) {
+          if (pair.getSecond() == null) continue; // undeployed
           this.regionManager.startAction(pair.getFirst().getRegionName(),
             pair.getFirst(), pair.getSecond(), op);
+        }
       }
       break;
 

src/test/java/org/apache/hadoop/hbase/master/TestMaster.java

@@ -21,6 +21,7 @@ package org.apache.hadoop.hbase.master;
 
 import org.apache.commons.logging.Log;
 import org.apache.commons.logging.LogFactory;
+import org.apache.hadoop.hbase.HConstants;
 import org.apache.hadoop.hbase.MiniHBaseCluster;
 import org.apache.hadoop.hbase.HMsg;
 import org.apache.hadoop.hbase.HServerInfo;
@@ -36,12 +37,16 @@ import org.apache.hadoop.hbase.util.Bytes;
 import org.apache.hadoop.hbase.util.Pair;
 
 import java.io.IOException;
+import java.util.List;
 import java.util.concurrent.CountDownLatch;
 import java.util.concurrent.TimeUnit;
 
 import org.junit.AfterClass;
 import org.junit.BeforeClass;
 import org.junit.Test;
+
+import com.google.common.base.Joiner;
+
 import static org.junit.Assert.*;
 
 public class TestMaster {
@@ -70,30 +75,38 @@ public class TestMaster {
     TEST_UTIL.createTable(TABLENAME, FAMILYNAME);
     TEST_UTIL.loadTable(new HTable(TABLENAME), FAMILYNAME);
 
+    List<Pair<HRegionInfo, HServerAddress>> tableRegions =
+      m.getTableRegions(TABLENAME);
+    LOG.info("Regions after load: " + Joiner.on(',').join(tableRegions));
+    assertEquals(1, tableRegions.size());
+    assertArrayEquals(HConstants.EMPTY_START_ROW,
+        tableRegions.get(0).getFirst().getStartKey());
+    assertArrayEquals(HConstants.EMPTY_END_ROW,
+        tableRegions.get(0).getFirst().getEndKey());
+
+    // Now trigger a split and stop when the split is in progress
+    
     CountDownLatch aboutToOpen = new CountDownLatch(1);
     CountDownLatch proceed = new CountDownLatch(1);
     RegionOpenListener list = new RegionOpenListener(aboutToOpen, proceed);
     HBaseEventHandler.registerListener(list);
 
+    LOG.info("Splitting table");
     admin.split(TABLENAME);
+    LOG.info("Waiting for split result to be about to open");
     aboutToOpen.await(60, TimeUnit.SECONDS);
-
     try {
-      m.getTableRegions(TABLENAME);
+      LOG.info("Making sure we can call getTableRegions while opening");
+      tableRegions = m.getTableRegions(TABLENAME);
+      LOG.info("Regions: " + Joiner.on(',').join(tableRegions));
+      // We have three regions because one is split-in-progress
+      assertEquals(3, tableRegions.size());
+      LOG.info("Making sure we can call getTableRegionClosest while opening");
       Pair<HRegionInfo,HServerAddress> pair =
-        m.getTableRegionClosest(TABLENAME, Bytes.toBytes("cde"));
+        m.getTableRegionForRow(TABLENAME, Bytes.toBytes("cde"));
-      /**
+      LOG.info("Result is: " + pair);
-       * TODO: The assertNull below used to work before moving all RS->M 
+      Pair<HRegionInfo, HServerAddress> tableRegionFromName = m.getTableRegionFromName(pair.getFirst().getRegionName());
-       * communication to ZK, find out why this test's behavior has changed.
+      assertEquals(tableRegionFromName.getFirst(), pair.getFirst());
-       * Tracked in HBASE-2656.
-        assertNull(pair);
-        assertNotNull(pair);
-        
-        We used to assert NotNull for the pair but it seems that ain't
-        always true either. For now disabling this assertion.  Filing
-        an issue for it to be checked -- St.Ack.
-      */
-      m.getTableRegionFromName(pair.getFirst().getRegionName());
     } finally {
       proceed.countDown();
     }