From ebbdb536c7cd1cf1ab80e68d59b20e8ad421f6ad Mon Sep 17 00:00:00 2001 From: Todd Lipcon Date: Mon, 21 Jun 2010 20:36:39 +0000 Subject: [PATCH] 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 | 2 + .../org/apache/hadoop/hbase/HRegionInfo.java | 9 + .../hadoop/hbase/client/MetaScanner.java | 7 +- .../apache/hadoop/hbase/master/HMaster.java | 177 +++++++++--------- .../hadoop/hbase/master/TestMaster.java | 43 +++-- 5 files changed, 129 insertions(+), 109 deletions(-) diff --git a/CHANGES.txt b/CHANGES.txt index 8f0d454339f..bcd16c635c3 100644 --- a/CHANGES.txt +++ b/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 diff --git a/src/main/java/org/apache/hadoop/hbase/HRegionInfo.java b/src/main/java/org/apache/hadoop/hbase/HRegionInfo.java index d6f96115004..ee946900a51 100644 --- a/src/main/java/org/apache/hadoop/hbase/HRegionInfo.java +++ b/src/main/java/org/apache/hadoop/hbase/HRegionInfo.java @@ -423,6 +423,15 @@ public class HRegionInfo extends VersionedWritable implements WritableComparable Bytes.equals(endKey, HConstants.EMPTY_BYTE_ARRAY); 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(){ diff --git a/src/main/java/org/apache/hadoop/hbase/client/MetaScanner.java b/src/main/java/org/apache/hadoop/hbase/client/MetaScanner.java index 4c0f3929d60..bf308b3f6b9 100644 --- a/src/main/java/org/apache/hadoop/hbase/client/MetaScanner.java +++ b/src/main/java/org/apache/hadoop/hbase/client/MetaScanner.java @@ -33,8 +33,11 @@ import java.io.IOException; * Scanner class that contains the .META. 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 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 66dc6970ca3..12b9ce84f69 100644 --- a/src/main/java/org/apache/hadoop/hbase/master/HMaster.java +++ b/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> getTableRegions( final byte [] tableName) throws IOException { - List> result = - new ArrayList>(); - Set regions = - this.regionManager.getMetaRegionsForTable(tableName); - byte [] firstRowInTable = Bytes.toBytes(Bytes.toString(tableName) + ",,"); - for (MetaRegion m: regions) { - 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); + final ArrayList> result = + Lists.newArrayList(); + MetaScannerVisitor visitor = + new MetaScannerVisitor() { + @Override + public boolean processRow(Result data) throws IOException { if (data == null || data.size() <= 0) - break; - HRegionInfo info = Writables.getHRegionInfo( - data.getValue(HConstants.CATALOG_FAMILY, - HConstants.REGIONINFO_QUALIFIER)); - if (Bytes.equals(info.getTableDesc().getName(), tableName)) { - final byte[] value = data.getValue(HConstants.CATALOG_FAMILY, - HConstants.SERVER_QUALIFIER); - if (value != null && value.length > 0) { - HServerAddress server = new HServerAddress(Bytes.toString(value)); - result.add(new Pair(info, server)); - } - } else { - break; + return true; + Pair pair = + metaRowToRegionPair(data); + if (pair == null) return false; + if (!Bytes.equals(pair.getFirst().getTableDesc().getName(), + tableName)) { + return false; } + result.add(pair); + return true; } - } finally { - srvr.close(scannerid); - } - } + }; + + MetaScanner.metaScan(conf, visitor, tableName); return result; } - - Pair getTableRegionClosest( - final byte [] tableName, final byte [] rowKey) - throws IOException { - Set regions = - this.regionManager.getMetaRegionsForTable(tableName); - for (MetaRegion m: regions) { - byte [] firstRowInTable = Bytes.toBytes(Bytes.toString(tableName) + ",,"); - 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); - long scannerid = srvr.openScanner(metaRegionName, scan); - try { - while (true) { - Result data = srvr.next(scannerid); - if (data == null || data.size() <= 0) - break; - HRegionInfo info = Writables.getHRegionInfo( - data.getValue(HConstants.CATALOG_FAMILY, - HConstants.REGIONINFO_QUALIFIER)); - if (Bytes.compareTo(info.getTableDesc().getName(), tableName) == 0) { - if ((Bytes.compareTo(info.getStartKey(), rowKey) >= 0) && - (Bytes.compareTo(info.getEndKey(), rowKey) < 0)) { - 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(info, server); - } - } - } else { - break; - } - } - } finally { - srvr.close(scannerid); - } - } - return null; + + private Pair 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(info, server); + } else { + //undeployed + return new Pair(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 getTableRegionForRow( + final byte [] tableName, final byte [] rowKey) + throws IOException { + final AtomicReference> result = + new AtomicReference>(null); + + MetaScannerVisitor visitor = + new MetaScannerVisitor() { + @Override + public boolean processRow(Result data) throws IOException { + if (data == null || data.size() <= 0) + return true; + Pair pair = + metaRowToRegionPair(data); + if (pair == null) return false; + if (!Bytes.equals(pair.getFirst().getTableDesc().getName(), + tableName)) { + return false; + } + result.set(pair); + return true; + } + }; + + MetaScanner.metaScan(conf, visitor, tableName, rowKey, 1); + return result.get(); + } + Pair getTableRegionFromName( final byte [] regionName) throws IOException { byte [] tableName = HRegionInfo.parseRegionName(regionName)[0]; + Set 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( - 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(info, server); - } + return metaRowToRegionPair(data); } 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 pair: getTableRegions(tableName)) + for (Pair pair: getTableRegions(tableName)) { + if (pair.getSecond() == null) continue; // undeployed this.regionManager.startAction(pair.getFirst().getRegionName(), pair.getFirst(), pair.getSecond(), op); + } } break; diff --git a/src/test/java/org/apache/hadoop/hbase/master/TestMaster.java b/src/test/java/org/apache/hadoop/hbase/master/TestMaster.java index a5f4d6969a3..01ae0d241eb 100644 --- a/src/test/java/org/apache/hadoop/hbase/master/TestMaster.java +++ b/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> 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 pair = - m.getTableRegionClosest(TABLENAME, Bytes.toBytes("cde")); - /** - * TODO: The assertNull below used to work before moving all RS->M - * communication to ZK, find out why this test's behavior has changed. - * 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()); + m.getTableRegionForRow(TABLENAME, Bytes.toBytes("cde")); + LOG.info("Result is: " + pair); + Pair tableRegionFromName = m.getTableRegionFromName(pair.getFirst().getRegionName()); + assertEquals(tableRegionFromName.getFirst(), pair.getFirst()); } finally { proceed.countDown(); }