HBASE-15650 Remove TimeRangeTracker as point of contention when many threads reading a StoreFile

Refactor so we use the immutable, unsynchronized TimeRange when doing
time-based checks at read time rather than use heavily synchronized
TimeRangeTracker; let TimeRangeTracker be for write-time only.

While in here, changed the Segment stuff so that when an immutable
segment, it uses TimeRange rather than TimeRangeTracker too.

M hbase-common/src/main/java/org/apache/hadoop/hbase/io/TimeRange.java
  Make allTime final.
  Add a includesTimeRange method copied from TimeRangeTracker.

M hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/TimeRangeTracker.java
  Change name of a few methods so they match TimeRange methods that do
  same thing.
  (getTimeRangeTracker, getTimeRange, toTimeRange) add utility methods

M hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/StoreFile.java
  Change Reader to use TimeRange-based checks instead of
  TimeRangeTracker.

hbase-common/src/main/java/org/apache/hadoop/hbase/io/TimeRange.java

@@ -36,11 +36,13 @@ import org.apache.hadoop.hbase.util.Bytes;
 @InterfaceAudience.Public
 @InterfaceStability.Stable
 public class TimeRange {
-  private static final long MIN_TIME = 0L;
-  private static final long MAX_TIME = Long.MAX_VALUE;
+  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;
   private long minStamp = MIN_TIME;
   private long maxStamp = MAX_TIME;
-  private boolean allTime = false;
+  private final boolean allTime;
 
   /**
    * Default constructor.
@@ -60,9 +62,7 @@ public class TimeRange {
   @Deprecated
   public TimeRange(long minStamp) {
     this.minStamp = minStamp;
-    if (this.minStamp == MIN_TIME){
-      this.allTime = true;
-    }
+    this.allTime = this.minStamp == MIN_TIME;
   }
 
   /**
@@ -73,6 +73,7 @@ public class TimeRange {
   @Deprecated
   public TimeRange(byte [] minStamp) {
     this.minStamp = Bytes.toLong(minStamp);
+    this.allTime = false;
   }
 
   /**
@@ -94,9 +95,7 @@ public class TimeRange {
     }
     this.minStamp = minStamp;
     this.maxStamp = maxStamp;
-    if (this.minStamp == MIN_TIME && this.maxStamp == MAX_TIME){
-      this.allTime = true;
-    }
+    this.allTime = this.minStamp == MIN_TIME && this.maxStamp == MAX_TIME;
   }
 
   /**
@@ -157,11 +156,27 @@ public class TimeRange {
    * @return true if within TimeRange, false if not
    */
   public boolean withinTimeRange(long timestamp) {
-    if(allTime) return true;
+    if (this.allTime) {
+      return true;
+    }
     // check if >= minStamp
     return (minStamp <= timestamp && timestamp < maxStamp);
   }
 
+  /**
+   * Check if the range has any overlap with TimeRange
+   * @param tr TimeRange
+   * @return True if there is overlap, false otherwise
+   */
+  // This method came from TimeRangeTracker. We used to go there for this function but better
+  // to come here to the immutable, unsynchronized datastructure at read time.
+  public boolean includesTimeRange(final TimeRange tr) {
+    if (this.allTime) {
+      return true;
+    }
+    return getMin() < tr.getMax() && getMax() >= tr.getMin();
+  }
+
   /**
    * Check if the specified timestamp is within this TimeRange.
    * <p>

hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/HFilePrettyPrinter.java

@@ -401,8 +401,7 @@ public class HFilePrettyPrinter extends Configured implements Tool {
       } else if (Bytes.compareTo(e.getKey(), Bytes.toBytes("TIMERANGE")) == 0) {
         TimeRangeTracker timeRangeTracker = new TimeRangeTracker();
         Writables.copyWritable(e.getValue(), timeRangeTracker);
-        System.out.println(timeRangeTracker.getMinimumTimestamp() + "...."
-            + timeRangeTracker.getMaximumTimestamp());
+        System.out.println(timeRangeTracker.getMin() + "...." + timeRangeTracker.getMax());
       } else if (Bytes.compareTo(e.getKey(), FileInfo.AVG_KEY_LEN) == 0
           || Bytes.compareTo(e.getKey(), FileInfo.AVG_VALUE_LEN) == 0) {
         System.out.println(Bytes.toInt(e.getValue()));

hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/DefaultMemStore.java

@@ -678,10 +678,8 @@ public class DefaultMemStore implements MemStore {
       timeRange = scan.getTimeRange();
     }
     return (timeRangeTracker.includesTimeRange(timeRange) ||
-        snapshotTimeRangeTracker.includesTimeRange(timeRange))
-        && (Math.max(timeRangeTracker.getMaximumTimestamp(),
-                     snapshotTimeRangeTracker.getMaximumTimestamp()) >=
-            oldestUnexpiredTS);
+      snapshotTimeRangeTracker.includesTimeRange(timeRange)) &&
+      (Math.max(timeRangeTracker.getMax(), snapshotTimeRangeTracker.getMax()) >= oldestUnexpiredTS);
   }
 
   /*

hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/StoreFile.java

@@ -503,15 +503,11 @@ public class StoreFile {
     reader.loadBloomfilter(BlockType.DELETE_FAMILY_BLOOM_META);
 
     try {
-      byte [] timerangeBytes = metadataMap.get(TIMERANGE_KEY);
-      if (timerangeBytes != null) {
-        this.reader.timeRangeTracker = new TimeRangeTracker();
-        Writables.copyWritable(timerangeBytes, this.reader.timeRangeTracker);
-      }
+      this.reader.timeRange = TimeRangeTracker.getTimeRange(metadataMap.get(TIMERANGE_KEY));
     } catch (IllegalArgumentException e) {
       LOG.error("Error reading timestamp range data from meta -- " +
           "proceeding without", e);
-      this.reader.timeRangeTracker = null;
+      this.reader.timeRange = null;
     }
     // initialize so we can reuse them after reader closed.
     firstKey = reader.getFirstKey();
@@ -743,15 +739,11 @@ public class StoreFile {
   }
 
   public Long getMinimumTimestamp() {
-    return (getReader().timeRangeTracker == null) ?
-        null :
-        getReader().timeRangeTracker.getMinimumTimestamp();
+    return getReader().timeRange == null? null: getReader().timeRange.getMin();
   }
 
   public Long getMaximumTimestamp() {
-    return (getReader().timeRangeTracker == null) ?
-        null :
-        getReader().timeRangeTracker.getMaximumTimestamp();
+    return getReader().timeRange == null? null: getReader().timeRange.getMax();
   }
 
 
@@ -805,13 +797,14 @@ public class StoreFile {
     private long deleteFamilyCnt = 0;
 
     TimeRangeTracker timeRangeTracker = new TimeRangeTracker();
-    /* isTimeRangeTrackerSet keeps track if the timeRange has already been set
-     * When flushing a memstore, we set TimeRange and use this variable to
-     * indicate that it doesn't need to be calculated again while
-     * appending KeyValues.
-     * It is not set in cases of compactions when it is recalculated using only
-     * the appended KeyValues*/
-    boolean isTimeRangeTrackerSet = false;
+    /**
+     * 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
+     * memstore in here into this Writer and use this variable to indicate that we do not need to
+     * 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;
 
     protected HFile.Writer writer;
 
@@ -895,12 +888,16 @@ public class StoreFile {
     }
 
     /**
-     * Set TimeRangeTracker
-     * @param trt
+     * 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;
-      isTimeRangeTrackerSet = true;
+      timeRangeTrackerSet = true;
     }
 
     /**
@@ -914,7 +911,7 @@ public class StoreFile {
       if (KeyValue.Type.Put.getCode() == cell.getTypeByte()) {
         earliestPutTs = Math.min(earliestPutTs, cell.getTimestamp());
       }
-      if (!isTimeRangeTrackerSet) {
+      if (!timeRangeTrackerSet) {
         timeRangeTracker.includeTimestamp(cell);
       }
     }
@@ -1113,7 +1110,7 @@ public class StoreFile {
     protected BloomFilter deleteFamilyBloomFilter = null;
     protected BloomType bloomFilterType;
     private final HFile.Reader reader;
-    protected TimeRangeTracker timeRangeTracker = null;
+    protected TimeRange timeRange;
     protected long sequenceID = -1;
     private byte[] lastBloomKey;
     private long deleteFamilyCnt = -1;
@@ -1260,13 +1257,9 @@ public class StoreFile {
      *          determined by the column family's TTL
      * @return false if queried keys definitely don't exist in this StoreFile
      */
-    boolean passesTimerangeFilter(TimeRange timeRange, long oldestUnexpiredTS) {
-      if (timeRangeTracker == null) {
-        return true;
-      } else {
-        return timeRangeTracker.includesTimeRange(timeRange) &&
-            timeRangeTracker.getMaximumTimestamp() >= oldestUnexpiredTS;
-      }
+    boolean passesTimerangeFilter(TimeRange tr, long oldestUnexpiredTS) {
+      return this.timeRange == null? true:
+        this.timeRange.includesTimeRange(tr) && this.timeRange.getMax() >= oldestUnexpiredTS;
     }
 
     /**
@@ -1666,7 +1659,7 @@ public class StoreFile {
     }
 
     public long getMaxTimestamp() {
-      return timeRangeTracker == null ? Long.MAX_VALUE : timeRangeTracker.getMaximumTimestamp();
+      return timeRange == null ? Long.MAX_VALUE : timeRange.getMax();
     }
   }
 

hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/TimeRangeTracker.java

@@ -26,20 +26,28 @@ import org.apache.hadoop.hbase.Cell;
 import org.apache.hadoop.hbase.CellUtil;
 import org.apache.hadoop.hbase.classification.InterfaceAudience;
 import org.apache.hadoop.hbase.io.TimeRange;
+import org.apache.hadoop.hbase.util.Writables;
 import org.apache.hadoop.io.Writable;
 
 /**
- * Stores the minimum and maximum timestamp values (both are inclusive).
- * Can be used to find if any given time range overlaps with its time range
- * MemStores use this class to track its minimum and maximum timestamps.
- * When writing StoreFiles, this information is stored in meta blocks and used
- * at read time to match against the required TimeRange.
+ * Stores minimum and maximum timestamp values. Both timestamps are inclusive.
+ * Use this class at write-time ONLY. Too much synchronization to use at read time
+ * (TODO: there are two scenarios writing, once when lots of concurrency as part of memstore
+ * updates but then later we can make one as part of a compaction when there is only one thread
+ * involved -- consider making different version, the synchronized and the unsynchronized).
+ * Use {@link TimeRange} at read time instead of this. See toTimeRange() to make TimeRange to use.
+ * MemStores use this class to track minimum and maximum timestamps. The TimeRangeTracker made by
+ * the MemStore is passed to the StoreFile for it to write out as part a flush in the the file
+ * metadata. If no memstore involved -- i.e. a compaction -- then the StoreFile will calculate its
+ * own TimeRangeTracker as it appends. The StoreFile serialized TimeRangeTracker is used
+ * at read time via an instance of {@link TimeRange} to test if Cells fit the StoreFile TimeRange.
  */
 @InterfaceAudience.Private
 public class TimeRangeTracker implements Writable {
-  static final long INITIAL_MINIMUM_TIMESTAMP = Long.MAX_VALUE;
-  long minimumTimestamp = INITIAL_MINIMUM_TIMESTAMP;
-  long maximumTimestamp = -1;
+  static final long INITIAL_MIN_TIMESTAMP = Long.MAX_VALUE;
+  long minimumTimestamp = INITIAL_MIN_TIMESTAMP;
+  static final long INITIAL_MAX_TIMESTAMP = -1;
+  long maximumTimestamp = INITIAL_MAX_TIMESTAMP;
 
   /**
    * Default constructor.
@@ -52,7 +60,7 @@ public class TimeRangeTracker implements Writable {
    * @param trt source TimeRangeTracker
    */
   public TimeRangeTracker(final TimeRangeTracker trt) {
-    set(trt.getMinimumTimestamp(), trt.getMaximumTimestamp());
+    set(trt.getMin(), trt.getMax());
   }
 
   public TimeRangeTracker(long minimumTimestamp, long maximumTimestamp) {
@@ -69,13 +77,13 @@ public class TimeRangeTracker implements Writable {
    * @return True if we initialized values
    */
   private boolean init(final long l) {
-    if (this.minimumTimestamp != INITIAL_MINIMUM_TIMESTAMP) return false;
+    if (this.minimumTimestamp != INITIAL_MIN_TIMESTAMP) return false;
     set(l, l);
     return true;
   }
 
   /**
-   * Update the current TimestampRange to include the timestamp from Cell
+   * Update the current TimestampRange to include the timestamp from <code>cell</code>.
    * If the Key is of type DeleteColumn or DeleteFamily, it includes the
    * entire time range from 0 to timestamp of the key.
    * @param cell the Cell to include
@@ -128,14 +136,14 @@ public class TimeRangeTracker implements Writable {
   /**
    * @return the minimumTimestamp
    */
-  public synchronized long getMinimumTimestamp() {
+  public synchronized long getMin() {
     return minimumTimestamp;
   }
 
   /**
    * @return the maximumTimestamp
    */
-  public synchronized long getMaximumTimestamp() {
+  public synchronized long getMax() {
     return maximumTimestamp;
   }
 
@@ -153,4 +161,46 @@ public class TimeRangeTracker implements Writable {
   public synchronized String toString() {
     return "[" + minimumTimestamp + "," + maximumTimestamp + "]";
   }
+
+  /**
+   * @return An instance of TimeRangeTracker filled w/ the content of serialized
+   * TimeRangeTracker in <code>timeRangeTrackerBytes</code>.
+   * @throws IOException
+   */
+  public static TimeRangeTracker getTimeRangeTracker(final byte [] timeRangeTrackerBytes)
+  throws IOException {
+    if (timeRangeTrackerBytes == null) return null;
+    TimeRangeTracker trt = new TimeRangeTracker();
+    Writables.copyWritable(timeRangeTrackerBytes, trt);
+    return trt;
+  }
+
+  /**
+   * @return An instance of a TimeRange made from the serialized TimeRangeTracker passed in
+   * <code>timeRangeTrackerBytes</code>.
+   * @throws IOException
+   */
+  static TimeRange getTimeRange(final byte [] timeRangeTrackerBytes) throws IOException {
+    TimeRangeTracker trt = getTimeRangeTracker(timeRangeTrackerBytes);
+    return trt == null? null: trt.toTimeRange();
+  }
+
+  private boolean isFreshInstance() {
+    return getMin() == INITIAL_MIN_TIMESTAMP && getMax() == INITIAL_MAX_TIMESTAMP;
+  }
+
+  /**
+   * @return Make a TimeRange from current state of <code>this</code>.
+   */
+  TimeRange toTimeRange() throws IOException {
+    long min = getMin();
+    long max = getMax();
+    // Check for the case where the TimeRangeTracker is fresh. In that case it has
+    // initial values that are antithetical to a TimeRange... Return an uninitialized TimeRange
+    // if passed an uninitialized TimeRangeTracker.
+    if (isFreshInstance()) {
+      return new TimeRange();
+    }
+    return new TimeRange(min, max);
+  }
 }

hbase-server/src/test/java/org/apache/hadoop/hbase/mapreduce/TestHFileOutputFormat.java

@@ -286,10 +286,9 @@ public class TestHFileOutputFormat  {
       // unmarshall and check values.
       TimeRangeTracker timeRangeTracker = new TimeRangeTracker();
       Writables.copyWritable(range, timeRangeTracker);
-      LOG.info(timeRangeTracker.getMinimumTimestamp() +
-          "...." + timeRangeTracker.getMaximumTimestamp());
-      assertEquals(1000, timeRangeTracker.getMinimumTimestamp());
-      assertEquals(2000, timeRangeTracker.getMaximumTimestamp());
+      LOG.info(timeRangeTracker.getMin() + "...." + timeRangeTracker.getMax());
+      assertEquals(1000, timeRangeTracker.getMin());
+      assertEquals(2000, timeRangeTracker.getMax());
       rd.close();
     } finally {
       if (writer != null && context != null) writer.close(context);

hbase-server/src/test/java/org/apache/hadoop/hbase/mapreduce/TestHFileOutputFormat2.java

@@ -293,10 +293,10 @@ public class TestHFileOutputFormat2  {
       // unmarshall and check values.
       TimeRangeTracker timeRangeTracker = new TimeRangeTracker();
       Writables.copyWritable(range, timeRangeTracker);
-      LOG.info(timeRangeTracker.getMinimumTimestamp() +
-          "...." + timeRangeTracker.getMaximumTimestamp());
-      assertEquals(1000, timeRangeTracker.getMinimumTimestamp());
-      assertEquals(2000, timeRangeTracker.getMaximumTimestamp());
+      LOG.info(timeRangeTracker.getMin() +
+          "...." + timeRangeTracker.getMax());
+      assertEquals(1000, timeRangeTracker.getMin());
+      assertEquals(2000, timeRangeTracker.getMax());
       rd.close();
     } finally {
       if (writer != null && context != null) writer.close(context);

hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/MockStoreFile.java

@@ -112,12 +112,12 @@ public class MockStoreFile extends StoreFile {
 
   public Long getMinimumTimestamp() {
     return (timeRangeTracker == null) ?
-      null : timeRangeTracker.getMinimumTimestamp();
+      null : timeRangeTracker.getMin();
   }
 
   public Long getMaximumTimestamp() {
     return (timeRangeTracker == null) ?
-      null : timeRangeTracker.getMaximumTimestamp();
+      null : timeRangeTracker.getMax();
   }
 
   @Override
@@ -133,7 +133,7 @@ public class MockStoreFile extends StoreFile {
   @Override
   public StoreFile.Reader getReader() {
     final long len = this.length;
-    final TimeRangeTracker timeRange = this.timeRangeTracker;
+    final TimeRangeTracker timeRangeTracker = this.timeRangeTracker;
     final long entries = this.entryCount;
     return new StoreFile.Reader() {
       @Override
@@ -143,7 +143,7 @@ public class MockStoreFile extends StoreFile {
 
       @Override
       public long getMaxTimestamp() {
-        return timeRange == null ? Long.MAX_VALUE : timeRange.maximumTimestamp;
+        return timeRange == null? Long.MAX_VALUE: timeRangeTracker.getMax();
       }
 
       @Override

hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestTimeRangeTracker.java

@@ -36,8 +36,8 @@ public class TestTimeRangeTracker {
     trr.includeTimestamp(3);
     trr.includeTimestamp(2);
     trr.includeTimestamp(1);
-    assertTrue(trr.getMinimumTimestamp() != TimeRangeTracker.INITIAL_MINIMUM_TIMESTAMP);
-    assertTrue(trr.getMaximumTimestamp() != -1 /*The initial max value*/);
+    assertTrue(trr.getMin() != TimeRangeTracker.INITIAL_MIN_TIMESTAMP);
+    assertTrue(trr.getMax() != -1 /*The initial max value*/);
   }
 
   @Test
@@ -80,8 +80,8 @@ public class TestTimeRangeTracker {
     for (int i = 0; i < threads.length; i++) {
       threads[i].join();
     }
-    assertTrue(trr.getMaximumTimestamp() == calls * threadCount);
-    assertTrue(trr.getMinimumTimestamp() == 0);
+    assertTrue(trr.getMax() == calls * threadCount);
+    assertTrue(trr.getMin() == 0);
   }
 
   @Test
@@ -141,7 +141,7 @@ public class TestTimeRangeTracker {
     for (int i = 0; i < threads.length; i++) {
       threads[i].join();
     }
-    System.out.println(trr.getMinimumTimestamp() + " " + trr.getMaximumTimestamp() + " " +
+    System.out.println(trr.getMin() + " " + trr.getMax() + " " +
       (System.currentTimeMillis() - start));
   }
 }