From 9c1beb56badb1b5237f58cfc5bc3916f7c026709 Mon Sep 17 00:00:00 2001 From: stack Date: Wed, 15 Oct 2014 10:36:56 -0700 Subject: [PATCH] In AtomicUtils, change updateMin and updateMax to return whether they actually updated values (actually not used by this patch but useful) HBASE-12148 Remove TimeRangeTracker as point of contention when many threads writing a Store -- Retry Remove synchronize in TimeRangeTracker and use AtomicLongs instead. Use the AtomicUtil updateMin and updateMax doing updating. Behavior changes slightly in that min and max are no longer tied by synchronization so better performance. Add a few new tests around edges. New illegalstateexception if TRT initialized and used without having been written. --- .../org/apache/hadoop/hbase/util/AtomicUtils.java | 12 +++- .../hbase/regionserver/TimeRangeTracker.java | 82 +++++++++------------- .../hbase/regionserver/TestTimeRangeTracker.java | 20 +++++- 3 files changed, 61 insertions(+), 53 deletions(-) diff --git a/hbase-common/src/main/java/org/apache/hadoop/hbase/util/AtomicUtils.java b/hbase-common/src/main/java/org/apache/hadoop/hbase/util/AtomicUtils.java index 35391ee..640368f 100644 --- a/hbase-common/src/main/java/org/apache/hadoop/hbase/util/AtomicUtils.java +++ b/hbase-common/src/main/java/org/apache/hadoop/hbase/util/AtomicUtils.java @@ -29,8 +29,10 @@ public class AtomicUtils { /** * Updates a AtomicLong which is supposed to maintain the minimum values. This method is not * synchronized but is thread-safe. + * @return True if we updated min or false if current min is less than passed value. */ - public static void updateMin(AtomicLong min, long value) { + public static boolean updateMin(AtomicLong min, long value) { + boolean b = false; while (true) { long cur = min.get(); if (value >= cur) { @@ -38,16 +40,20 @@ public class AtomicUtils { } if (min.compareAndSet(cur, value)) { + b = true; break; } } + return b; } /** * Updates a AtomicLong which is supposed to maintain the maximum values. This method is not * synchronized but is thread-safe. + * @return True if we updated max or false if current max is greater than passed value. */ - public static void updateMax(AtomicLong max, long value) { + public static boolean updateMax(AtomicLong max, long value) { + boolean b = false; while (true) { long cur = max.get(); if (value <= cur) { @@ -55,9 +61,11 @@ public class AtomicUtils { } if (max.compareAndSet(cur, value)) { + b = true; break; } } + return b; } } 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 0044634..ff7ab07 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 @@ -21,11 +21,13 @@ package org.apache.hadoop.hbase.regionserver; import java.io.DataInput; import java.io.DataOutput; import java.io.IOException; +import java.util.concurrent.atomic.AtomicLong; 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.AtomicUtils; import org.apache.hadoop.io.Writable; /** @@ -38,8 +40,9 @@ import org.apache.hadoop.io.Writable; @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_MAXIMUM_TIMESTAMP = -1; + private AtomicLong minimumTimestamp = new AtomicLong(INITIAL_MINIMUM_TIMESTAMP); + private AtomicLong maximumTimestamp = new AtomicLong(INITIAL_MAXIMUM_TIMESTAMP); /** * Default constructor. @@ -52,27 +55,14 @@ public class TimeRangeTracker implements Writable { * @param trt source TimeRangeTracker */ public TimeRangeTracker(final TimeRangeTracker trt) { - set(trt.getMinimumTimestamp(), trt.getMaximumTimestamp()); + this(trt.getMinimumTimestamp(), trt.getMaximumTimestamp()); } public TimeRangeTracker(long minimumTimestamp, long maximumTimestamp) { - set(minimumTimestamp, maximumTimestamp); + this.minimumTimestamp.set(minimumTimestamp); + this.maximumTimestamp.set(maximumTimestamp); } - private void set(final long min, final long max) { - this.minimumTimestamp = min; - this.maximumTimestamp = max; - } - - /** - * @param l - * @return True if we initialized values - */ - private boolean init(final long l) { - if (this.minimumTimestamp != INITIAL_MINIMUM_TIMESTAMP) return false; - set(l, l); - return true; - } /** * Update the current TimestampRange to include the timestamp from Cell @@ -92,63 +82,55 @@ public class TimeRangeTracker implements Writable { * @param timestamp the timestamp value to include */ void includeTimestamp(final long timestamp) { - // Do test outside of synchronization block. Synchronization in here can be problematic - // when many threads writing one Store -- they can all pile up trying to add in here. - // Happens when doing big write upload where we are hammering on one region. - if (timestamp < this.minimumTimestamp) { - synchronized (this) { - if (!init(timestamp)) { - if (timestamp < this.minimumTimestamp) { - this.minimumTimestamp = timestamp; - } - } - } - } else if (timestamp > this.maximumTimestamp) { - synchronized (this) { - if (!init(timestamp)) { - if (this.maximumTimestamp < timestamp) { - this.maximumTimestamp = timestamp; - } - } - } - } + // Call both updateMin and updateMax. This way we clear initial values. On first call, + // min == max. Subsequently, they diverge even if all we do is go more and more min (or more + // and more max). + AtomicUtils.updateMin(this.minimumTimestamp, timestamp); + AtomicUtils.updateMax(this.maximumTimestamp, timestamp); } /** - * Check if the range has any overlap with TimeRange + * Check if the range has any overlap with TimeRange. + * The min and max are not tied with a synchronize. Either may change after we enter this check. + * That should be fine though. Min will get smaller, and max will only get larger. * @param tr TimeRange * @return True if there is overlap, false otherwise */ - public synchronized boolean includesTimeRange(final TimeRange tr) { - return (this.minimumTimestamp < tr.getMax() && this.maximumTimestamp >= tr.getMin()); + public boolean includesTimeRange(final TimeRange tr) { + long min = this.minimumTimestamp.get(); + long max = this.maximumTimestamp.get(); + if (min == INITIAL_MINIMUM_TIMESTAMP || max == INITIAL_MAXIMUM_TIMESTAMP) { + throw new IllegalStateException("Initialized but min and max not set"); + } + return this.minimumTimestamp.get() < tr.getMax() && this.maximumTimestamp.get() >= tr.getMin(); } /** * @return the minimumTimestamp */ - public synchronized long getMinimumTimestamp() { - return minimumTimestamp; + public long getMinimumTimestamp() { + return minimumTimestamp.get(); } /** * @return the maximumTimestamp */ - public synchronized long getMaximumTimestamp() { - return maximumTimestamp; + public long getMaximumTimestamp() { + return maximumTimestamp.get(); } public synchronized void write(final DataOutput out) throws IOException { - out.writeLong(minimumTimestamp); - out.writeLong(maximumTimestamp); + out.writeLong(getMinimumTimestamp()); + out.writeLong(getMaximumTimestamp()); } public synchronized void readFields(final DataInput in) throws IOException { - this.minimumTimestamp = in.readLong(); - this.maximumTimestamp = in.readLong(); + this.minimumTimestamp.set(in.readLong()); + this.maximumTimestamp.set(in.readLong()); } @Override public synchronized String toString() { - return "[" + minimumTimestamp + "," + maximumTimestamp + "]"; + return "[" + getMinimumTimestamp() + "," + getMaximumTimestamp() + "]"; } } \ 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 edec023..a3e0d87 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 @@ -27,6 +27,24 @@ import org.junit.experimental.categories.Category; @Category({RegionServerTests.class, SmallTests.class}) public class TestTimeRangeTracker { + @Test (expected=IllegalStateException.class) + public void testExpectedException() { + TimeRangeTracker trr = new TimeRangeTracker(); + assertTrue(trr.getMinimumTimestamp() == TimeRangeTracker.INITIAL_MINIMUM_TIMESTAMP); + assertTrue(trr.getMaximumTimestamp() == TimeRangeTracker.INITIAL_MAXIMUM_TIMESTAMP); + trr.includesTimeRange(new TimeRange(1)); + } + + @Test + public void testInitialized() { + TimeRangeTracker trr = new TimeRangeTracker(); + assertTrue(trr.getMinimumTimestamp() == TimeRangeTracker.INITIAL_MINIMUM_TIMESTAMP); + assertTrue(trr.getMaximumTimestamp() == TimeRangeTracker.INITIAL_MAXIMUM_TIMESTAMP); + trr.includeTimestamp(1); + assertTrue(trr.getMinimumTimestamp() != TimeRangeTracker.INITIAL_MINIMUM_TIMESTAMP); + assertTrue(trr.getMaximumTimestamp() != TimeRangeTracker.INITIAL_MAXIMUM_TIMESTAMP); + } + @Test public void testAlwaysDecrementingSetsMaximum() { TimeRangeTracker trr = new TimeRangeTracker(); @@ -34,7 +52,7 @@ public class TestTimeRangeTracker { trr.includeTimestamp(2); trr.includeTimestamp(1); assertTrue(trr.getMinimumTimestamp() != TimeRangeTracker.INITIAL_MINIMUM_TIMESTAMP); - assertTrue(trr.getMaximumTimestamp() != -1 /*The initial max value*/); + assertTrue(trr.getMaximumTimestamp() != TimeRangeTracker.INITIAL_MAXIMUM_TIMESTAMP); } @Test -- 1.8.5.2 (Apple Git-48)