From ef4d353ab50e1fe8f1ee9f51e5f5fe942306a7e3 Mon Sep 17 00:00:00 2001 From: Ryan Rawson Date: Sat, 15 May 2010 00:23:10 +0000 Subject: [PATCH] HBASE-2509 NPEs in various places, HRegion.get, HRS.close git-svn-id: https://svn.apache.org/repos/asf/hadoop/hbase/trunk@944533 13f79535-47bb-0310-9956-ffa450edef68 --- .../hadoop/hbase/regionserver/HRegion.java | 10 +- .../hbase/regionserver/TestHRegion.java | 108 ++++++++++++++++-- 2 files changed, 105 insertions(+), 13 deletions(-) diff --git a/core/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java b/core/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java index 74c6b0ddfa5..a385ce1f767 100644 --- a/core/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java +++ b/core/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java @@ -1921,7 +1921,8 @@ public class HRegion implements HConstants, HeapSize { // , Writable{ * It is used to combine scanners from multiple Stores (aka column families). */ class RegionScanner implements InternalScanner { - private KeyValueHeap storeHeap = null; + // Package local for testability + KeyValueHeap storeHeap = null; private final byte [] stopRow; private Filter filter; private List results = new ArrayList(); @@ -2091,8 +2092,11 @@ public class HRegion implements HConstants, HeapSize { // , Writable{ } public synchronized void close() { - storeHeap.close(); - this.filterClosed = true; + if (storeHeap != null) { + storeHeap.close(); + storeHeap = null; + } + this.filterClosed = true; } /** diff --git a/core/src/test/java/org/apache/hadoop/hbase/regionserver/TestHRegion.java b/core/src/test/java/org/apache/hadoop/hbase/regionserver/TestHRegion.java index 6d51b8f14b1..482caa900a1 100644 --- a/core/src/test/java/org/apache/hadoop/hbase/regionserver/TestHRegion.java +++ b/core/src/test/java/org/apache/hadoop/hbase/regionserver/TestHRegion.java @@ -41,11 +41,9 @@ import org.apache.hadoop.hbase.filter.CompareFilter.CompareOp; import org.apache.hadoop.hbase.filter.Filter; import org.apache.hadoop.hbase.filter.FilterList; import org.apache.hadoop.hbase.filter.PrefixFilter; -import org.apache.hadoop.hbase.filter.RowFilter; import org.apache.hadoop.hbase.filter.SingleColumnValueFilter; import org.apache.hadoop.hbase.regionserver.HRegion.RegionScanner; import org.apache.hadoop.hbase.util.Bytes; -import org.junit.Assert; import java.io.IOException; import java.util.ArrayList; @@ -54,6 +52,9 @@ import java.util.HashMap; import java.util.List; import java.util.Map; import java.util.TreeMap; +import java.util.concurrent.atomic.AtomicBoolean; +import java.util.concurrent.atomic.AtomicInteger; + /** * Basic stand-alone testing of HRegion. @@ -92,6 +93,93 @@ public class TestHRegion extends HBaseTestCase { // /tmp/testtable ////////////////////////////////////////////////////////////////////////////// + public void testGetWhileRegionClose() throws IOException { + HBaseConfiguration hc = initSplit(); + int numRows = 100; + byte [][] families = {fam1, fam2, fam3}; + + //Setting up region + String method = this.getName(); + initHRegion(tableName, method, hc, families); + + // Put data in region + final int startRow = 100; + putData(startRow, numRows, qual1, families); + putData(startRow, numRows, qual2, families); + putData(startRow, numRows, qual3, families); + // this.region.flushcache(); + final AtomicBoolean done = new AtomicBoolean(false); + final AtomicInteger gets = new AtomicInteger(0); + GetTillDoneOrException [] threads = new GetTillDoneOrException[10]; + try { + // Set ten threads running concurrently getting from the region. + for (int i = 0; i < threads.length / 2; i++) { + threads[i] = new GetTillDoneOrException(i, Bytes.toBytes("" + startRow), + done, gets); + threads[i].setDaemon(true); + threads[i].start(); + } + // Artificially make the condition by setting closing flag explicitly. + // I can't make the issue happen with a call to region.close(). + this.region.closing.set(true); + for (int i = threads.length / 2; i < threads.length; i++) { + threads[i] = new GetTillDoneOrException(i, Bytes.toBytes("" + startRow), + done, gets); + threads[i].setDaemon(true); + threads[i].start(); + } + } finally { + if (this.region != null) { + this.region.close(); + this.region.getLog().closeAndDelete(); + } + } + done.set(true); + for (GetTillDoneOrException t: threads) { + try { + t.join(); + } catch (InterruptedException e) { + e.printStackTrace(); + } + if (t.e != null) { + LOG.info("Exception=" + t.e); + assertFalse("Found a NPE in " + t.getName(), + t.e instanceof NullPointerException); + } + } + } + + /* + * Thread that does get on single row until 'done' flag is flipped. If an + * exception causes us to fail, it records it. + */ + class GetTillDoneOrException extends Thread { + private final Get g; + private final AtomicBoolean done; + private final AtomicInteger count; + private Exception e; + + GetTillDoneOrException(final int i, final byte[] r, final AtomicBoolean d, + final AtomicInteger c) { + super("getter." + i); + this.g = new Get(r); + this.done = d; + this.count = c; + } + + @Override + public void run() { + while (!this.done.get()) { + try { + assertTrue(region.get(g, null).size() > 0); + this.count.incrementAndGet(); + } catch (Exception e) { + this.e = e; + break; + } + } + } + } /* * An involved filter test. Has multiple column families and deletes in mix. @@ -1034,13 +1122,13 @@ public class TestHRegion extends HBaseTestCase { scan.addFamily(fam4); is = (RegionScanner) region.getScanner(scan); is.initHeap(); // i dont like this test - assertEquals(1, ((RegionScanner)is).getStoreHeap().getHeap().size()); - + assertEquals(1, ((RegionScanner)is).storeHeap.getHeap().size()); + scan = new Scan(); is = (RegionScanner) region.getScanner(scan); is.initHeap(); assertEquals(families.length -1, - ((RegionScanner)is).getStoreHeap().getHeap().size()); + ((RegionScanner)is).storeHeap.getHeap().size()); } public void testRegionScanner_Next() throws IOException { @@ -1921,7 +2009,7 @@ public class TestHRegion extends HBaseTestCase { if (!toggle) { flushThread.flush(); } - Assert.assertEquals("i=" + i, expectedCount, res.size()); + assertEquals("i=" + i, expectedCount, res.size()); toggle = !toggle; } } @@ -1944,7 +2032,7 @@ public class TestHRegion extends HBaseTestCase { public void checkNoError() { if (error != null) { - Assert.assertNull(error); + assertNull(error); } } @@ -2082,7 +2170,7 @@ public class TestHRegion extends HBaseTestCase { public void checkNoError() { if (error != null) { - Assert.assertNull(error); + assertNull(error); } } @@ -2175,7 +2263,7 @@ public class TestHRegion extends HBaseTestCase { boolean previousEmpty = result == null || result.isEmpty(); result = region.get(get, null); if (!result.isEmpty() || !previousEmpty || i > compactInterval) { - Assert.assertEquals("i=" + i, expectedCount, result.size()); + assertEquals("i=" + i, expectedCount, result.size()); // TODO this was removed, now what dangit?! // search looking for the qualifier in question? long timestamp = 0; @@ -2185,7 +2273,7 @@ public class TestHRegion extends HBaseTestCase { timestamp = kv.getTimestamp(); } } - Assert.assertTrue(timestamp >= prevTimestamp); + assertTrue(timestamp >= prevTimestamp); prevTimestamp = timestamp; byte [] gotValue = null;