From 801a2a5d034a5f95a63b2dae39feb1028ccb63bc Mon Sep 17 00:00:00 2001 From: stack Date: Sun, 3 Jul 2016 21:21:57 -0700 Subject: [PATCH] HBASE-16074 ITBLL fails, reports lost big or tine families 1. Change HFile Writer constructor so we pass in the TimeRangeTracker, if one, on construction rather than set later (the flag and reference were not volatile so could have made for issues in concurrent case) 2. Make sure the construction of a TimeRange from a TimeRangeTracer on open of an HFile Reader never makes a bad minimum value, one that would preclude us reading any values from a file (add a log and set min to 0) M hbase-common/src/main/java/org/apache/hadoop/hbase/io/TimeRange.java Call through to next constructor (if minStamp was 0, we'd skip setting allTime=true) M hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HStore.java Add constructor override that takes a TimeRangeTracker (set when flushing but not when compacting) M hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/Store.java Add override creating an HFile in tmp that takes a TimeRangeTracker M hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/StoreFile.java Add override for HFile Writer that takes a TimeRangeTracker Take it on construction instead of having it passed by a setter later (flags and reference set by the setter were not volatile... could have been prob in concurrent case) M hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/TimeRangeTracker.java Log WARN if bad initial TimeRange value (and then 'fix' it) M hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestTimeRangeTracker.java A few tests to prove serialization works as expected and that we'll get a bad min if not constructed properly. --- .../java/org/apache/hadoop/hbase/io/TimeRange.java | 5 +- .../hbase/regionserver/DefaultStoreFlusher.java | 4 +- .../apache/hadoop/hbase/regionserver/HStore.java | 27 ++++++++-- .../apache/hadoop/hbase/regionserver/Store.java | 20 +++++++- .../hadoop/hbase/regionserver/StoreFile.java | 58 ++++++++++++++++------ .../hbase/regionserver/StripeStoreFlusher.java | 4 +- .../hbase/regionserver/TimeRangeTracker.java | 9 +++- .../hbase/regionserver/TestTimeRangeTracker.java | 30 +++++++++++ 8 files changed, 127 insertions(+), 30 deletions(-) diff --git a/hbase-common/src/main/java/org/apache/hadoop/hbase/io/TimeRange.java b/hbase-common/src/main/java/org/apache/hadoop/hbase/io/TimeRange.java index 2b70644..c9eae04 100644 --- a/hbase-common/src/main/java/org/apache/hadoop/hbase/io/TimeRange.java +++ b/hbase-common/src/main/java/org/apache/hadoop/hbase/io/TimeRange.java @@ -36,7 +36,7 @@ import org.apache.hadoop.hbase.util.Bytes; @InterfaceAudience.Public @InterfaceStability.Stable public class TimeRange { - static final long INITIAL_MIN_TIMESTAMP = 0L; + public static final long INITIAL_MIN_TIMESTAMP = 0L; private static final long MIN_TIME = INITIAL_MIN_TIMESTAMP; static final long INITIAL_MAX_TIMESTAMP = Long.MAX_VALUE; static final long MAX_TIME = INITIAL_MAX_TIMESTAMP; @@ -72,8 +72,7 @@ public class TimeRange { */ @Deprecated public TimeRange(byte [] minStamp) { - this.minStamp = Bytes.toLong(minStamp); - this.allTime = false; + this(Bytes.toLong(minStamp)); } /** diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/DefaultStoreFlusher.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/DefaultStoreFlusher.java index 935813c..90c16f9 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/DefaultStoreFlusher.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/DefaultStoreFlusher.java @@ -68,8 +68,8 @@ public class DefaultStoreFlusher extends StoreFlusher { /* isCompaction = */ false, /* includeMVCCReadpoint = */ true, /* includesTags = */ snapshot.isTagsPresent(), - /* shouldDropBehind = */ false); - writer.setTimeRangeTracker(snapshot.getTimeRangeTracker()); + /* shouldDropBehind = */ false, + snapshot.getTimeRangeTracker()); IOException e = null; try { performFlush(scanner, writer, smallestReadPoint, throughputController); 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 db66641..66eaebc 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 @@ -999,6 +999,23 @@ public class HStore implements Store { boolean isCompaction, boolean includeMVCCReadpoint, boolean includesTag, boolean shouldDropBehind) throws IOException { + return createWriterInTmp(maxKeyCount, compression, isCompaction, includeMVCCReadpoint, + includesTag, shouldDropBehind, null); + } + + /* + * @param maxKeyCount + * @param compression Compression algorithm to use + * @param isCompaction whether we are creating a new file in a compaction + * @param includesMVCCReadPoint - whether to include MVCC or not + * @param includesTag - includesTag or not + * @return Writer for a new StoreFile in the tmp dir. + */ + @Override + public StoreFile.Writer createWriterInTmp(long maxKeyCount, Compression.Algorithm compression, + boolean isCompaction, boolean includeMVCCReadpoint, boolean includesTag, + boolean shouldDropBehind, final TimeRangeTracker trt) + throws IOException { final CacheConfig writerCacheConf; if (isCompaction) { // Don't cache data on write on compactions. @@ -1014,7 +1031,7 @@ public class HStore implements Store { } HFileContext hFileContext = createFileContext(compression, includeMVCCReadpoint, includesTag, cryptoContext); - StoreFile.Writer w = new StoreFile.WriterBuilder(conf, writerCacheConf, + StoreFile.WriterBuilder builder = new StoreFile.WriterBuilder(conf, writerCacheConf, this.getFileSystem()) .withFilePath(fs.createTempName()) .withComparator(comparator) @@ -1022,9 +1039,11 @@ public class HStore implements Store { .withMaxKeyCount(maxKeyCount) .withFavoredNodes(favoredNodes) .withFileContext(hFileContext) - .withShouldDropCacheBehind(shouldDropBehind) - .build(); - return w; + .withShouldDropCacheBehind(shouldDropBehind); + if (trt != null) { + builder.withTimeRangeTracker(trt); + } + return builder.build(); } private HFileContext createFileContext(Compression.Algorithm compression, 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 dec27ad..e7a4de5 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 @@ -211,8 +211,24 @@ public interface Store extends HeapSize, StoreConfigInformation, PropagatingConf boolean shouldDropBehind ) throws IOException; - - + /** + * @param maxKeyCount + * @param compression Compression algorithm to use + * @param isCompaction whether we are creating a new file in a compaction + * @param includeMVCCReadpoint whether we should out the MVCC readpoint + * @param shouldDropBehind should the writer drop caches behind writes + * @param trt Ready-made timetracker to use. + * @return Writer for a new StoreFile in the tmp dir. + */ + StoreFile.Writer createWriterInTmp( + long maxKeyCount, + Compression.Algorithm compression, + boolean isCompaction, + boolean includeMVCCReadpoint, + boolean includesTags, + boolean shouldDropBehind, + final TimeRangeTracker trt + ) throws IOException; // Compaction oriented methods diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/StoreFile.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/StoreFile.java index 845a8d2..1abc17a 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/StoreFile.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/StoreFile.java @@ -617,6 +617,7 @@ public class StoreFile { private Path filePath; private InetSocketAddress[] favoredNodes; private HFileContext fileContext; + private TimeRangeTracker trt; public WriterBuilder(Configuration conf, CacheConfig cacheConf, FileSystem fs) { @@ -626,6 +627,17 @@ public class StoreFile { } /** + * @param trt A premade TimeRangeTracker to use rather than build one per append (building one + * of these is expensive so good to pass one in if you have one). + * @return this (for chained invocation) + */ + public WriterBuilder withTimeRangeTracker(final TimeRangeTracker trt) { + Preconditions.checkNotNull(trt); + this.trt = trt; + return this; + } + + /** * Use either this method or {@link #withFilePath}, but not both. * @param dir Path to column family directory. The directory is created if * does not exist. The file is given a unique name within this @@ -718,7 +730,7 @@ public class StoreFile { comparator = KeyValue.COMPARATOR; } return new Writer(fs, filePath, - conf, cacheConf, comparator, bloomType, maxKeyCount, favoredNodes, fileContext); + conf, cacheConf, comparator, bloomType, maxKeyCount, favoredNodes, fileContext, trt); } } @@ -794,7 +806,7 @@ public class StoreFile { private Cell lastDeleteFamilyCell = null; private long deleteFamilyCnt = 0; - TimeRangeTracker timeRangeTracker = new TimeRangeTracker(); + final TimeRangeTracker timeRangeTracker; /** * timeRangeTrackerSet is used to figure if we were passed a filled-out TimeRangeTracker or not. * When flushing a memstore, we set the TimeRangeTracker that it accumulated during updates to @@ -802,7 +814,7 @@ public class StoreFile { * recalculate the timeRangeTracker bounds; it was done already as part of add-to-memstore. * A completed TimeRangeTracker is not set in cases of compactions when it is recalculated. */ - boolean timeRangeTrackerSet = false; + private final boolean timeRangeTrackerSet; protected HFile.Writer writer; @@ -825,6 +837,33 @@ public class StoreFile { final KVComparator comparator, BloomType bloomType, long maxKeys, InetSocketAddress[] favoredNodes, HFileContext fileContext) throws IOException { + this(fs, path, conf, cacheConf, comparator, bloomType, maxKeys, favoredNodes, fileContext, + null); + } + + /** + * Creates an HFile.Writer that also write helpful meta data. + * @param fs file system to write to + * @param path file name to create + * @param conf user configuration + * @param comparator key comparator + * @param bloomType bloom filter setting + * @param maxKeys the expected maximum number of keys to be added. Was used + * for Bloom filter size in {@link HFile} format version 1. + * @param favoredNodes + * @param fileContext - The HFile context + * @param trt Ready-made timetracker to use. + * @throws IOException problem writing to FS + */ + private Writer(FileSystem fs, Path path, + final Configuration conf, + CacheConfig cacheConf, + final KVComparator comparator, BloomType bloomType, long maxKeys, + InetSocketAddress[] favoredNodes, HFileContext fileContext, + final TimeRangeTracker trt) + throws IOException { + this.timeRangeTrackerSet = trt != null; + this.timeRangeTracker = this.timeRangeTrackerSet? trt: new TimeRangeTracker(); writer = HFile.getWriterFactory(conf, cacheConf) .withPath(fs, path) .withComparator(comparator) @@ -886,19 +925,6 @@ public class StoreFile { } /** - * Set TimeRangeTracker. - * Called when flushing to pass us a pre-calculated TimeRangeTracker, one made during updates - * to memstore so we don't have to make one ourselves as Cells get appended. Call before first - * append. If this method is not called, we will calculate our own range of the Cells that - * comprise this StoreFile (and write them on the end as metadata). It is good to have this stuff - * passed because it is expensive to make. - */ - public void setTimeRangeTracker(final TimeRangeTracker trt) { - this.timeRangeTracker = trt; - timeRangeTrackerSet = true; - } - - /** * Record the earlest Put timestamp. * * If the timeRangeTracker is not set, diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/StripeStoreFlusher.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/StripeStoreFlusher.java index 0c3432c..c367b52 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/StripeStoreFlusher.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/StripeStoreFlusher.java @@ -115,8 +115,8 @@ public class StripeStoreFlusher extends StoreFlusher { /* isCompaction = */ false, /* includeMVCCReadpoint = */ true, /* includesTags = */ true, - /* shouldDropBehind = */ false); - writer.setTimeRangeTracker(tracker); + /* shouldDropBehind = */ false, + tracker); return writer; } }; diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/TimeRangeTracker.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/TimeRangeTracker.java index f4175cc..0c1d723 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/TimeRangeTracker.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/TimeRangeTracker.java @@ -22,6 +22,8 @@ import java.io.DataInput; import java.io.DataOutput; import java.io.IOException; +import org.apache.commons.logging.Log; +import org.apache.commons.logging.LogFactory; import org.apache.hadoop.hbase.Cell; import org.apache.hadoop.hbase.CellUtil; import org.apache.hadoop.hbase.classification.InterfaceAudience; @@ -44,6 +46,7 @@ import org.apache.hadoop.io.Writable; */ @InterfaceAudience.Private public class TimeRangeTracker implements Writable { + private static final Log LOG = LogFactory.getLog(TimeRangeTracker.class); static final long INITIAL_MIN_TIMESTAMP = Long.MAX_VALUE; long minimumTimestamp = INITIAL_MIN_TIMESTAMP; static final long INITIAL_MAX_TIMESTAMP = -1; @@ -201,6 +204,10 @@ public class TimeRangeTracker implements Writable { if (isFreshInstance()) { return new TimeRange(); } + if (min == TimeRangeTracker.INITIAL_MIN_TIMESTAMP) { + LOG.warn("BAD: MINIMUM IS " + INITIAL_MIN_TIMESTAMP + " " + this); + min = TimeRange.INITIAL_MIN_TIMESTAMP; + } return new TimeRange(min, max); } -} +} \ No newline at end of file diff --git a/hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestTimeRangeTracker.java b/hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestTimeRangeTracker.java index 46fe611..cfd80e2 100644 --- a/hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestTimeRangeTracker.java +++ b/hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestTimeRangeTracker.java @@ -17,16 +17,46 @@ */ package org.apache.hadoop.hbase.regionserver; +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertTrue; +import java.io.IOException; + import org.apache.hadoop.hbase.io.TimeRange; import org.apache.hadoop.hbase.testclassification.SmallTests; +import org.apache.hadoop.hbase.util.Writables; import org.junit.Test; import org.junit.experimental.categories.Category; @Category({SmallTests.class}) public class TestTimeRangeTracker { @Test + public void testTimeRangeInitialized() { + TimeRangeTracker src = new TimeRangeTracker(); + TimeRange tr = new TimeRange(System.currentTimeMillis()); + assertFalse(src.includesTimeRange(tr)); + } + + @Test + public void testTimeRangeTrackerNullIsSameAsTimeRangeNull() throws IOException { + TimeRangeTracker src = new TimeRangeTracker(1, 2); + byte [] bytes = Writables.getBytes(src); + TimeRange tgt = TimeRangeTracker.getTimeRange(bytes); + assertEquals(src.getMin(), tgt.getMin()); + assertEquals(src.getMax(), tgt.getMax()); + } + + @Test + public void testSerialization() throws IOException { + TimeRangeTracker src = new TimeRangeTracker(1, 2); + TimeRangeTracker tgt = new TimeRangeTracker(); + Writables.copyWritable(src, tgt); + assertEquals(src.getMin(), tgt.getMin()); + assertEquals(src.getMax(), tgt.getMax()); + } + + @Test public void testAlwaysDecrementingSetsMaximum() { TimeRangeTracker trr = new TimeRangeTracker(); trr.includeTimestamp(3); -- 2.6.1