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

eolivelli pushed a commit to branch branch-3.6
in repository https://gitbox.apache.org/repos/asf/zookeeper.git


The following commit(s) were added to refs/heads/branch-3.6 by this push:
     new e2aef19  ZOOKEEPER-3793: Request throttling is broken when 
RequestThrottler is disabled or configured incorrectly..
e2aef19 is described below

commit e2aef19f4be4cd8015b66dbf63bb860efe4745f4
Author: Michael Han <h...@apache.org>
AuthorDate: Sun Apr 12 11:14:45 2020 +0200

    ZOOKEEPER-3793: Request throttling is broken when RequestThrottler is 
disabled or configured incorrectly..
    
    When RequestThrottler is not enabled or is enabled but configured 
incorrectly, ZooKeeper server will stop throttling. This is a serious bug as 
without request throttling, it's fairly easy to overwhelm ZooKeeper which leads 
to all sorts of issues.
    
    This is a regression introduced in ZOOKEEPER-3243, where the total number 
of queued requests in request processing pipeline is not taking into 
consideration when deciding whether to throttle or not, or only taken into 
consideration conditionally based on RequestThrottler's configurations. We 
should make sure always taking into account the number of queued requests in 
request processing pipeline before making throttling decisions.
    
    Author: Michael Han <h...@apache.org>
    
    Reviewers: Enrico Olivelli <eolive...@apache.org>
    
    Closes #1316 from hanm/ZOOKEEPER-3793
    
    (cherry picked from commit 4d32f6cf39f76d606b436bd4c04a8d1bc9c60148)
    Signed-off-by: Enrico Olivelli <eolive...@apache.org>
---
 .../apache/zookeeper/server/ZooKeeperServer.java   |  3 +-
 .../zookeeper/server/RequestThrottlerTest.java     | 39 ++++++++++++++++++++++
 2 files changed, 41 insertions(+), 1 deletion(-)

diff --git 
a/zookeeper-server/src/main/java/org/apache/zookeeper/server/ZooKeeperServer.java
 
b/zookeeper-server/src/main/java/org/apache/zookeeper/server/ZooKeeperServer.java
index 1a2d9a7..2594b15 100644
--- 
a/zookeeper-server/src/main/java/org/apache/zookeeper/server/ZooKeeperServer.java
+++ 
b/zookeeper-server/src/main/java/org/apache/zookeeper/server/ZooKeeperServer.java
@@ -1419,7 +1419,8 @@ public class ZooKeeperServer implements SessionExpirer, 
ServerStats.Provider {
     }
 
     public boolean shouldThrottle(long outStandingCount) {
-        if (getGlobalOutstandingLimit() < getInflight()) {
+        int globalOutstandingLimit = getGlobalOutstandingLimit();
+        if (globalOutstandingLimit < getInflight() || globalOutstandingLimit < 
getInProcess()) {
             return outStandingCount > 0;
         }
         return false;
diff --git 
a/zookeeper-server/src/test/java/org/apache/zookeeper/server/RequestThrottlerTest.java
 
b/zookeeper-server/src/test/java/org/apache/zookeeper/server/RequestThrottlerTest.java
index 3afe81c..3e1de55 100644
--- 
a/zookeeper-server/src/test/java/org/apache/zookeeper/server/RequestThrottlerTest.java
+++ 
b/zookeeper-server/src/test/java/org/apache/zookeeper/server/RequestThrottlerTest.java
@@ -47,6 +47,7 @@ public class RequestThrottlerTest extends ZKTestCase {
     private static final Logger LOG = 
LoggerFactory.getLogger(RequestThrottlerTest.class);
 
     private static String HOSTPORT = "127.0.0.1:" + PortAssignment.unique();
+    private static String GLOBAL_OUTSTANDING_LIMIT = "1";
     private static final int TOTAL_REQUESTS = 5;
     private static final int STALL_TIME = 5000;
 
@@ -307,4 +308,42 @@ public class RequestThrottlerTest extends ZKTestCase {
         metrics = MetricsUtils.currentServerMetrics();
         Assert.assertEquals(2, (long) metrics.get("stale_replies"));
     }
+
+    @Test
+    public void 
testGlobalOutstandingRequestThrottlingWithRequestThrottlerDisabled() throws 
Exception {
+        try {
+            System.setProperty(ZooKeeperServer.GLOBAL_OUTSTANDING_LIMIT, 
GLOBAL_OUTSTANDING_LIMIT);
+
+            ServerMetrics.getMetrics().resetAll();
+
+            // Here we disable RequestThrottler and let incoming requests 
queued at first request processor.
+            RequestThrottler.setMaxRequests(0);
+            resumeProcess = new CountDownLatch(1);
+            int totalRequests = 10;
+            submitted = new CountDownLatch(totalRequests);
+
+            for (int i = 0; i < totalRequests; i++) {
+                zk.create("/request_throttle_test- " + i, 
("/request_throttle_test- "
+                        + i).getBytes(), ZooDefs.Ids.OPEN_ACL_UNSAFE, 
CreateMode.PERSISTENT, (rc, path, ctx, name) -> {
+                }, null);
+            }
+
+            submitted.await(5, TimeUnit.SECONDS);
+
+            resumeProcess.countDown();
+
+            // We should start throttling instead of queuing more requests.
+            //
+            // We always allow up to GLOBAL_OUTSTANDING_LIMIT + 1 number of 
requests coming in request processing pipeline
+            // before throttling. For the next request, we will throttle by 
disabling receiving future requests but we still
+            // allow this single request coming in. So the total number of 
queued requests in processing pipeline would
+            // be GLOBAL_OUTSTANDING_LIMIT + 2.
+            assertEquals(Integer.parseInt(GLOBAL_OUTSTANDING_LIMIT) + 2,
+                    (long) 
MetricsUtils.currentServerMetrics().get("prep_processor_request_queued"));
+        } catch (Exception e) {
+            throw e;
+        } finally {
+            System.clearProperty(ZooKeeperServer.GLOBAL_OUTSTANDING_LIMIT);
+        }
+    }
 }

Reply via email to