diff --git src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java index 3bd75b8..a2d4661 100644 --- src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java +++ src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java @@ -711,8 +711,8 @@ public class HRegion implements HeapSize { // , Writable{ wasFlushing = writestate.flushing; LOG.debug("Closing " + this + ": disabling compactions & flushes"); while (writestate.compacting > 0 || writestate.flushing) { - LOG.debug("waiting for " + writestate.compacting + " compactions" + - (writestate.flushing ? " & cache flush" : "") + + LOG.debug("Waiting for " + writestate.compacting + " compaction(s)" + + (writestate.flushing ? " & a cache flush" : "") + " to complete for region " + this); try { writestate.wait(); @@ -1147,7 +1147,7 @@ public class HRegion implements HeapSize { // , Writable{ LOG.debug("Started memstore flush for " + this + ", current region memstore size " + StringUtils.humanReadableInt(this.memstoreSize.get()) + - ((wal != null)? "": "; wal is null, using passed sequenceid=" + myseqid)); + ((wal != null)? "": "; wal is null, using passed seqid=" + myseqid)); } // Stop updates while we snapshot the memstore of all stores. We only have diff --git src/main/java/org/apache/hadoop/hbase/regionserver/wal/HLog.java src/main/java/org/apache/hadoop/hbase/regionserver/wal/HLog.java index 623edbe..cfd4c9a 100644 --- src/main/java/org/apache/hadoop/hbase/regionserver/wal/HLog.java +++ src/main/java/org/apache/hadoop/hbase/regionserver/wal/HLog.java @@ -758,7 +758,7 @@ public class HLog implements Syncable { /* * @return Logs older than this id are safe to remove. */ - private Long getOldestOutstandingSeqNum() { + Long getOldestOutstandingSeqNum() { return Collections.min(this.lastSeqWritten.values()); } @@ -1323,7 +1323,6 @@ public class HLog implements Syncable { return obtainSeqNum(); } - /** * Complete the cache flush * @@ -1353,17 +1352,11 @@ public class HLog implements Syncable { } // sync txn to file system this.sync(); - } finally { // updateLock not needed for removing snapshot's entry // Cleaning up of lastSeqWritten is in the finally clause because we // don't want to confuse getOldestOutstandingSeqNum() - this.lastSeqWritten.remove(getSnapshotName(encodedRegionName)); - Long l = this.lastSeqWritten.remove(encodedRegionName); - if (l != null) { - LOG.warn("Why is there a raw encodedRegionName in lastSeqWritten? name=" + - Bytes.toString(encodedRegionName) + ", seqid=" + l); - } + Long l = this.lastSeqWritten.remove(getSnapshotName(encodedRegionName)); this.cacheFlushLock.unlock(); } } diff --git src/test/java/org/apache/hadoop/hbase/regionserver/wal/TestHLog.java src/test/java/org/apache/hadoop/hbase/regionserver/wal/TestHLog.java index 16f0ee6..09aaf96 100644 --- src/test/java/org/apache/hadoop/hbase/regionserver/wal/TestHLog.java +++ src/test/java/org/apache/hadoop/hbase/regionserver/wal/TestHLog.java @@ -20,8 +20,9 @@ package org.apache.hadoop.hbase.regionserver.wal; import static org.junit.Assert.assertEquals; -import static org.junit.Assert.assertTrue; import static org.junit.Assert.assertNotNull; +import static org.junit.Assert.assertNull; +import static org.junit.Assert.assertTrue; import java.io.IOException; import java.lang.reflect.Method; @@ -38,19 +39,23 @@ import org.apache.hadoop.fs.FSDataOutputStream; import org.apache.hadoop.fs.FileStatus; import org.apache.hadoop.fs.FileSystem; import org.apache.hadoop.fs.Path; -import org.apache.hadoop.hbase.*; -import org.apache.hadoop.hbase.regionserver.wal.HLog.Reader; -import org.apache.hadoop.hbase.util.Bytes; -import org.apache.hadoop.hbase.util.FSUtils; import org.apache.hadoop.hbase.Coprocessor; +import org.apache.hadoop.hbase.HBaseTestingUtility; +import org.apache.hadoop.hbase.HColumnDescriptor; +import org.apache.hadoop.hbase.HConstants; +import org.apache.hadoop.hbase.HRegionInfo; +import org.apache.hadoop.hbase.HTableDescriptor; +import org.apache.hadoop.hbase.KeyValue; import org.apache.hadoop.hbase.coprocessor.CoprocessorHost; import org.apache.hadoop.hbase.coprocessor.SampleRegionWALObserver; +import org.apache.hadoop.hbase.regionserver.wal.HLog.Reader; +import org.apache.hadoop.hbase.util.Bytes; +import org.apache.hadoop.hbase.util.FSUtils; import org.apache.hadoop.hdfs.DFSClient; import org.apache.hadoop.hdfs.DistributedFileSystem; import org.apache.hadoop.hdfs.MiniDFSCluster; import org.apache.hadoop.hdfs.protocol.FSConstants; import org.apache.hadoop.hdfs.server.datanode.DataNode; -import org.apache.hadoop.hdfs.server.namenode.FSNamesystem; import org.apache.hadoop.hdfs.server.namenode.LeaseManager; import org.apache.hadoop.io.SequenceFile; import org.apache.log4j.Level; @@ -122,12 +127,64 @@ public class TestHLog { oldLogDir = new Path(hbaseDir, ".oldlogs"); dir = new Path(hbaseDir, getName()); } + private static String getName() { // TODO Auto-generated method stub return "TestHLog"; } /** + * @see HBASE-4789 does overzealous pruning of seqids + * @throws IOException + */ + @Test + public void testRemoveLastSequenceWrittenOnClose() throws IOException { + final byte [] tableName = Bytes.toBytes("testRemoveLastSequenceWrittenOnClose"); + HLog log = new HLog(fs, dir, oldLogDir, conf); + HRegionInfo hri = new HRegionInfo(tableName, + HConstants.EMPTY_START_ROW, HConstants.EMPTY_END_ROW); + + // Add a single edit. Assert oldest seqid makes sense and that its + // what we see when we start a flush as the associated flush seqid. + addEdits(log, hri, tableName, 1); + long oldest = log.getOldestOutstandingSeqNum(); + long seqid = log.getSequenceNumber(); + assertEquals(oldest, seqid); + + // Flush the region and in between start and complete of flush add more edits + long currentseqid = startCacheFlush(log, hri, oldest); + addEdits(log, hri, tableName, 1); + assertEquals(oldest, log.getOldestOutstandingSeqNum().longValue()); + // Complete of flush should remove oldest. + log.completeCacheFlush(hri.getEncodedNameAsBytes(), tableName, currentseqid, false); + long newoldest = log.getOldestOutstandingSeqNum().longValue(); + assertTrue(oldest < newoldest); + + // OK. Now act like we're doing a close. During close no edits come in + // between flush start and end and we should therefore be letting go of + // oldest edit for the flushing region. + oldest = newoldest; + currentseqid = startCacheFlush(log, hri, oldest); + log.completeCacheFlush(hri.getEncodedNameAsBytes(), tableName, currentseqid, false); + assertNull(log.getOldestOutstandingSeqNum()); + } + + /** + * Start a cache flush and make sure of some assertions while at it. + * @param log + * @param hri + * @param oldest + * @return currentseqid + */ + private long startCacheFlush(final HLog log, final HRegionInfo hri, + final long oldest) { + long currentseqid = log.startCacheFlush(hri.getEncodedNameAsBytes()); + assertEquals(oldest, log.getOldestOutstandingSeqNum().longValue()); + assertEquals(oldest + 1, currentseqid); + return currentseqid; + } + + /** * Just write multiple logs then split. Before fix for HADOOP-2283, this * would fail. * @throws IOException @@ -173,7 +230,7 @@ public class TestHLog { } log.close(); HLogSplitter logSplitter = HLogSplitter.createLogSplitter(conf, - hbaseDir, logdir, this.oldLogDir, this.fs); + hbaseDir, logdir, TestHLog.oldLogDir, TestHLog.fs); List splits = logSplitter.splitLog(); verifySplits(splits, howmany); @@ -434,7 +491,7 @@ public class TestHLog { // Make sure you can read all the content SequenceFile.Reader reader - = new SequenceFile.Reader(this.fs, walPath, this.conf); + = new SequenceFile.Reader(TestHLog.fs, walPath, TestHLog.conf); int count = 0; HLogKey key = HLog.newKey(conf); WALEdit val = new WALEdit(); @@ -682,8 +739,8 @@ public class TestHLog { assertNotNull(c); } - private void addEdits(HLog log, HRegionInfo hri, byte [] tableName, - int times) throws IOException { + private void addEdits(HLog log, HRegionInfo hri, byte [] tableName, int times) + throws IOException { HTableDescriptor htd = new HTableDescriptor(); htd.addFamily(new HColumnDescriptor("row"));