From 13a4e2d51913f1d69f598c96aebd526297b57755 Mon Sep 17 00:00:00 2001 From: Dmitriy Kukharev Date: Tue, 5 Mar 2019 15:28:16 +0300 Subject: [PATCH] HBASE-21774 Added a new method to EnvironmentEdge: monotonicTime which is better suited for measuring intervals. --- .../hbase/client/SimpleRequestController.java | 4 ++-- .../hbase/util/DefaultEnvironmentEdge.java | 12 +----------- .../hadoop/hbase/util/EnvironmentEdge.java | 19 +++++++++++++++++-- .../hbase/util/EnvironmentEdgeManager.java | 10 ++++++++++ .../quotas/AverageIntervalRateLimiter.java | 4 ++-- .../quotas/FixedIntervalRateLimiter.java | 4 ++-- .../hbase/util/ManualEnvironmentEdge.java | 7 ++++++- .../hadoop/hbase/quotas/TestRateLimiter.java | 18 +++++++++--------- .../util/TestDefaultEnvironmentEdge.java | 11 ++++------- 9 files changed, 53 insertions(+), 36 deletions(-) diff --git a/hbase-client/src/main/java/org/apache/hadoop/hbase/client/SimpleRequestController.java b/hbase-client/src/main/java/org/apache/hadoop/hbase/client/SimpleRequestController.java index 261d663e0f..d7f125ca6a 100644 --- a/hbase-client/src/main/java/org/apache/hadoop/hbase/client/SimpleRequestController.java +++ b/hbase-client/src/main/java/org/apache/hadoop/hbase/client/SimpleRequestController.java @@ -408,8 +408,8 @@ class SimpleRequestController implements RequestController { return; } EnvironmentEdge ee = EnvironmentEdgeManager.getDelegate(); - final long start = ee.currentTime(); - while ((ee.currentTime() - start) <= MAX_WAITING_TIME) { + final long start = ee.monotonicTime(); + while ((ee.monotonicTime() - start) <= MAX_WAITING_TIME) { for (byte[] region : busyRegions) { AtomicInteger count = taskCounterPerRegion.get(region); if (count == null || count.get() < maxConcurrentTasksPerRegion) { diff --git a/hbase-common/src/main/java/org/apache/hadoop/hbase/util/DefaultEnvironmentEdge.java b/hbase-common/src/main/java/org/apache/hadoop/hbase/util/DefaultEnvironmentEdge.java index 422cc16507..83210f2aa9 100644 --- a/hbase-common/src/main/java/org/apache/hadoop/hbase/util/DefaultEnvironmentEdge.java +++ b/hbase-common/src/main/java/org/apache/hadoop/hbase/util/DefaultEnvironmentEdge.java @@ -24,15 +24,5 @@ import org.apache.yetus.audience.InterfaceAudience; * Default implementation of an environment edge. */ @InterfaceAudience.Private -public class DefaultEnvironmentEdge implements EnvironmentEdge { - /** - * {@inheritDoc} - *

- * This implementation returns {@link System#currentTimeMillis()} - *

- */ - @Override - public long currentTime() { - return System.currentTimeMillis(); - } +/* package */ class DefaultEnvironmentEdge implements EnvironmentEdge { } diff --git a/hbase-common/src/main/java/org/apache/hadoop/hbase/util/EnvironmentEdge.java b/hbase-common/src/main/java/org/apache/hadoop/hbase/util/EnvironmentEdge.java index 635c2764d2..03b21a1c06 100644 --- a/hbase-common/src/main/java/org/apache/hadoop/hbase/util/EnvironmentEdge.java +++ b/hbase-common/src/main/java/org/apache/hadoop/hbase/util/EnvironmentEdge.java @@ -20,6 +20,8 @@ package org.apache.hadoop.hbase.util; import org.apache.yetus.audience.InterfaceAudience; +import java.util.concurrent.TimeUnit; + /** * Has some basic interaction with the environment. Alternate implementations * can be used where required (eg in tests). @@ -29,9 +31,22 @@ import org.apache.yetus.audience.InterfaceAudience; @InterfaceAudience.Private public interface EnvironmentEdge { /** - * Returns the currentTime. + * Returns the currentTime in milliseconds. * * @return Current time. */ - long currentTime(); + default long currentTime() { + return System.currentTimeMillis(); + } + + /** + * Returns current monotonic time from some arbitrary time base in the past (or in the future). + * Use this for measuring intervals as {@code #currentTime} is not reliable - it is system-clock + * dependent. See the documentation of System#nanoTime for caveats since the default + * implementation of this method uses it. + * @return a monotonic clock that counts in milliseconds. + */ + default long monotonicTime() { + return TimeUnit.NANOSECONDS.toMillis(System.nanoTime()); + } } diff --git a/hbase-common/src/main/java/org/apache/hadoop/hbase/util/EnvironmentEdgeManager.java b/hbase-common/src/main/java/org/apache/hadoop/hbase/util/EnvironmentEdgeManager.java index c38ddf7f86..af3081da41 100644 --- a/hbase-common/src/main/java/org/apache/hadoop/hbase/util/EnvironmentEdgeManager.java +++ b/hbase-common/src/main/java/org/apache/hadoop/hbase/util/EnvironmentEdgeManager.java @@ -108,4 +108,14 @@ public class EnvironmentEdgeManager { public static long currentTime() { return getDelegate().currentTime(); } + + /** + * Defers to the delegate and calls the + * {@link EnvironmentEdge#monotonicTime()} method. + * + * @return current monotonic time in millis according to the delegate. + */ + public static long monotonicTime() { + return getDelegate().monotonicTime(); + } } diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/quotas/AverageIntervalRateLimiter.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/quotas/AverageIntervalRateLimiter.java index b245795919..2c6545393c 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/quotas/AverageIntervalRateLimiter.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/quotas/AverageIntervalRateLimiter.java @@ -27,10 +27,10 @@ public class AverageIntervalRateLimiter extends RateLimiter { @Override public long refill(long limit) { - final long now = EnvironmentEdgeManager.currentTime(); + final long now = EnvironmentEdgeManager.monotonicTime(); if (nextRefillTime == -1) { // Till now no resource has been consumed. - nextRefillTime = EnvironmentEdgeManager.currentTime(); + nextRefillTime = EnvironmentEdgeManager.monotonicTime(); return limit; } diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/quotas/FixedIntervalRateLimiter.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/quotas/FixedIntervalRateLimiter.java index e67eda5c5b..999d43112b 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/quotas/FixedIntervalRateLimiter.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/quotas/FixedIntervalRateLimiter.java @@ -25,7 +25,7 @@ public class FixedIntervalRateLimiter extends RateLimiter { @Override public long refill(long limit) { - final long now = EnvironmentEdgeManager.currentTime(); + final long now = EnvironmentEdgeManager.monotonicTime(); if (now < nextRefillTime) { return 0; } @@ -38,7 +38,7 @@ public class FixedIntervalRateLimiter extends RateLimiter { if (nextRefillTime == -1) { return 0; } - final long now = EnvironmentEdgeManager.currentTime(); + final long now = EnvironmentEdgeManager.monotonicTime(); final long refillTime = nextRefillTime; return refillTime - now; } diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/util/ManualEnvironmentEdge.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/util/ManualEnvironmentEdge.java index e5081273d4..dec2d57048 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/util/ManualEnvironmentEdge.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/util/ManualEnvironmentEdge.java @@ -40,6 +40,11 @@ public class ManualEnvironmentEdge implements EnvironmentEdge { @Override public long currentTime() { - return this.value; + return value; + } + + @Override + public long monotonicTime() { + return value; } } diff --git a/hbase-server/src/test/java/org/apache/hadoop/hbase/quotas/TestRateLimiter.java b/hbase-server/src/test/java/org/apache/hadoop/hbase/quotas/TestRateLimiter.java index 96dc9902b6..8447c1ca4e 100644 --- a/hbase-server/src/test/java/org/apache/hadoop/hbase/quotas/TestRateLimiter.java +++ b/hbase-server/src/test/java/org/apache/hadoop/hbase/quotas/TestRateLimiter.java @@ -130,10 +130,10 @@ public class TestRateLimiter { // fix the current time in order to get the precise value of interval EnvironmentEdge edge = new EnvironmentEdge() { - private final long ts = System.currentTimeMillis(); + private final long ts = TimeUnit.NANOSECONDS.toMillis(System.nanoTime()); @Override - public long currentTime() { + public long monotonicTime() { return ts; } }; @@ -195,22 +195,22 @@ public class TestRateLimiter { RateLimiter limiter = new AverageIntervalRateLimiter(); // when set limit is 100 per sec, this AverageIntervalRateLimiter will support at max 200 per sec limiter.set(100, TimeUnit.SECONDS); - limiter.setNextRefillTime(EnvironmentEdgeManager.currentTime()); + limiter.setNextRefillTime(EnvironmentEdgeManager.monotonicTime()); assertEquals(50, testCanExecuteByRate(limiter, 50)); // refill the avail to limit limiter.set(100, TimeUnit.SECONDS); - limiter.setNextRefillTime(EnvironmentEdgeManager.currentTime()); + limiter.setNextRefillTime(EnvironmentEdgeManager.monotonicTime()); assertEquals(100, testCanExecuteByRate(limiter, 100)); // refill the avail to limit limiter.set(100, TimeUnit.SECONDS); - limiter.setNextRefillTime(EnvironmentEdgeManager.currentTime()); + limiter.setNextRefillTime(EnvironmentEdgeManager.monotonicTime()); assertEquals(200, testCanExecuteByRate(limiter, 200)); // refill the avail to limit limiter.set(100, TimeUnit.SECONDS); - limiter.setNextRefillTime(EnvironmentEdgeManager.currentTime()); + limiter.setNextRefillTime(EnvironmentEdgeManager.monotonicTime()); assertEquals(200, testCanExecuteByRate(limiter, 500)); } @@ -219,17 +219,17 @@ public class TestRateLimiter { RateLimiter limiter = new FixedIntervalRateLimiter(); // when set limit is 100 per sec, this FixedIntervalRateLimiter will support at max 100 per sec limiter.set(100, TimeUnit.SECONDS); - limiter.setNextRefillTime(EnvironmentEdgeManager.currentTime()); + limiter.setNextRefillTime(EnvironmentEdgeManager.monotonicTime()); assertEquals(50, testCanExecuteByRate(limiter, 50)); // refill the avail to limit limiter.set(100, TimeUnit.SECONDS); - limiter.setNextRefillTime(EnvironmentEdgeManager.currentTime()); + limiter.setNextRefillTime(EnvironmentEdgeManager.monotonicTime()); assertEquals(100, testCanExecuteByRate(limiter, 100)); // refill the avail to limit limiter.set(100, TimeUnit.SECONDS); - limiter.setNextRefillTime(EnvironmentEdgeManager.currentTime()); + limiter.setNextRefillTime(EnvironmentEdgeManager.monotonicTime()); assertEquals(100, testCanExecuteByRate(limiter, 200)); } diff --git a/hbase-server/src/test/java/org/apache/hadoop/hbase/util/TestDefaultEnvironmentEdge.java b/hbase-server/src/test/java/org/apache/hadoop/hbase/util/TestDefaultEnvironmentEdge.java index 4d2c893970..3ccdde9ea8 100644 --- a/hbase-server/src/test/java/org/apache/hadoop/hbase/util/TestDefaultEnvironmentEdge.java +++ b/hbase-server/src/test/java/org/apache/hadoop/hbase/util/TestDefaultEnvironmentEdge.java @@ -39,19 +39,16 @@ public class TestDefaultEnvironmentEdge { HBaseClassTestRule.forClass(TestDefaultEnvironmentEdge.class); @Test - public void testGetCurrentTimeUsesSystemClock() { + public void monotonicTimeIsMonotonic() { DefaultEnvironmentEdge edge = new DefaultEnvironmentEdge(); - long systemTime = System.currentTimeMillis(); - long edgeTime = edge.currentTime(); - assertTrue("System time must be either the same or less than the edge time", - systemTime < edgeTime || systemTime == edgeTime); + long startTime = edge.monotonicTime(); try { Thread.sleep(1); } catch (InterruptedException e) { fail(e.getMessage()); } - long secondEdgeTime = edge.currentTime(); + long endTime = edge.currentTime(); assertTrue("Second time must be greater than the first", - secondEdgeTime > edgeTime); + endTime > startTime); } } -- 2.17.1