You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@zookeeper.apache.org by GitBox <gi...@apache.org> on 2020/04/10 18:04:13 UTC
[GitHub] [zookeeper] hanm opened a new pull request #1316: ZOOKEEPER-3793:
Request throttling is broken when RequestThrottler is disabled or
configured incorrectly..
hanm opened a new pull request #1316: ZOOKEEPER-3793: Request throttling is broken when RequestThrottler is disabled or configured incorrectly..
URL: https://github.com/apache/zookeeper/pull/1316
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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [zookeeper] eolivelli commented on a change in pull request #1316:
ZOOKEEPER-3793: Request throttling is broken when RequestThrottler is
disabled or configured incorrectly..
Posted by GitBox <gi...@apache.org>.
eolivelli commented on a change in pull request #1316: ZOOKEEPER-3793: Request throttling is broken when RequestThrottler is disabled or configured incorrectly..
URL: https://github.com/apache/zookeeper/pull/1316#discussion_r407038889
##########
File path: zookeeper-server/src/test/java/org/apache/zookeeper/server/RequestThrottlerTest.java
##########
@@ -311,34 +311,38 @@ public void testLargeRequestThrottling() throws Exception {
@Test
public void testGlobalOutstandingRequestThrottlingWithRequestThrottlerDisabled() throws Exception {
- System.setProperty(ZooKeeperServer.GLOBAL_OUTSTANDING_LIMIT, GLOBAL_OUTSTANDING_LIMIT);
- ServerMetrics.getMetrics().resetAll();
+ try {
+ System.setProperty(ZooKeeperServer.GLOBAL_OUTSTANDING_LIMIT, GLOBAL_OUTSTANDING_LIMIT);
- // 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);
+ ServerMetrics.getMetrics().resetAll();
- 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);
- }
+ // 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);
- 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"));
+ 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);
+ }
- System.clearProperty(ZooKeeperServer.GLOBAL_OUTSTANDING_LIMIT);
+ 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) {
Review comment:
I can amend your commit @hanm if you do not have time.
I would like to prepare the RC0 of 3.6.1 today or tomorrow
sorry I missed this problem
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [zookeeper] eolivelli commented on a change in pull request #1316:
ZOOKEEPER-3793: Request throttling is broken when RequestThrottler is
disabled or configured incorrectly..
Posted by GitBox <gi...@apache.org>.
eolivelli commented on a change in pull request #1316: ZOOKEEPER-3793: Request throttling is broken when RequestThrottler is disabled or configured incorrectly..
URL: https://github.com/apache/zookeeper/pull/1316#discussion_r406879391
##########
File path: zookeeper-server/src/test/java/org/apache/zookeeper/server/RequestThrottlerTest.java
##########
@@ -307,4 +308,37 @@ public void testLargeRequestThrottling() throws Exception {
metrics = MetricsUtils.currentServerMetrics();
Assert.assertEquals(2, (long) metrics.get("stale_replies"));
}
+
+ @Test
+ public void testGlobalOutstandingRequestThrottlingWithRequestThrottlerDisabled() throws Exception {
+ 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"));
+
+ System.clearProperty(ZooKeeperServer.GLOBAL_OUTSTANDING_LIMIT);
Review comment:
It would be better to clear the property in a finally block
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [zookeeper] hanm commented on a change in pull request #1316:
ZOOKEEPER-3793: Request throttling is broken when RequestThrottler is
disabled or configured incorrectly..
Posted by GitBox <gi...@apache.org>.
hanm commented on a change in pull request #1316: ZOOKEEPER-3793: Request throttling is broken when RequestThrottler is disabled or configured incorrectly..
URL: https://github.com/apache/zookeeper/pull/1316#discussion_r406884310
##########
File path: zookeeper-server/src/test/java/org/apache/zookeeper/server/RequestThrottlerTest.java
##########
@@ -307,4 +308,37 @@ public void testLargeRequestThrottling() throws Exception {
metrics = MetricsUtils.currentServerMetrics();
Assert.assertEquals(2, (long) metrics.get("stale_replies"));
}
+
+ @Test
+ public void testGlobalOutstandingRequestThrottlingWithRequestThrottlerDisabled() throws Exception {
+ 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"));
+
+ System.clearProperty(ZooKeeperServer.GLOBAL_OUTSTANDING_LIMIT);
Review comment:
updated
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [zookeeper] eolivelli commented on a change in pull request #1316:
ZOOKEEPER-3793: Request throttling is broken when RequestThrottler is
disabled or configured incorrectly..
Posted by GitBox <gi...@apache.org>.
eolivelli commented on a change in pull request #1316: ZOOKEEPER-3793: Request throttling is broken when RequestThrottler is disabled or configured incorrectly..
URL: https://github.com/apache/zookeeper/pull/1316#discussion_r407038635
##########
File path: zookeeper-server/src/test/java/org/apache/zookeeper/server/RequestThrottlerTest.java
##########
@@ -311,34 +311,38 @@ public void testLargeRequestThrottling() throws Exception {
@Test
public void testGlobalOutstandingRequestThrottlingWithRequestThrottlerDisabled() throws Exception {
- System.setProperty(ZooKeeperServer.GLOBAL_OUTSTANDING_LIMIT, GLOBAL_OUTSTANDING_LIMIT);
- ServerMetrics.getMetrics().resetAll();
+ try {
+ System.setProperty(ZooKeeperServer.GLOBAL_OUTSTANDING_LIMIT, GLOBAL_OUTSTANDING_LIMIT);
- // 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);
+ ServerMetrics.getMetrics().resetAll();
- 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);
- }
+ // 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);
- 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"));
+ 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);
+ }
- System.clearProperty(ZooKeeperServer.GLOBAL_OUTSTANDING_LIMIT);
+ 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) {
Review comment:
ops, this is not good.
I missed this part.
if you catch every Exception you are making JUnit ignore errors
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [zookeeper] hanm commented on issue #1316: ZOOKEEPER-3793: Request
throttling is broken when RequestThrottler is disabled or configured
incorrectly..
Posted by GitBox <gi...@apache.org>.
hanm commented on issue #1316: ZOOKEEPER-3793: Request throttling is broken when RequestThrottler is disabled or configured incorrectly..
URL: https://github.com/apache/zookeeper/pull/1316#issuecomment-612149794
@jhuan31 do you mind review this?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [zookeeper] hanm commented on a change in pull request #1316:
ZOOKEEPER-3793: Request throttling is broken when RequestThrottler is
disabled or configured incorrectly..
Posted by GitBox <gi...@apache.org>.
hanm commented on a change in pull request #1316: ZOOKEEPER-3793: Request throttling is broken when RequestThrottler is disabled or configured incorrectly..
URL: https://github.com/apache/zookeeper/pull/1316#discussion_r407141337
##########
File path: zookeeper-server/src/test/java/org/apache/zookeeper/server/RequestThrottlerTest.java
##########
@@ -311,34 +311,38 @@ public void testLargeRequestThrottling() throws Exception {
@Test
public void testGlobalOutstandingRequestThrottlingWithRequestThrottlerDisabled() throws Exception {
- System.setProperty(ZooKeeperServer.GLOBAL_OUTSTANDING_LIMIT, GLOBAL_OUTSTANDING_LIMIT);
- ServerMetrics.getMetrics().resetAll();
+ try {
+ System.setProperty(ZooKeeperServer.GLOBAL_OUTSTANDING_LIMIT, GLOBAL_OUTSTANDING_LIMIT);
- // 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);
+ ServerMetrics.getMetrics().resetAll();
- 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);
- }
+ // 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);
- 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"));
+ 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);
+ }
- System.clearProperty(ZooKeeperServer.GLOBAL_OUTSTANDING_LIMIT);
+ 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) {
Review comment:
fixed by rethrowing.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [zookeeper] hanm edited a comment on issue #1316: ZOOKEEPER-3793:
Request throttling is broken when RequestThrottler is disabled or
configured incorrectly..
Posted by GitBox <gi...@apache.org>.
hanm edited a comment on issue #1316: ZOOKEEPER-3793: Request throttling is broken when RequestThrottler is disabled or configured incorrectly..
URL: https://github.com/apache/zookeeper/pull/1316#issuecomment-612149794
@jhuan31 do you mind review this?
also cc @lvfangmin
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [zookeeper] eolivelli closed pull request #1316: ZOOKEEPER-3793:
Request throttling is broken when RequestThrottler is disabled or
configured incorrectly..
Posted by GitBox <gi...@apache.org>.
eolivelli closed pull request #1316: ZOOKEEPER-3793: Request throttling is broken when RequestThrottler is disabled or configured incorrectly..
URL: https://github.com/apache/zookeeper/pull/1316
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services