[incubator-gobblin] branch master updated: [GOBBLIN-752] Fix a bug in QPS throttling policy where it was incorrectly indicating permits were impossible to satisfy.

[ib...@apache.org]

This is an automated email from the ASF dual-hosted git repository.

ibuenros pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/incubator-gobblin.git


The following commit(s) were added to refs/heads/master by this push:
     new 19eab6a  [GOBBLIN-752] Fix a bug in QPS throttling policy where it was incorrectly indicating permits were impossible to satisfy.
19eab6a is described below

commit 19eab6a48ffb459d85af82b4aa199dbf04449c87
Author: ibuenros <is...@gmail.com>
AuthorDate: Tue Apr 23 17:26:47 2019 -0700

    [GOBBLIN-752] Fix a bug in QPS throttling policy where it was incorrectly indicating permits were impossible to satisfy.
    
    Closes #2617 from ibuenros/limiter-logging
---
 .../gobblin/util/limiter/BatchedPermitsRequester.java    |  2 +-
 .../org/apache/gobblin/restli/throttling/QPSPolicy.java  | 16 +++++++++++++---
 2 files changed, 14 insertions(+), 4 deletions(-)

diff --git a/gobblin-restli/gobblin-throttling-service/gobblin-throttling-service-client/src/main/java/org/apache/gobblin/util/limiter/BatchedPermitsRequester.java b/gobblin-restli/gobblin-throttling-service/gobblin-throttling-service-client/src/main/java/org/apache/gobblin/util/limiter/BatchedPermitsRequester.java
index c5c5460..ba099a0 100644
--- a/gobblin-restli/gobblin-throttling-service/gobblin-throttling-service-client/src/main/java/org/apache/gobblin/util/limiter/BatchedPermitsRequester.java
+++ b/gobblin-restli/gobblin-throttling-service/gobblin-throttling-service-client/src/main/java/org/apache/gobblin/util/limiter/BatchedPermitsRequester.java
@@ -154,7 +154,7 @@ class BatchedPermitsRequester {
     this.lock.lock();
     try {
       while (true) {
-        if (permits > this.knownUnsatisfiablePermits) {
+        if (permits >= this.knownUnsatisfiablePermits) {
           // We are requesting more permits than the remote policy will ever be able to satisfy, return immediately with no permits
           break;
         }
diff --git a/gobblin-restli/gobblin-throttling-service/gobblin-throttling-service-server/src/main/java/org/apache/gobblin/restli/throttling/QPSPolicy.java b/gobblin-restli/gobblin-throttling-service/gobblin-throttling-service-server/src/main/java/org/apache/gobblin/restli/throttling/QPSPolicy.java
index be1ecb8..f94dab6 100644
--- a/gobblin-restli/gobblin-throttling-service/gobblin-throttling-service-server/src/main/java/org/apache/gobblin/restli/throttling/QPSPolicy.java
+++ b/gobblin-restli/gobblin-throttling-service/gobblin-throttling-service-server/src/main/java/org/apache/gobblin/restli/throttling/QPSPolicy.java
@@ -43,6 +43,10 @@ public class QPSPolicy implements ThrottlingPolicy {
 
   public static final String FACTORY_ALIAS = "qps";
 
+  /** Max time we will ask clients to wait for a new allocation. By default set to the Rest.li timeout because
+   * that is how we determine whether permists can be granted. */
+  public static final long MAX_WAIT_MILLIS = LimiterServerResource.TIMEOUT_MILLIS;
+
   /**
    * The qps the policy should enforce.
    */
@@ -98,9 +102,15 @@ public class QPSPolicy implements ThrottlingPolicy {
     allocation.setPermits(permitsGranted.getPermits());
     allocation.setExpiration(Long.MAX_VALUE);
     allocation.setWaitForPermitUseMillis(permitsGranted.getDelay());
-    allocation.setUnsatisfiablePermits(request.getMinPermits(GetMode.DEFAULT));
-    if (permitsGranted.getPermits() <= 0) {
-      allocation.setMinRetryDelayMillis(LimiterServerResource.TIMEOUT_MILLIS);
+    if (!permitsGranted.isPossibleToSatisfy()) {
+      allocation.setUnsatisfiablePermits(request.getMinPermits(GetMode.DEFAULT));
+    }
+    if (permitsRequested > 0) {
+      // This heuristic asks clients to wait before making any requests
+      // for an amount of time based on the percentage of their requested permits that were granted
+      double fractionGranted = Math.min(1.0, ((double) permitsGranted.getPermits() / permitsRequested));
+      double wait = MAX_WAIT_MILLIS * (1 - fractionGranted);
+      allocation.setMinRetryDelayMillis((long) wait);
     }
     return allocation;
   }