Author: liyin Date: Wed Apr 23 18:18:22 2014 New Revision: 1589480 URL: http://svn.apache.org/r1589480 Log: [master] Fix negative sequenceid exception
Author: everyoung Summary: Fix negative sequenceid exception in HRegionSeqidTransanction when opening region with no families. The HLog sequence id always starts from 1, so using 0 to indicate an empty record is fine. Test Plan: All tests Reviewers: daviddeng, jiqingt, aaiyer, rshroff, liyintang, elliott Reviewed By: daviddeng CC: hbase-dev@ Differential Revision: https://phabricator.fb.com/D1289443 Task ID: 4192110 Modified: hbase/branches/0.89-fb/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java hbase/branches/0.89-fb/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionSeqidTransition.java hbase/branches/0.89-fb/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java hbase/branches/0.89-fb/src/test/java/org/apache/hadoop/hbase/regionserver/TestHRegionSeqidTransition.java Modified: hbase/branches/0.89-fb/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java URL: http://svn.apache.org/viewvc/hbase/branches/0.89-fb/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java?rev=1589480&r1=1589479&r2=1589480&view=diff ============================================================================== --- hbase/branches/0.89-fb/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java (original) +++ hbase/branches/0.89-fb/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java Wed Apr 23 18:18:22 2014 @@ -3307,9 +3307,9 @@ public class HRegion implements HeapSize // add seqid to hlog // to be strict, next sequence id is getSequenceNumber() + 1 // because of incrementAndGet in Hlog.java obtainSeqNum method - // after calling writeToHLog method nex line. + // after calling log.writeSeqidTransition next line. HRegionSeqidTransition seqidTransition = - new HRegionSeqidTransition(seqid-1, log.getSequenceNumber() + 1); + new HRegionSeqidTransition(Math.max(seqid-1, 0), log.getSequenceNumber() + 1); // no serverInfo b/c outside an HRS context log.writeSeqidTransition(seqidTransition, null, r.getRegionInfo()); } Modified: hbase/branches/0.89-fb/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionSeqidTransition.java URL: http://svn.apache.org/viewvc/hbase/branches/0.89-fb/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionSeqidTransition.java?rev=1589480&r1=1589479&r2=1589480&view=diff ============================================================================== --- hbase/branches/0.89-fb/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionSeqidTransition.java (original) +++ hbase/branches/0.89-fb/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionSeqidTransition.java Wed Apr 23 18:18:22 2014 @@ -35,12 +35,12 @@ public final class HRegionSeqidTransitio private final long nextSeqid; public HRegionSeqidTransition(final long lastSeqid, final long nextSeqid) { - if ((lastSeqid >= 0) && (nextSeqid >= 0)) { - this.lastSeqid = lastSeqid; - this.nextSeqid = nextSeqid; - } else { + if (lastSeqid < 0 || nextSeqid < 0) { throw new IllegalArgumentException("Seqids cannot be negative!"); } + + this.lastSeqid = lastSeqid; + this.nextSeqid = nextSeqid; } public long getLastSeqid() { @@ -60,18 +60,19 @@ public final class HRegionSeqidTransitio if (fromByte == null) { return null; } - if (fromByte.length >= 3 * Bytes.SIZEOF_LONG) { - if (Bytes.toLong(fromByte, 0) == VERSION) { - return new HRegionSeqidTransition( - Bytes.toLong(fromByte, Bytes.SIZEOF_LONG), - Bytes.toLong(fromByte, 2 * Bytes.SIZEOF_LONG)); - } else { - // can be changed to deal versions later - throw new IllegalArgumentException("Only version 1 exists!!"); - } - } else { + + if (fromByte.length < 3 * Bytes.SIZEOF_LONG) { throw new IllegalArgumentException("Bytes array too short!"); } + + if (Bytes.toLong(fromByte, 0) != VERSION) { + // can be changed to deal versions later + throw new IllegalArgumentException("Only version 1 exists!!"); + } + + return new HRegionSeqidTransition( + Bytes.toLong(fromByte, Bytes.SIZEOF_LONG), + Bytes.toLong(fromByte, 2 * Bytes.SIZEOF_LONG)); } /** Modified: hbase/branches/0.89-fb/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java URL: http://svn.apache.org/viewvc/hbase/branches/0.89-fb/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java?rev=1589480&r1=1589479&r2=1589480&view=diff ============================================================================== --- hbase/branches/0.89-fb/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java (original) +++ hbase/branches/0.89-fb/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java Wed Apr 23 18:18:22 2014 @@ -2545,8 +2545,11 @@ public class HRegionServer implements HR // If hlog.seqid = 100, seqid = 200, after hlog.setSequenceNumber(200), // hlog.getSequenceNumber may return 210 if there're 10 edits inbound. // The transition seqid is still valid + // to be strict, next sequence id is getSequenceNumber() + 1 + // because of incrementAndGet in Hlog.java obtainSeqNum method + // after calling hlog.writeSeqidTransition method next line. HRegionSeqidTransition seqidTransition = - new HRegionSeqidTransition(seqid - 1, hlog.getSequenceNumber()); + new HRegionSeqidTransition(Math.max(seqid-1, 0), hlog.getSequenceNumber() + 1); LOG.info("Sequence id of region " + regionInfo.getRegionNameAsString() + " has a transition from " + seqidTransition.getLastSeqid() + " to " + seqidTransition.getNextSeqid() + Modified: hbase/branches/0.89-fb/src/test/java/org/apache/hadoop/hbase/regionserver/TestHRegionSeqidTransition.java URL: http://svn.apache.org/viewvc/hbase/branches/0.89-fb/src/test/java/org/apache/hadoop/hbase/regionserver/TestHRegionSeqidTransition.java?rev=1589480&r1=1589479&r2=1589480&view=diff ============================================================================== --- hbase/branches/0.89-fb/src/test/java/org/apache/hadoop/hbase/regionserver/TestHRegionSeqidTransition.java (original) +++ hbase/branches/0.89-fb/src/test/java/org/apache/hadoop/hbase/regionserver/TestHRegionSeqidTransition.java Wed Apr 23 18:18:22 2014 @@ -31,9 +31,12 @@ import org.apache.hadoop.fs.FileSystem; import org.apache.hadoop.fs.Path; import org.apache.hadoop.hbase.HBaseTestingUtility; 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.MasterNotRunningException; import org.apache.hadoop.hbase.MiniHBaseCluster; +import org.apache.hadoop.hbase.client.HBaseAdmin; import org.apache.hadoop.hbase.client.HTable; import org.apache.hadoop.hbase.client.Result; import org.apache.hadoop.hbase.client.ResultScanner; @@ -103,6 +106,23 @@ public class TestHRegionSeqidTransition } } + @Test + public void testNegSeqid() throws IOException { + HTableDescriptor htd = new HTableDescriptor(TABLENAME + "NegSeqid"); + HRegionInfo hri = new HRegionInfo(htd, null, null); + HRegion.createHRegion(hri, TEST_UTIL.getTestDir(), c); + HLog log = new HLog(TEST_UTIL.getTestFileSystem(), + new Path(TEST_UTIL.getTestDir(), HConstants.HREGION_LOGDIR_NAME), + new Path(TEST_UTIL.getTestDir(), HConstants.HREGION_OLDLOGDIR_NAME), + c, null); + try { + HRegion.openHRegion(hri, TEST_UTIL.getTestDir(), log, c); + } catch (IllegalArgumentException e) { + assertTrue("Got negative seqid exception when opening region with no families!.", false); + } + HRegion.deleteRegion(TEST_UTIL.getTestFileSystem(), TEST_UTIL.getTestDir(), hri); + } + @Test(timeout=300000) public void testSeqidTransitionOnRegionShutdown() throws Exception {
