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 <hanm@apache.org>

Reviewers: Enrico Olivelli <eolivelli@apache.org>

Closes #1316 from hanm/ZOOKEEPER-3793

zookeeper-server/src/main/java/org/apache/zookeeper/server/ZooKeeperServer.java

@@ -1431,7 +1431,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;

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);
+        }
+    }
 }