From a0c64f0905cdc6ff3760db21713479ef0c2cd2bb Mon Sep 17 00:00:00 2001 From: Josh Elser Date: Fri, 24 Mar 2017 14:01:12 -0400 Subject: [PATCH 2/2] HBASE-17833 Stabilize some flakey tests In general, try to make the tests more deterministic by ensuring we observe stable values as we know that those values are mutable. This should help avoid problems where size reports are delayed and we see an incomplete value. --- .../hbase/quotas/SpaceQuotaHelperForTests.java | 8 +- .../hbase/quotas/TestSpaceQuotasWithSnapshots.java | 140 ++++++++++++++++----- 2 files changed, 112 insertions(+), 36 deletions(-) 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 b0d3e30e04..90b0e37984 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 @@ -312,7 +312,11 @@ public class SpaceQuotaHelperForTests { this(Objects.requireNonNull(conn), null, Objects.requireNonNull(ns)); } - private SpaceQuotaSnapshotPredicate(Connection conn, TableName tn, String ns) { + SpaceQuotaSnapshotPredicate(Connection conn, TableName tn, String ns) { + if ((null != tn && null != ns) || (null == tn && null == ns)) { + throw new IllegalArgumentException( + "One of TableName and Namespace must be non-null, and the other null"); + } this.conn = conn; this.tn = tn; this.ns = ns; @@ -327,7 +331,7 @@ public class SpaceQuotaHelperForTests { snapshot = QuotaTableUtil.getCurrentSnapshot(conn, ns); } - LOG.debug("Saw quota snapshot for " + tn + ": " + snapshot); + LOG.debug("Saw quota snapshot for " + (null == tn ? ns : tn) + ": " + snapshot); if (null == snapshot) { return false; } 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 28c763f327..9278fa92ad 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 @@ -16,12 +16,14 @@ */ package org.apache.hadoop.hbase.quotas; +import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertNotNull; import static org.junit.Assert.assertTrue; +import java.io.IOException; import java.util.Map; +import java.util.concurrent.atomic.AtomicInteger; import java.util.concurrent.atomic.AtomicLong; -import java.util.concurrent.atomic.AtomicReference; import org.apache.commons.logging.Log; import org.apache.commons.logging.LogFactory; @@ -61,6 +63,7 @@ public class TestSpaceQuotasWithSnapshots { private static final HBaseTestingUtility TEST_UTIL = new HBaseTestingUtility(); // Global for all tests in the class private static final AtomicLong COUNTER = new AtomicLong(0); + private static final long FUDGE_FOR_TABLE_SIZE = 500L * SpaceQuotaHelperForTests.ONE_KILOBYTE; @Rule public TestName testName = new TestName(); @@ -89,7 +92,6 @@ public class TestSpaceQuotasWithSnapshots { @Test public void testTablesInheritSnapshotSize() throws Exception { - final long fudgeAmount = 500L * SpaceQuotaHelperForTests.ONE_KILOBYTE; TableName tn = helper.createTableWithRegions(1); LOG.info("Writing data"); // Set a quota @@ -108,9 +110,12 @@ public class TestSpaceQuotasWithSnapshots { } }); + // 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 " + initialSize); + LOG.info("Initial table size was " + actualInitialSize); LOG.info("Snapshot the table"); final String snapshot1 = tn.toString() + "_snapshot1"; @@ -125,45 +130,42 @@ public class TestSpaceQuotasWithSnapshots { LOG.info("Synchronously compacting the table"); TEST_UTIL.compact(tn, true); - final long upperBound = initialSize + fudgeAmount; - final long lowerBound = initialSize - fudgeAmount; + final long upperBound = initialSize + FUDGE_FOR_TABLE_SIZE; + final long lowerBound = initialSize - FUDGE_FOR_TABLE_SIZE; // Store the actual size after writing more data and then compacting it down to one file - final AtomicReference lastSeenSize = new AtomicReference<>(); - LOG.info("Waiting for the region reports to reflect the correct size, between (" + lowerBound + ", " + upperBound + ")"); TEST_UTIL.waitFor(30 * 1000, 500, new Predicate() { @Override public boolean evaluate() throws Exception { - Map sizes = QuotaTableUtil.getMasterReportedTableSizes(conn); - LOG.debug("Master observed table sizes from region size reports: " + sizes); - Long size = sizes.get(tn); - if (null == size) { - return false; - } - lastSeenSize.set(size); + long size = getRegionSizeReportForTable(conn, tn); return size < upperBound && size > lowerBound; } }); - assertNotNull("Did not expect to see a null size", lastSeenSize.get()); - LOG.info("Last seen size: " + lastSeenSize.get()); + // Make sure we see the "final" new size for the table, not some intermediate + waitForStableRegionSizeReport(conn, tn); + final long finalSize = getRegionSizeReportForTable(conn, tn); + assertNotNull("Did not expect to see a null size", finalSize); + LOG.info("Last seen size: " + finalSize); // Make sure the QuotaObserverChore has time to reflect the new region size reports // (we saw above). The usage of the table should *not* decrease when we check it below, // though, because the snapshot on our table will cause the table to "retain" the size. - TEST_UTIL.waitFor(10 * 1000, 500, new SpaceQuotaSnapshotPredicate(conn, tn) { + TEST_UTIL.waitFor(20 * 1000, 500, new SpaceQuotaSnapshotPredicate(conn, tn) { @Override public boolean evaluate(SpaceQuotaSnapshot snapshot) throws Exception { - return lastSeenSize.get() >= snapshot.getUsage(); + return snapshot.getUsage() >= finalSize; } }); // The final usage should be the sum of the initial size (referenced by the snapshot) and the // new size we just wrote above. - long expectedFinalSize = actualInitialSize + lastSeenSize.get(); - LOG.info("Expecting table usage to be " + expectedFinalSize); + long expectedFinalSize = actualInitialSize + finalSize; + LOG.info( + "Expecting table usage to be " + actualInitialSize + " + " + finalSize + + " = " + expectedFinalSize); // The size of the table (WRT quotas) should now be approximately double what it was previously TEST_UTIL.waitFor(30 * 1000, 1000, new SpaceQuotaSnapshotPredicate(conn, tn) { @Override boolean evaluate(SpaceQuotaSnapshot snapshot) throws Exception { @@ -171,11 +173,15 @@ public class TestSpaceQuotasWithSnapshots { return expectedFinalSize == snapshot.getUsage(); } }); + + Map snapshotSizes = QuotaTableUtil.getObservedSnapshotSizes(conn); + Long size = snapshotSizes.get(snapshot1); + assertNotNull("Did not observe the size of the snapshot", size); + assertEquals(actualInitialSize, size.longValue()); } @Test public void testNamespacesInheritSnapshotSize() throws Exception { - final long fudgeAmount = 500L * SpaceQuotaHelperForTests.ONE_KILOBYTE; String ns = helper.createNamespace().getName(); TableName tn = helper.createTableWithRegions(ns, 1); LOG.info("Writing data"); @@ -197,9 +203,12 @@ public class TestSpaceQuotasWithSnapshots { } }); + // Make sure we see the "final" new size for the table, not some intermediate + waitForStableQuotaSize(conn, null, ns); + // The actual size on disk after we wrote our data the first time final long actualInitialSize = QuotaTableUtil.getCurrentSnapshot(conn, ns).getUsage(); - LOG.info("Initial table size was " + initialSize); + LOG.info("Initial table size was " + actualInitialSize); LOG.info("Snapshot the table"); final String snapshot1 = tn.getQualifierAsString() + "_snapshot1"; @@ -214,11 +223,8 @@ public class TestSpaceQuotasWithSnapshots { LOG.info("Synchronously compacting the table"); TEST_UTIL.compact(tn, true); - final long upperBound = initialSize + fudgeAmount; - final long lowerBound = initialSize - fudgeAmount; - - // Store the actual size after writing more data and then compacting it down to one file - final AtomicReference lastSeenSize = new AtomicReference<>(); + final long upperBound = initialSize + FUDGE_FOR_TABLE_SIZE; + final long lowerBound = initialSize - FUDGE_FOR_TABLE_SIZE; LOG.info("Waiting for the region reports to reflect the correct size, between (" + lowerBound + ", " + upperBound + ")"); @@ -231,34 +237,44 @@ public class TestSpaceQuotasWithSnapshots { if (null == size) { return false; } - lastSeenSize.set(size); return size < upperBound && size > lowerBound; } }); - assertNotNull("Did not expect to see a null size", lastSeenSize.get()); - LOG.info("Last seen size: " + lastSeenSize.get()); + // Make sure we see the "final" new size for the table, not some intermediate + waitForStableRegionSizeReport(conn, tn); + final long finalSize = getRegionSizeReportForTable(conn, tn); + assertNotNull("Did not expect to see a null size", finalSize); + LOG.info("Final observed size of table: " + finalSize); // Make sure the QuotaObserverChore has time to reflect the new region size reports // (we saw above). The usage of the table should *not* decrease when we check it below, // though, because the snapshot on our table will cause the table to "retain" the size. - TEST_UTIL.waitFor(10 * 1000, 500, new SpaceQuotaSnapshotPredicate(conn, ns) { + TEST_UTIL.waitFor(20 * 1000, 500, new SpaceQuotaSnapshotPredicate(conn, ns) { @Override public boolean evaluate(SpaceQuotaSnapshot snapshot) throws Exception { - return lastSeenSize.get() >= snapshot.getUsage(); + return snapshot.getUsage() >= finalSize; } }); // The final usage should be the sum of the initial size (referenced by the snapshot) and the // new size we just wrote above. - long expectedFinalSize = actualInitialSize + lastSeenSize.get(); - LOG.info("Expecting namespace usage to be " + expectedFinalSize); + long expectedFinalSize = actualInitialSize + finalSize; + LOG.info( + "Expecting namespace usage to be " + actualInitialSize + " + " + finalSize + + " = " + expectedFinalSize); // The size of the table (WRT quotas) should now be approximately double what it was previously TEST_UTIL.waitFor(30 * 1000, 1000, new SpaceQuotaSnapshotPredicate(conn, ns) { @Override boolean evaluate(SpaceQuotaSnapshot snapshot) throws Exception { + LOG.debug("Checking for " + expectedFinalSize + " == " + snapshot.getUsage()); return expectedFinalSize == snapshot.getUsage(); } }); + + Map snapshotSizes = QuotaTableUtil.getObservedSnapshotSizes(conn); + Long size = snapshotSizes.get(snapshot1); + assertNotNull("Did not observe the size of the snapshot", size); + assertEquals(actualInitialSize, size.longValue()); } @Test @@ -317,4 +333,60 @@ public class TestSpaceQuotasWithSnapshots { } }); } + + 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 + AtomicLong lastValue = new AtomicLong(-1); + AtomicInteger counter = new AtomicInteger(0); + TEST_UTIL.waitFor(15_000, 500, new SpaceQuotaSnapshotPredicate(conn, tn, ns) { + @Override boolean evaluate(SpaceQuotaSnapshot snapshot) throws Exception { + LOG.debug("Last observed size=" + lastValue.get()); + if (snapshot.getUsage() == lastValue.get()) { + int numMatches = counter.incrementAndGet(); + if (numMatches >= 5) { + return true; + } + // Not yet.. + return false; + } + counter.set(0); + lastValue.set(snapshot.getUsage()); + return false; + } + }); + } + + long getRegionSizeReportForTable(Connection conn, TableName tn) throws IOException { + Map sizes = QuotaTableUtil.getMasterReportedTableSizes(conn); + Long value = sizes.get(tn); + if (null == value) { + return 0L; + } + return value.longValue(); + } + + void waitForStableRegionSizeReport(Connection conn, TableName tn) throws Exception { + // For some stability in the value before proceeding + // Helps make sure that we got the actual last value, not some inbetween + AtomicLong lastValue = new AtomicLong(-1); + AtomicInteger counter = new AtomicInteger(0); + TEST_UTIL.waitFor(15_000, 500, new Predicate() { + @Override public boolean evaluate() throws Exception { + LOG.debug("Last observed size=" + lastValue.get()); + long actual = getRegionSizeReportForTable(conn, tn); + if (actual == lastValue.get()) { + int numMatches = counter.incrementAndGet(); + if (numMatches >= 5) { + return true; + } + // Not yet.. + return false; + } + counter.set(0); + lastValue.set(actual); + return false; + } + }); + } } -- 2.12.0