HBASE-27704 Quotas can drastically overflow configured limit (#5153)

Signed-off-by: Nick Dimiduk <ndimiduk@apache.org>

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

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;

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
+    // We consumed twice the quota. Need to wait 1s to get back to 0, then another 100ms for the 1
-    assertEquals(100, limiter.waitInterval(1));
+    assertEquals(1100, limiter.waitInterval(1));
-    // To consume 10 resource wait for 1000ms
+    // We consumed twice the quota. Need to wait 1s to get back to 0, then another 1s to get to 10
-    assertEquals(1000, limiter.waitInterval(10));
+    assertEquals(2000, limiter.waitInterval(10));
 
-    limiter.setNextRefillTime(limiter.getNextRefillTime() - 900);
+    // Verify that after 1sec we need to wait for another 0.1sec to get a resource available
-    // Verify that after 1sec the 1 resource is available
+    limiter.setNextRefillTime(limiter.getNextRefillTime() - 1000);
-    assertTrue(limiter.canExecute(1));
+    assertFalse(limiter.canExecute(1));
     limiter.setNextRefillTime(limiter.getNextRefillTime() - 100);
-    // Verify that after 1sec the 10 resource is available
+    // We've waited the full 1.1sec, should now have 1 available
-    assertTrue(limiter.canExecute());
+    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
+    // We over-consumed by 10. Since this is a fixed interval refill, where
-    assertEquals(1000, limiter.waitInterval(1));
+    // each interval we refill the full limit amount, we need to wait 2 intervals -- first
-    // To consume 10 resource wait for 100ms
+    // interval gets us from -10 to 0, second gets us from 0 to 10, though we just want the 1.
-    assertEquals(1000, limiter.waitInterval(10));
+    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() - 1000);
-    limiter.setNextRefillTime(limiter.getNextRefillTime() - 100);
+    assertFalse(limiter.canExecute());
-
+    // Verify that after total 2sec the 10 resource is available
-    // Verify that after 1sec the 10 resource is available
+    limiter.setNextRefillTime(limiter.getNextRefillTime() - 1000);
     assertTrue(limiter.canExecute());
     assertEquals(0, limiter.waitInterval());
   }