From 20bfb43db6871678bc805e42bc09875dc9480631 Mon Sep 17 00:00:00 2001 From: Guanghao Zhang Date: Thu, 19 Sep 2019 08:08:49 +0800 Subject: [PATCH] HBASE-23044 CatalogJanitor#cleanMergeQualifier may clean wrong parent regions (#637) Signed-off-by: Duo Zhang --- .../hadoop/hbase/MetaTableAccessor.java | 41 +--- .../hadoop/hbase/TestMetaTableAccessor.java | 195 +++++++++++------- 2 files changed, 130 insertions(+), 106 deletions(-) diff --git a/hbase-client/src/main/java/org/apache/hadoop/hbase/MetaTableAccessor.java b/hbase-client/src/main/java/org/apache/hadoop/hbase/MetaTableAccessor.java index 8c85383290e..ad54324c7b3 100644 --- a/hbase-client/src/main/java/org/apache/hadoop/hbase/MetaTableAccessor.java +++ b/hbase-client/src/main/java/org/apache/hadoop/hbase/MetaTableAccessor.java @@ -58,8 +58,6 @@ import org.apache.hadoop.hbase.client.coprocessor.Batch; import org.apache.hadoop.hbase.exceptions.DeserializationException; import org.apache.hadoop.hbase.filter.Filter; import org.apache.hadoop.hbase.filter.FirstKeyOnlyFilter; -import org.apache.hadoop.hbase.filter.QualifierFilter; -import org.apache.hadoop.hbase.filter.RegexStringComparator; import org.apache.hadoop.hbase.filter.RowFilter; import org.apache.hadoop.hbase.filter.SubstringComparator; import org.apache.hadoop.hbase.ipc.CoprocessorRpcUtils; @@ -373,7 +371,7 @@ public class MetaTableAccessor { @Nullable public static List getMergeRegions(Connection connection, byte[] regionName) throws IOException { - return getMergeRegions(getMergeRegionsRaw(connection, regionName)); + return getMergeRegions(getRegionResult(connection, regionName).rawCells()); } /** @@ -426,31 +424,6 @@ public class MetaTableAccessor { PrivateCellUtil.qualifierStartsWith(cell, HConstants.MERGE_QUALIFIER_PREFIX); } - /** - * @return Array of Cells made from all columns on the regionName row - * that match the regex 'info:merge.*'. - */ - @Nullable - private static Cell [] getMergeRegionsRaw(Connection connection, byte [] regionName) - throws IOException { - Scan scan = new Scan().withStartRow(regionName). - setOneRowLimit(). - readVersions(1). - addFamily(HConstants.CATALOG_FAMILY). - setFilter(new QualifierFilter(CompareOperator.EQUAL, - new RegexStringComparator(HConstants.MERGE_QUALIFIER_PREFIX_STR+ ".*"))); - try (Table m = getMetaHTable(connection); ResultScanner scanner = m.getScanner(scan)) { - // Should be only one result in this scanner if any. - Result result = scanner.next(); - if (result == null) { - return null; - } - // Should be safe to just return all Cells found since we had filter in place. - // All values should be RegionInfos or something wrong. - return result.rawCells(); - } - } - /** * Checks if the specified table exists. Looks at the hbase:meta table hosted on * the specified server. @@ -1837,21 +1810,23 @@ public class MetaTableAccessor { throws IOException { Delete delete = new Delete(mergeRegion.getRegionName()); // NOTE: We are doing a new hbase:meta read here. - Cell [] cells = getMergeRegionsRaw(connection, mergeRegion.getRegionName()); + Cell[] cells = getRegionResult(connection, mergeRegion.getRegionName()).rawCells(); if (cells == null || cells.length == 0) { return; } - List qualifiers = new ArrayList<>(cells.length); + List qualifiers = new ArrayList<>(); for (Cell cell : cells) { + if (!isMergeQualifierPrefix(cell)) { + continue; + } byte[] qualifier = CellUtil.cloneQualifier(cell); qualifiers.add(qualifier); delete.addColumns(getCatalogFamily(), qualifier, HConstants.LATEST_TIMESTAMP); } deleteFromMetaTable(connection, delete); LOG.info("Deleted merge references in " + mergeRegion.getRegionNameAsString() + - ", deleted qualifiers " + - qualifiers.stream().map(Bytes::toStringBinary). - collect(Collectors.joining(", "))); + ", deleted qualifiers " + qualifiers.stream().map(Bytes::toStringBinary). + collect(Collectors.joining(", "))); } public static Put addRegionInfo(final Put p, final RegionInfo hri) diff --git a/hbase-server/src/test/java/org/apache/hadoop/hbase/TestMetaTableAccessor.java b/hbase-server/src/test/java/org/apache/hadoop/hbase/TestMetaTableAccessor.java index 744987d9b26..087076fc05b 100644 --- a/hbase-server/src/test/java/org/apache/hadoop/hbase/TestMetaTableAccessor.java +++ b/hbase-server/src/test/java/org/apache/hadoop/hbase/TestMetaTableAccessor.java @@ -17,6 +17,13 @@ */ package org.apache.hadoop.hbase; +import static org.junit.Assert.assertArrayEquals; +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertNotEquals; +import static org.junit.Assert.assertNotNull; +import static org.junit.Assert.assertNull; +import static org.junit.Assert.assertTrue; import static org.mockito.ArgumentMatchers.anyObject; import static org.mockito.Mockito.doReturn; import static org.mockito.Mockito.mock; @@ -30,6 +37,8 @@ import java.util.HashMap; import java.util.List; import java.util.Map; import java.util.Random; +import java.util.concurrent.TimeUnit; + import org.apache.hadoop.conf.Configuration; import org.apache.hadoop.hbase.client.Admin; import org.apache.hadoop.hbase.client.Connection; @@ -58,7 +67,6 @@ import org.apache.hadoop.hbase.util.ManualEnvironmentEdge; import org.apache.hadoop.hbase.util.Pair; import org.apache.hadoop.hbase.zookeeper.MetaTableLocator; import org.junit.AfterClass; -import org.junit.Assert; import org.junit.BeforeClass; import org.junit.ClassRule; import org.junit.Rule; @@ -105,8 +113,49 @@ public class TestMetaTableAccessor { UTIL.shutdownMiniCluster(); } + /** + * Test for HBASE-23044. + */ @Test - public void testGettingMergeRegions() throws IOException { + public void testGetMergeRegions() throws Exception { + TableName tn = TableName.valueOf(this.name.getMethodName()); + UTIL.createMultiRegionTable(tn, Bytes.toBytes("CF"), 4); + UTIL.waitTableAvailable(tn); + try (Admin admin = UTIL.getAdmin()) { + List regions = admin.getRegions(tn); + assertEquals(4, regions.size()); + admin.mergeRegionsAsync(regions.get(0).getRegionName(), regions.get(1).getRegionName(), false) + .get(60, TimeUnit.SECONDS); + admin.mergeRegionsAsync(regions.get(2).getRegionName(), regions.get(3).getRegionName(), false) + .get(60, TimeUnit.SECONDS); + + List mergedRegions = admin.getRegions(tn); + assertEquals(2, mergedRegions.size()); + RegionInfo mergedRegion0 = mergedRegions.get(0); + RegionInfo mergedRegion1 = mergedRegions.get(1); + + List mergeParents = + MetaTableAccessor.getMergeRegions(connection, mergedRegion0.getRegionName()); + assertTrue(mergeParents.contains(regions.get(0))); + assertTrue(mergeParents.contains(regions.get(1))); + mergeParents = MetaTableAccessor.getMergeRegions(connection, mergedRegion1.getRegionName()); + assertTrue(mergeParents.contains(regions.get(2))); + assertTrue(mergeParents.contains(regions.get(3))); + + // Delete merge qualifiers for mergedRegion0, then cannot getMergeRegions again + MetaTableAccessor.deleteMergeQualifiers(connection, mergedRegion0); + mergeParents = MetaTableAccessor.getMergeRegions(connection, mergedRegion0.getRegionName()); + assertNull(mergeParents); + + mergeParents = MetaTableAccessor.getMergeRegions(connection, mergedRegion1.getRegionName()); + assertTrue(mergeParents.contains(regions.get(2))); + assertTrue(mergeParents.contains(regions.get(3))); + } + UTIL.deleteTable(tn); + } + + @Test + public void testAddMergeRegions() throws IOException { TableName tn = TableName.valueOf(this.name.getMethodName()); Put put = new Put(Bytes.toBytes(this.name.getMethodName())); List ris = new ArrayList<>(); @@ -120,12 +169,12 @@ public class TestMetaTableAccessor { put = MetaTableAccessor.addMergeRegions(put, ris); List cells = put.getFamilyCellMap().get(HConstants.CATALOG_FAMILY); String previousQualifier = null; - Assert.assertEquals(limit, cells.size()); + assertEquals(limit, cells.size()); for (Cell cell: cells) { LOG.info(cell.toString()); String qualifier = Bytes.toString(cell.getQualifierArray()); - Assert.assertTrue(qualifier.startsWith(HConstants.MERGE_QUALIFIER_PREFIX_STR)); - Assert.assertNotEquals(qualifier, previousQualifier); + assertTrue(qualifier.startsWith(HConstants.MERGE_QUALIFIER_PREFIX_STR)); + assertNotEquals(qualifier, previousQualifier); previousQualifier = qualifier; } } @@ -133,7 +182,7 @@ public class TestMetaTableAccessor { @Test public void testIsMetaWhenAllHealthy() throws InterruptedException { HMaster m = UTIL.getMiniHBaseCluster().getMaster(); - Assert.assertTrue(m.waitForMetaOnline()); + assertTrue(m.waitForMetaOnline()); } @Test @@ -142,7 +191,7 @@ public class TestMetaTableAccessor { int index = UTIL.getMiniHBaseCluster().getServerWithMeta(); HRegionServer rsWithMeta = UTIL.getMiniHBaseCluster().getRegionServer(index); rsWithMeta.abort("TESTING"); - Assert.assertTrue(m.waitForMetaOnline()); + assertTrue(m.waitForMetaOnline()); } /** @@ -187,8 +236,8 @@ public class TestMetaTableAccessor { try { // Make sure reader and writer are working. - Assert.assertTrue(reader.isProgressing()); - Assert.assertTrue(writer.isProgressing()); + assertTrue(reader.isProgressing()); + assertTrue(writer.isProgressing()); // Kill server hosting meta -- twice . See if our reader/writer ride over the // meta moves. They'll need to retry. @@ -207,8 +256,8 @@ public class TestMetaTableAccessor { } } - Assert.assertTrue("reader: " + reader.toString(), reader.isProgressing()); - Assert.assertTrue("writer: " + writer.toString(), writer.isProgressing()); + assertTrue("reader: " + reader.toString(), reader.isProgressing()); + assertTrue("writer: " + writer.toString(), writer.isProgressing()); } catch (IOException e) { throw e; } finally { @@ -219,7 +268,7 @@ public class TestMetaTableAccessor { t.close(); } long exeTime = System.currentTimeMillis() - startTime; - Assert.assertTrue("Timeout: test took " + exeTime / 1000 + " sec", exeTime < timeOut); + assertTrue("Timeout: test took " + exeTime / 1000 + " sec", exeTime < timeOut); } /** @@ -274,27 +323,27 @@ public class TestMetaTableAccessor { @Test public void testGetRegionsFromMetaTable() throws IOException, InterruptedException { List regions = MetaTableLocator.getMetaRegions(UTIL.getZooKeeperWatcher()); - Assert.assertTrue(regions.size() >= 1); - Assert.assertTrue( + assertTrue(regions.size() >= 1); + assertTrue( MetaTableLocator.getMetaRegionsAndLocations(UTIL.getZooKeeperWatcher()).size() >= 1); } @Test public void testTableExists() throws IOException { final TableName tableName = TableName.valueOf(name.getMethodName()); - Assert.assertFalse(MetaTableAccessor.tableExists(connection, tableName)); + assertFalse(MetaTableAccessor.tableExists(connection, tableName)); UTIL.createTable(tableName, HConstants.CATALOG_FAMILY); - Assert.assertTrue(MetaTableAccessor.tableExists(connection, tableName)); + assertTrue(MetaTableAccessor.tableExists(connection, tableName)); Admin admin = UTIL.getAdmin(); admin.disableTable(tableName); admin.deleteTable(tableName); - Assert.assertFalse(MetaTableAccessor.tableExists(connection, tableName)); - Assert.assertTrue(MetaTableAccessor.tableExists(connection, + assertFalse(MetaTableAccessor.tableExists(connection, tableName)); + assertTrue(MetaTableAccessor.tableExists(connection, TableName.META_TABLE_NAME)); UTIL.createTable(tableName, HConstants.CATALOG_FAMILY); - Assert.assertTrue(MetaTableAccessor.tableExists(connection, tableName)); + assertTrue(MetaTableAccessor.tableExists(connection, tableName)); admin.disableTable(tableName); admin.deleteTable(tableName); - Assert.assertFalse(MetaTableAccessor.tableExists(connection, tableName)); + assertFalse(MetaTableAccessor.tableExists(connection, tableName)); } @Test public void testGetRegion() throws IOException, InterruptedException { @@ -303,7 +352,7 @@ public class TestMetaTableAccessor { // Test get on non-existent region. Pair pair = MetaTableAccessor.getRegion(connection, Bytes.toBytes("nonexistent-region")); - Assert.assertNull(pair); + assertNull(pair); LOG.info("Finished " + name); } @@ -326,18 +375,18 @@ public class TestMetaTableAccessor { // Now make sure we only get the regions from 1 of the tables at a time - Assert.assertEquals(1, MetaTableAccessor.getTableRegions(connection, tableName).size()); - Assert.assertEquals(1, MetaTableAccessor.getTableRegions(connection, greaterName).size()); + assertEquals(1, MetaTableAccessor.getTableRegions(connection, tableName).size()); + assertEquals(1, MetaTableAccessor.getTableRegions(connection, greaterName).size()); } private static List testGettingTableRegions(final Connection connection, final TableName name, final int regionCount) throws IOException, InterruptedException { List regions = MetaTableAccessor.getTableRegions(connection, name); - Assert.assertEquals(regionCount, regions.size()); + assertEquals(regionCount, regions.size()); Pair pair = MetaTableAccessor.getRegion(connection, regions.get(0).getRegionName()); - Assert.assertEquals(regions.get(0).getEncodedName(), + assertEquals(regions.get(0).getEncodedName(), pair.getFirst().getEncodedName()); return regions; } @@ -347,48 +396,48 @@ public class TestMetaTableAccessor { throws IOException, InterruptedException { Pair pair = MetaTableAccessor.getRegion(connection, region.getRegionName()); - Assert.assertEquals(region.getEncodedName(), + assertEquals(region.getEncodedName(), pair.getFirst().getEncodedName()); } @Test public void testParseReplicaIdFromServerColumn() { String column1 = HConstants.SERVER_QUALIFIER_STR; - Assert.assertEquals(0, + assertEquals(0, MetaTableAccessor.parseReplicaIdFromServerColumn(Bytes.toBytes(column1))); String column2 = column1 + MetaTableAccessor.META_REPLICA_ID_DELIMITER; - Assert.assertEquals(-1, + assertEquals(-1, MetaTableAccessor.parseReplicaIdFromServerColumn(Bytes.toBytes(column2))); String column3 = column2 + "00"; - Assert.assertEquals(-1, + assertEquals(-1, MetaTableAccessor.parseReplicaIdFromServerColumn(Bytes.toBytes(column3))); String column4 = column3 + "2A"; - Assert.assertEquals(42, + assertEquals(42, MetaTableAccessor.parseReplicaIdFromServerColumn(Bytes.toBytes(column4))); String column5 = column4 + "2A"; - Assert.assertEquals(-1, + assertEquals(-1, MetaTableAccessor.parseReplicaIdFromServerColumn(Bytes.toBytes(column5))); String column6 = HConstants.STARTCODE_QUALIFIER_STR; - Assert.assertEquals(-1, + assertEquals(-1, MetaTableAccessor.parseReplicaIdFromServerColumn(Bytes.toBytes(column6))); } @Test public void testMetaReaderGetColumnMethods() { - Assert.assertArrayEquals(HConstants.SERVER_QUALIFIER, MetaTableAccessor.getServerColumn(0)); - Assert.assertArrayEquals(Bytes.toBytes(HConstants.SERVER_QUALIFIER_STR + assertArrayEquals(HConstants.SERVER_QUALIFIER, MetaTableAccessor.getServerColumn(0)); + assertArrayEquals(Bytes.toBytes(HConstants.SERVER_QUALIFIER_STR + MetaTableAccessor.META_REPLICA_ID_DELIMITER + "002A"), MetaTableAccessor.getServerColumn(42)); - Assert.assertArrayEquals(HConstants.STARTCODE_QUALIFIER, + assertArrayEquals(HConstants.STARTCODE_QUALIFIER, MetaTableAccessor.getStartCodeColumn(0)); - Assert.assertArrayEquals(Bytes.toBytes(HConstants.STARTCODE_QUALIFIER_STR + assertArrayEquals(Bytes.toBytes(HConstants.STARTCODE_QUALIFIER_STR + MetaTableAccessor.META_REPLICA_ID_DELIMITER + "002A"), MetaTableAccessor.getStartCodeColumn(42)); - Assert.assertArrayEquals(HConstants.SEQNUM_QUALIFIER, + assertArrayEquals(HConstants.SEQNUM_QUALIFIER, MetaTableAccessor.getSeqNumColumn(0)); - Assert.assertArrayEquals(Bytes.toBytes(HConstants.SEQNUM_QUALIFIER_STR + assertArrayEquals(Bytes.toBytes(HConstants.SEQNUM_QUALIFIER_STR + MetaTableAccessor.META_REPLICA_ID_DELIMITER + "002A"), MetaTableAccessor.getSeqNumColumn(42)); } @@ -457,14 +506,14 @@ public class TestMetaTableAccessor { long seqNum, int replicaId, boolean checkSeqNum) throws IOException { Get get = new Get(row); Result result = meta.get(get); - Assert.assertTrue(Bytes.equals( + assertTrue(Bytes.equals( result.getValue(HConstants.CATALOG_FAMILY, MetaTableAccessor.getServerColumn(replicaId)), Bytes.toBytes(serverName.getHostAndPort()))); - Assert.assertTrue(Bytes.equals( + assertTrue(Bytes.equals( result.getValue(HConstants.CATALOG_FAMILY, MetaTableAccessor.getStartCodeColumn(replicaId)), Bytes.toBytes(serverName.getStartcode()))); if (checkSeqNum) { - Assert.assertTrue(Bytes.equals( + assertTrue(Bytes.equals( result.getValue(HConstants.CATALOG_FAMILY, MetaTableAccessor.getSeqNumColumn(replicaId)), Bytes.toBytes(seqNum))); } @@ -478,10 +527,10 @@ public class TestMetaTableAccessor { MetaTableAccessor.getServerColumn(replicaId)); Cell startCodeCell = result.getColumnLatestCell(HConstants.CATALOG_FAMILY, MetaTableAccessor.getStartCodeColumn(replicaId)); - Assert.assertNotNull(serverCell); - Assert.assertNotNull(startCodeCell); - Assert.assertEquals(0, serverCell.getValueLength()); - Assert.assertEquals(0, startCodeCell.getValueLength()); + assertNotNull(serverCell); + assertNotNull(startCodeCell); + assertEquals(0, serverCell.getValueLength()); + assertEquals(0, startCodeCell.getValueLength()); } @Test @@ -509,12 +558,12 @@ public class TestMetaTableAccessor { Cell snCell = result.getColumnLatestCell(HConstants.CATALOG_FAMILY, MetaTableAccessor.getServerNameColumn(replicaId)); if (replicaId == 0) { - Assert.assertNotNull(stateCell); + assertNotNull(stateCell); } else { - Assert.assertNull(serverCell); - Assert.assertNull(startCodeCell); - Assert.assertNull(stateCell); - Assert.assertNull(snCell); + assertNull(serverCell); + assertNull(startCodeCell); + assertNull(stateCell); + assertNull(snCell); } } } finally { @@ -705,15 +754,15 @@ public class TestMetaTableAccessor { MetaTableAccessor.getStartCodeColumn(0)); Cell seqNumCell = result.getColumnLatestCell(HConstants.CATALOG_FAMILY, MetaTableAccessor.getSeqNumColumn(0)); - Assert.assertNotNull(serverCell); - Assert.assertNotNull(startCodeCell); - Assert.assertNotNull(seqNumCell); - Assert.assertTrue(serverCell.getValueLength() > 0); - Assert.assertTrue(startCodeCell.getValueLength() > 0); - Assert.assertTrue(seqNumCell.getValueLength() > 0); - Assert.assertEquals(masterSystemTime, serverCell.getTimestamp()); - Assert.assertEquals(masterSystemTime, startCodeCell.getTimestamp()); - Assert.assertEquals(masterSystemTime, seqNumCell.getTimestamp()); + assertNotNull(serverCell); + assertNotNull(startCodeCell); + assertNotNull(seqNumCell); + assertTrue(serverCell.getValueLength() > 0); + assertTrue(startCodeCell.getValueLength() > 0); + assertTrue(seqNumCell.getValueLength() > 0); + assertEquals(masterSystemTime, serverCell.getTimestamp()); + assertEquals(masterSystemTime, startCodeCell.getTimestamp()); + assertEquals(masterSystemTime, seqNumCell.getTimestamp()); } } @@ -763,8 +812,8 @@ public class TestMetaTableAccessor { Result result = meta.get(get); Cell serverCell = result.getColumnLatestCell(HConstants.CATALOG_FAMILY, MetaTableAccessor.getServerColumn(0)); - Assert.assertNotNull(serverCell); - Assert.assertEquals(serverNameTime, serverCell.getTimestamp()); + assertNotNull(serverCell); + assertEquals(serverNameTime, serverCell.getTimestamp()); ManualEnvironmentEdge edge = new ManualEnvironmentEdge(); edge.setValue(masterSystemTime); @@ -785,9 +834,9 @@ public class TestMetaTableAccessor { MetaTableAccessor.getStartCodeColumn(0)); Cell seqNumCell = result.getColumnLatestCell(HConstants.CATALOG_FAMILY, MetaTableAccessor.getSeqNumColumn(0)); - Assert.assertNull(serverCell); - Assert.assertNull(startCodeCell); - Assert.assertNull(seqNumCell); + assertNull(serverCell); + assertNull(startCodeCell); + assertNull(seqNumCell); } } @@ -868,7 +917,7 @@ public class TestMetaTableAccessor { MetaTableAccessor.splitRegion(connection, parent, -1L, splitA, splitB, loc.getServerName(), 1); - Assert.assertTrue(prevCalls < scheduler.numPriorityCalls); + assertTrue(prevCalls < scheduler.numPriorityCalls); } } @@ -911,8 +960,8 @@ public class TestMetaTableAccessor { MetaTableAccessor.getServerColumn(splitA.getReplicaId())); Cell startCodeCellA = resultA.getColumnLatestCell(HConstants.CATALOG_FAMILY, MetaTableAccessor.getStartCodeColumn(splitA.getReplicaId())); - Assert.assertNull(serverCellA); - Assert.assertNull(startCodeCellA); + assertNull(serverCellA); + assertNull(startCodeCellA); Get get2 = new Get(splitA.getRegionName()); Result resultB = meta.get(get2); @@ -920,8 +969,8 @@ public class TestMetaTableAccessor { MetaTableAccessor.getServerColumn(splitB.getReplicaId())); Cell startCodeCellB = resultB.getColumnLatestCell(HConstants.CATALOG_FAMILY, MetaTableAccessor.getStartCodeColumn(splitB.getReplicaId())); - Assert.assertNull(serverCellB); - Assert.assertNull(startCodeCellB); + assertNull(serverCellB); + assertNull(startCodeCellB); } finally { if (meta != null) { meta.close(); @@ -937,10 +986,10 @@ public class TestMetaTableAccessor { final String encodedName = regions.get(0).getRegionInfo().getEncodedName(); final Result result = MetaTableAccessor.scanByRegionEncodedName(UTIL.getConnection(), encodedName); - Assert.assertNotNull(result); - Assert.assertTrue(result.advance()); + assertNotNull(result); + assertTrue(result.advance()); final String resultingRowKey = CellUtil.getCellKeyAsString(result.current()); - Assert.assertTrue(resultingRowKey.contains(encodedName)); + assertTrue(resultingRowKey.contains(encodedName)); UTIL.deleteTable(tableName); } @@ -949,7 +998,7 @@ public class TestMetaTableAccessor { final String encodedName = "nonexistingregion"; final Result result = MetaTableAccessor.scanByRegionEncodedName(UTIL.getConnection(), encodedName); - Assert.assertNull(result); + assertNull(result); } }