You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@hbase.apache.org by bb...@apache.org on 2023/03/30 13:13:07 UTC
[hbase] branch branch-2 updated: HBASE-27704 Quotas can drastically overflow configured limit (#5153)
This is an automated email from the ASF dual-hosted git repository.
bbeaudreault pushed a commit to branch branch-2
in repository https://gitbox.apache.org/repos/asf/hbase.git
The following commit(s) were added to refs/heads/branch-2 by this push:
new 9f4b31e6190 HBASE-27704 Quotas can drastically overflow configured limit (#5153)
9f4b31e6190 is described below
commit 9f4b31e619067cba9e00c7c9cc36cddb434058a3
Author: Bryan Beaudreault <bb...@apache.org>
AuthorDate: Thu Mar 30 09:13:00 2023 -0400
HBASE-27704 Quotas can drastically overflow configured limit (#5153)
Signed-off-by: Nick Dimiduk <nd...@apache.org>
---
.../hbase/quotas/FixedIntervalRateLimiter.java | 18 +++++++++-
.../apache/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());
}