From c52f4b286cbc4332b2accabb83916be268de84b9 Mon Sep 17 00:00:00 2001 From: Bryan Beaudreault Date: Wed, 29 Mar 2023 16:19:13 -0400 Subject: [PATCH] HBASE-27704 Quotas can drastically overflow configured limit (#5099) Signed-off-by: Nick Dimiduk --- .../quotas/FixedIntervalRateLimiter.java | 18 ++++++++- .../hadoop/hbase/quotas/RateLimiter.java | 7 +--- .../hadoop/hbase/quotas/TestRateLimiter.java | 38 +++++++++---------- 3 files changed, 37 insertions(+), 26 deletions(-) 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 0de584eaeab..a717305b8c0 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 @@ -46,7 +46,23 @@ public class FixedIntervalRateLimiter extends RateLimiter { } final long now = EnvironmentEdgeManager.currentTime(); final long refillTime = nextRefillTime; - return refillTime - now; + long diff = amount - available; + // We will add limit at next interval. If diff is less than that limit, the wait interval + // is just time between now and then. + long nextRefillInterval = refillTime - now; + if (diff <= limit) { + return nextRefillInterval; + } + + // Otherwise, we need to figure out how many refills are needed. + // There will be one at nextRefillInterval, and then some number of extra refills. + // Division will round down if not even, so we can just add that to our next interval + long extraRefillsNecessary = diff / limit; + // If it's even, subtract one since that will be covered by nextRefillInterval + if (diff % limit == 0) { + extraRefillsNecessary--; + } + return nextRefillInterval + (extraRefillsNecessary * super.getTimeUnitInMillis()); } // This method is for strictly testing purpose only diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/quotas/RateLimiter.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/quotas/RateLimiter.java index b83e45e9407..bda60ffa690 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/quotas/RateLimiter.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/quotas/RateLimiter.java @@ -157,9 +157,9 @@ public abstract class RateLimiter { } // check for positive overflow if (avail <= Long.MAX_VALUE - refillAmount) { - avail = Math.max(0, Math.min(avail + refillAmount, limit)); + avail = Math.min(avail + refillAmount, limit); } else { - avail = Math.max(0, limit); + avail = limit; } if (avail >= amount) { return true; @@ -186,9 +186,6 @@ public abstract class RateLimiter { if (amount >= 0) { this.avail -= amount; - if (this.avail < 0) { - this.avail = 0; - } } else { if (this.avail <= Long.MAX_VALUE + amount) { this.avail -= amount; 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 307ede2c815..49df937f7c5 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 @@ -109,17 +109,17 @@ public class TestRateLimiter { // Verify that we have to wait at least 1.1sec to have 1 resource available assertTrue(limiter.canExecute()); limiter.consume(20); - // To consume 1 resource wait for 100ms - assertEquals(100, limiter.waitInterval(1)); - // To consume 10 resource wait for 1000ms - assertEquals(1000, limiter.waitInterval(10)); + // We consumed twice the quota. Need to wait 1s to get back to 0, then another 100ms for the 1 + assertEquals(1100, limiter.waitInterval(1)); + // We consumed twice the quota. Need to wait 1s to get back to 0, then another 1s to get to 10 + assertEquals(2000, limiter.waitInterval(10)); - limiter.setNextRefillTime(limiter.getNextRefillTime() - 900); - // Verify that after 1sec the 1 resource is available - assertTrue(limiter.canExecute(1)); + // Verify that after 1sec we need to wait for another 0.1sec to get a resource available + limiter.setNextRefillTime(limiter.getNextRefillTime() - 1000); + assertFalse(limiter.canExecute(1)); limiter.setNextRefillTime(limiter.getNextRefillTime() - 100); - // Verify that after 1sec the 10 resource is available - assertTrue(limiter.canExecute()); + // We've waited the full 1.1sec, should now have 1 available + assertTrue(limiter.canExecute(1)); assertEquals(0, limiter.waitInterval()); } @@ -138,22 +138,20 @@ public class TestRateLimiter { } }; EnvironmentEdgeManager.injectEdge(edge); - // 10 resources are available, but we need to consume 20 resources - // Verify that we have to wait at least 1.1sec to have 1 resource available assertTrue(limiter.canExecute()); + // 10 resources are available, but we need to consume 20 resources limiter.consume(20); - // To consume 1 resource also wait for 1000ms - assertEquals(1000, limiter.waitInterval(1)); - // To consume 10 resource wait for 100ms - assertEquals(1000, limiter.waitInterval(10)); + // We over-consumed by 10. Since this is a fixed interval refill, where + // each interval we refill the full limit amount, we need to wait 2 intervals -- first + // interval gets us from -10 to 0, second gets us from 0 to 10, though we just want the 1. + assertEquals(2000, limiter.waitInterval(1)); EnvironmentEdgeManager.reset(); - limiter.setNextRefillTime(limiter.getNextRefillTime() - 900); // Verify that after 1sec also no resource should be available - assertFalse(limiter.canExecute(1)); - limiter.setNextRefillTime(limiter.getNextRefillTime() - 100); - - // Verify that after 1sec the 10 resource is available + limiter.setNextRefillTime(limiter.getNextRefillTime() - 1000); + assertFalse(limiter.canExecute()); + // Verify that after total 2sec the 10 resource is available + limiter.setNextRefillTime(limiter.getNextRefillTime() - 1000); assertTrue(limiter.canExecute()); assertEquals(0, limiter.waitInterval()); }