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