From 638a908621151d427872f7b2524648ef03b65a0d Mon Sep 17 00:00:00 2001 From: Josh Elser Date: Mon, 27 Mar 2017 14:58:25 -0400 Subject: [PATCH] HBASE-17750 Only include HFiles when computing a Region's size Introduces a new method to Store that returns the size of only the StoreFiles which are HFiles (not HFileLinks). From here, it is trivial to update FileSystemUtilizationChore to perform the expected computation. --- .../hbase/quotas/FileSystemUtilizationChore.java | 9 ++- .../apache/hadoop/hbase/regionserver/HStore.java | 16 +++++- .../apache/hadoop/hbase/regionserver/Store.java | 5 ++ .../hbase/quotas/SpaceQuotaHelperForTests.java | 11 +++- .../quotas/TestFileSystemUtilizationChore.java | 54 +++++++++++++++++- .../hbase/quotas/TestSpaceQuotasWithSnapshots.java | 66 ++++++++++++++++++++++ 6 files changed, 153 insertions(+), 8 deletions(-) diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/quotas/FileSystemUtilizationChore.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/quotas/FileSystemUtilizationChore.java index efc17ff05f..67af854083 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/quotas/FileSystemUtilizationChore.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/quotas/FileSystemUtilizationChore.java @@ -33,6 +33,8 @@ import org.apache.hadoop.hbase.regionserver.HRegion; import org.apache.hadoop.hbase.regionserver.HRegionServer; import org.apache.hadoop.hbase.regionserver.Region; import org.apache.hadoop.hbase.regionserver.Store; +import org.apache.hadoop.hbase.regionserver.StoreFile; +import org.apache.hadoop.hbase.regionserver.StoreFileReader; import org.apache.hadoop.hbase.util.EnvironmentEdgeManager; /** @@ -168,9 +170,10 @@ public class FileSystemUtilizationChore extends ScheduledChore { long computeSize(Region r) { long regionSize = 0L; for (Store store : r.getStores()) { - // StoreFile/StoreFileReaders are already instantiated with the file length cached. - // Can avoid extra NN ops. - regionSize += store.getStorefilesSize(); + regionSize += store.getHFilesSize(); + } + if (LOG.isTraceEnabled()) { + LOG.trace("Size of " + r + " is " + regionSize); } return regionSize; } diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HStore.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HStore.java index a988c5b9f3..37fd7b8410 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HStore.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HStore.java @@ -47,6 +47,7 @@ import java.util.concurrent.ThreadPoolExecutor; import java.util.concurrent.atomic.AtomicBoolean; import java.util.concurrent.locks.ReentrantLock; import java.util.concurrent.locks.ReentrantReadWriteLock; +import java.util.function.Predicate; import org.apache.commons.logging.Log; import org.apache.commons.logging.LogFactory; @@ -2011,6 +2012,17 @@ public class HStore implements Store { @Override public long getStorefilesSize() { + // Include all StoreFiles + return getStorefilesSize(storeFile -> true); + } + + @Override + public long getHFilesSize() { + // Include only StoreFiles which are HFiles + return getStorefilesSize(storeFile -> storeFile.isHFile()); + } + + private long getStorefilesSize(Predicate predicate) { long size = 0; for (StoreFile s: this.storeEngine.getStoreFileManager().getStorefiles()) { StoreFileReader r = s.getReader(); @@ -2018,7 +2030,9 @@ public class HStore implements Store { LOG.warn("StoreFile " + s + " has a null Reader"); continue; } - size += r.length(); + if (predicate.test(s)) { + size += r.length(); + } } return size; } diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/Store.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/Store.java index 76595f3622..f5e90eb85c 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/Store.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/Store.java @@ -402,6 +402,11 @@ public interface Store extends HeapSize, StoreConfigInformation, PropagatingConf long getStorefilesSize(); /** + * @return The size of only the store files which are HFiles, in bytes. + */ + long getHFilesSize(); + + /** * @return The size of the store file indexes, in bytes. */ long getStorefilesIndexSize(); diff --git a/hbase-server/src/test/java/org/apache/hadoop/hbase/quotas/SpaceQuotaHelperForTests.java b/hbase-server/src/test/java/org/apache/hadoop/hbase/quotas/SpaceQuotaHelperForTests.java index 90b0e37984..88fbd98286 100644 --- a/hbase-server/src/test/java/org/apache/hadoop/hbase/quotas/SpaceQuotaHelperForTests.java +++ b/hbase-server/src/test/java/org/apache/hadoop/hbase/quotas/SpaceQuotaHelperForTests.java @@ -233,10 +233,17 @@ public class SpaceQuotaHelperForTests { return createTableWithRegions(NamespaceDescriptor.DEFAULT_NAMESPACE_NAME_STR, numRegions); } + TableName getNextTableName() { + return getNextTableName(NamespaceDescriptor.DEFAULT_NAMESPACE_NAME_STR); + } + + TableName getNextTableName(String namespace) { + return TableName.valueOf(namespace, testName.getMethodName() + counter.getAndIncrement()); + } + TableName createTableWithRegions(String namespace, int numRegions) throws Exception { final Admin admin = testUtil.getAdmin(); - final TableName tn = TableName.valueOf( - namespace, testName.getMethodName() + counter.getAndIncrement()); + final TableName tn = getNextTableName(namespace); // Delete the old table if (admin.tableExists(tn)) { diff --git a/hbase-server/src/test/java/org/apache/hadoop/hbase/quotas/TestFileSystemUtilizationChore.java b/hbase-server/src/test/java/org/apache/hadoop/hbase/quotas/TestFileSystemUtilizationChore.java index 18e47af7d1..823b1f7b89 100644 --- a/hbase-server/src/test/java/org/apache/hadoop/hbase/quotas/TestFileSystemUtilizationChore.java +++ b/hbase-server/src/test/java/org/apache/hadoop/hbase/quotas/TestFileSystemUtilizationChore.java @@ -38,6 +38,7 @@ import org.apache.hadoop.hbase.HRegionInfo; import org.apache.hadoop.hbase.regionserver.HRegionServer; import org.apache.hadoop.hbase.regionserver.Region; import org.apache.hadoop.hbase.regionserver.Store; +import org.apache.hadoop.hbase.regionserver.StoreFile; import org.apache.hadoop.hbase.testclassification.SmallTests; import org.junit.Test; import org.junit.experimental.categories.Category; @@ -238,13 +239,13 @@ public class TestFileSystemUtilizationChore { final Configuration conf = getDefaultHBaseConfiguration(); final HRegionServer rs = mockRegionServer(conf); - // Three regions with multiple store sizes + // Two regions with multiple store sizes final List r1Sizes = Arrays.asList(1024L, 2048L); final long r1Sum = sum(r1Sizes); final List r2Sizes = Arrays.asList(1024L * 1024L); final FileSystemUtilizationChore chore = new FileSystemUtilizationChore(rs); - doAnswer(new ExpectedRegionSizeSummationAnswer(sum(Arrays.asList(r1Sum)))) + doAnswer(new ExpectedRegionSizeSummationAnswer(r1Sum)) .when(rs) .reportRegionSizesForQuotas((Map) any(Map.class)); @@ -254,6 +255,33 @@ public class TestFileSystemUtilizationChore { chore.chore(); } + @SuppressWarnings("unchecked") + @Test + public void testNonHFilesAreIgnored() { + final Configuration conf = getDefaultHBaseConfiguration(); + final HRegionServer rs = mockRegionServer(conf); + + // Region r1 has two store files, one hfile link and one hfile + final List r1StoreFileSizes = Arrays.asList(1024L, 2048L); + final List r1HFileSizes = Arrays.asList(0L, 2048L); + final long r1HFileSizeSum = sum(r1HFileSizes); + // Region r2 has one store file which is a hfile link + final List r2StoreFileSizes = Arrays.asList(1024L * 1024L); + final List r2HFileSizes = Arrays.asList(0L); + final long r2HFileSizeSum = sum(r2HFileSizes); + + // We expect that only the hfiles would be counted (hfile links are ignored) + final FileSystemUtilizationChore chore = new FileSystemUtilizationChore(rs); + doAnswer(new ExpectedRegionSizeSummationAnswer( + sum(Arrays.asList(r1HFileSizeSum, r2HFileSizeSum)))) + .when(rs).reportRegionSizesForQuotas((Map) any(Map.class)); + + final Region r1 = mockRegionWithHFileLinks(r1StoreFileSizes, r1HFileSizes); + final Region r2 = mockRegionWithHFileLinks(r2StoreFileSizes, r2HFileSizes); + when(rs.getOnlineRegions()).thenReturn(Arrays.asList(r1, r2)); + chore.chore(); + } + /** * Creates an HBase Configuration object for the default values. */ @@ -300,7 +328,29 @@ public class TestFileSystemUtilizationChore { for (Long storeSize : storeSizes) { final Store s = mock(Store.class); stores.add(s); + when(s.getHFilesSize()).thenReturn(storeSize); + } + return r; + } + + private Region mockRegionWithHFileLinks(Collection storeSizes, Collection hfileSizes) { + final Region r = mock(Region.class); + final HRegionInfo info = mock(HRegionInfo.class); + when(r.getRegionInfo()).thenReturn(info); + List stores = new ArrayList<>(); + when(r.getStores()).thenReturn(stores); + assertEquals( + "Logic error, storeSizes and linkSizes must be equal in size", storeSizes.size(), + hfileSizes.size()); + Iterator storeSizeIter = storeSizes.iterator(); + Iterator hfileSizeIter = hfileSizes.iterator(); + while (storeSizeIter.hasNext() && hfileSizeIter.hasNext()) { + final long storeSize = storeSizeIter.next(); + final long hfileSize = hfileSizeIter.next(); + final Store s = mock(Store.class); + stores.add(s); when(s.getStorefilesSize()).thenReturn(storeSize); + when(s.getHFilesSize()).thenReturn(hfileSize); } return r; } diff --git a/hbase-server/src/test/java/org/apache/hadoop/hbase/quotas/TestSpaceQuotasWithSnapshots.java b/hbase-server/src/test/java/org/apache/hadoop/hbase/quotas/TestSpaceQuotasWithSnapshots.java index 9278fa92ad..4c7d8bebd3 100644 --- a/hbase-server/src/test/java/org/apache/hadoop/hbase/quotas/TestSpaceQuotasWithSnapshots.java +++ b/hbase-server/src/test/java/org/apache/hadoop/hbase/quotas/TestSpaceQuotasWithSnapshots.java @@ -334,6 +334,72 @@ public class TestSpaceQuotasWithSnapshots { }); } + @Test + public void testRematerializedTablesDoNoInheritSpace() throws Exception { + TableName tn = helper.createTableWithRegions(1); + TableName tn2 = helper.getNextTableName(); + LOG.info("Writing data"); + // Set a quota on both tables + QuotaSettings settings = QuotaSettingsFactory.limitTableSpace( + tn, SpaceQuotaHelperForTests.ONE_GIGABYTE, SpaceViolationPolicy.NO_INSERTS); + admin.setQuota(settings); + QuotaSettings settings2 = QuotaSettingsFactory.limitTableSpace( + tn2, SpaceQuotaHelperForTests.ONE_GIGABYTE, SpaceViolationPolicy.NO_INSERTS); + admin.setQuota(settings2); + // Write some data + final long initialSize = 2L * SpaceQuotaHelperForTests.ONE_MEGABYTE; + helper.writeData(tn, initialSize); + + LOG.info("Waiting until table size reflects written data"); + // Wait until that data is seen by the master + TEST_UTIL.waitFor(30 * 1000, 500, new SpaceQuotaSnapshotPredicate(conn, tn) { + @Override boolean evaluate(SpaceQuotaSnapshot snapshot) throws Exception { + return snapshot.getUsage() >= initialSize; + } + }); + + // Make sure we see the final quota usage size + waitForStableQuotaSize(conn, tn, null); + + // The actual size on disk after we wrote our data the first time + final long actualInitialSize = QuotaTableUtil.getCurrentSnapshot(conn, tn).getUsage(); + LOG.info("Initial table size was " + actualInitialSize); + + LOG.info("Snapshot the table"); + final String snapshot1 = tn.toString() + "_snapshot1"; + admin.snapshot(snapshot1, tn); + + admin.cloneSnapshot(snapshot1, tn2); + + // Write some more data to the first table + helper.writeData(tn, initialSize, "q2"); + admin.flush(tn); + + // Watch the usage of the first table with some more data to know when the new + // region size reports were sent to the master + TEST_UTIL.waitFor(30_000, 1_000, new SpaceQuotaSnapshotPredicate(conn, tn) { + @Override + boolean evaluate(SpaceQuotaSnapshot snapshot) throws Exception { + return snapshot.getUsage() >= actualInitialSize * 2; + } + }); + + // We know that reports were sent by our RS, verify that they take up zero size. + SpaceQuotaSnapshot snapshot = QuotaTableUtil.getCurrentSnapshot(conn, tn2); + assertNotNull(snapshot); + assertEquals(0, snapshot.getUsage()); + + // Compact the cloned table to force it to own its own files. + TEST_UTIL.compact(tn2, true); + // After the table is compacted, it should have its own files and be the same size as originally + TEST_UTIL.waitFor(30_000, 1_000, new SpaceQuotaSnapshotPredicate(conn, tn2) { + @Override + boolean evaluate(SpaceQuotaSnapshot snapshot) throws Exception { + return snapshot.getUsage() == actualInitialSize; + } + }); + } + void waitForStableQuotaSize(Connection conn, TableName tn, String ns) throws Exception { // For some stability in the value before proceeding // Helps make sure that we got the actual last value, not some inbetween -- 2.12.2