ZOOKEEPER-3243: Add server-side request throttling #986

jhuan31 · 2019-06-12T22:59:06Z

No description provided.

jhuan31 · 2019-06-13T19:01:22Z

close for now. will refactor and have a new PR

asfgit · 2019-06-14T16:04:19Z

FAILURE

--none--

eolivelli · 2019-06-14T16:37:35Z

That 'FAILURE' maybe related to the fact that I have disable ant job for master branch

jhuan31 · 2019-06-14T18:20:41Z

That 'FAILURE' maybe related to the fact that I have disable ant job for master branch

It actually ran

jhuan31 · 2019-06-15T15:45:14Z

retest ant build

jhuan31 · 2019-06-15T21:36:45Z

retest ant build

hanm

This is a very much needed feature for running ZK at scale, thanks for putting this up. Did first pass review, left some comments.

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

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

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

hanm · 2019-06-22T04:09:59Z

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

+                    }
+                }
+
+                if (killed) {


this could be elevated to the top of the first while block, so we can break early and start draining the queue sooner. consider one case - we have a big queue of submitted requests and most of them must be dropped. If we leave the check here, we'll not break the loop until processing the last request (the request of death.).

The check for killed is also in the inner while loop for each request in the queue.
So it won't delay the shutdown.

Assume while we are shutting down (kill is set to true), we have a large queue of requests, where each request is stale and must be dropped. With current code, the outer most while (true) { loop will try loop through every request in the queue, because L145 if (request.mustDrop()) { is evaluated as true for every request (which is stale). In this case, the check for killed in the inner while loop will not get executed. This was the reason I was suggesting to move the if (killed) { check before the if (request.mustDrop()) {.

Does this example make sense? Albeit this example somewhat hypothetical, having if (killed) { check earlier sounds no harm either.

Yes, get it! Thank you for your detailed explanation! Fix on the way... Actually I decide to add a check at the beginning of the outer loop instead of moving the existing check. If we remove the check after the inner while, then the throttler will submit one last request when killed is set. What do you think?

@jhuan31 agree, add instead of moving is better.

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

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

jhuan31 · 2019-07-01T22:11:59Z

retest maven build

hanm

looks great, thanks for update patch @jhuan31

There are two remaining issues I'd like to further discuss, please take a look.

hanm · 2019-07-02T17:04:07Z

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

+                    }
+                }
+
+                if (killed) {


Assume while we are shutting down (kill is set to true), we have a large queue of requests, where each request is stale and must be dropped. With current code, the outer most while (true) { loop will try loop through every request in the queue, because L145 if (request.mustDrop()) { is evaluated as true for every request (which is stale). In this case, the check for killed in the inner while loop will not get executed. This was the reason I was suggesting to move the if (killed) { check before the if (request.mustDrop()) {.

Does this example make sense? Albeit this example somewhat hypothetical, having if (killed) { check earlier sounds no harm either.

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

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

hanm

Now I only one remaining question (none blocking though). lgtm +1.

hanm · 2019-07-08T17:19:48Z

retest this please

hanm

Thanks for the update @jhuan31. The patch looks great to me.

Just one final thing to resolve before I can merge this: there is one unit test testAddSessionAfterSessionExpiry consistently failing on CI and locally. Would you please check this out and fix it? The test log output for reference:

2019-07-08 17:30:51,517 [myid:] - INFO [Time-limited test:JUnit4ZKTestRunner$LoggedInvokeMethod@99] - TEST METHOD FAILED testAddSessionAfterSessionExpiry java.lang.AssertionError: Duplicate session expiry request has been generated expected:<1> but was:<0> at org.junit.Assert.fail(Assert.java:88) at org.junit.Assert.failNotEquals(Assert.java:834) at org.junit.Assert.assertEquals(Assert.java:645) at org.apache.zookeeper.server.SessionTrackerTest.testAddSessionAfterSessionExpiry(SessionTrackerTest.java:83) at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method) at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62) at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43) at java.lang.reflect.Method.invoke(Method.java:498) at org.junit.runners.model.FrameworkMethod$1.runReflectiveCall(FrameworkMethod.java:50) at org.junit.internal.runners.model.ReflectiveCallable.run(ReflectiveCallable.java:12) at org.junit.runners.model.FrameworkMethod.invokeExplosively(FrameworkMethod.java:47) at org.junit.internal.runners.statements.InvokeMethod.evaluate(InvokeMethod.java:17) at org.apache.zookeeper.JUnit4ZKTestRunner$LoggedInvokeMethod.evaluate(JUnit4ZKTestRunner.java:80) at org.junit.internal.runners.statements.FailOnTimeout$CallableStatement.call(FailOnTimeout.java:298) at org.junit.internal.runners.statements.FailOnTimeout$CallableStatement.call(FailOnTimeout.java:292) at java.util.concurrent.FutureTask.run(FutureTask.java:266) at java.lang.Thread.run(Thread.java:748) 2019-07-08 17:30:51,521 [myid:] - INFO [main:ZKTestCase$1@75] - FAILED testAddSessionAfterSessionExpiry java.lang.AssertionError: Duplicate session expiry request has been generated expected:<1> but was:<0> at org.junit.Assert.fail(Assert.java:88) at org.junit.Assert.failNotEquals(Assert.java:834) at org.junit.Assert.assertEquals(Assert.java:645) at org.apache.zookeeper.server.SessionTrackerTest.testAddSessionAfterSessionExpiry(SessionTrackerTest.java:83) at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method) at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62) at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43) at java.lang.reflect.Method.invoke(Method.java:498) at org.junit.runners.model.FrameworkMethod$1.runReflectiveCall(FrameworkMethod.java:50) at org.junit.internal.runners.model.ReflectiveCallable.run(ReflectiveCallable.java:12) at org.junit.runners.model.FrameworkMethod.invokeExplosively(FrameworkMethod.java:47) at org.junit.internal.runners.statements.InvokeMethod.evaluate(InvokeMethod.java:17) at org.apache.zookeeper.JUnit4ZKTestRunner$LoggedInvokeMethod.evaluate(JUnit4ZKTestRunner.java:80) at org.junit.internal.runners.statements.FailOnTimeout$CallableStatement.call(FailOnTimeout.java:298) at org.junit.internal.runners.statements.FailOnTimeout$CallableStatement.call(FailOnTimeout.java:292) at java.util.concurrent.FutureTask.run(FutureTask.java:266) at java.lang.Thread.run(Thread.java:748) 2019-07-08 17:30:51,522 [myid:] - INFO [main:ZKTestCase$1@65] - FINISHED testAddSessionAfterSessionExpiry

jhuan31 · 2019-07-08T21:14:14Z

The broken unit test has been fixed :) It was broken because we removed that enable-request-throttle flag so need to update the test setup.

hanm · 2019-07-08T21:27:17Z

awesome, thanks for the fix @jhuan31. merged to master after all check is green.

Author: Jie Huang <jiehuang@fb.com> Author: Joseph Blomstedt <jdb@fb.com> Reviewers: Michael Han <hanm@apache.org> Closes apache#986 from jhuan31/ZOOKEEPER-3243

jhuan31 closed this Jun 13, 2019

jhuan31 reopened this Jun 14, 2019

hanm suggested changes Jun 22, 2019

View reviewed changes

hanm reviewed Jul 2, 2019

View reviewed changes

hanm approved these changes Jul 3, 2019

View reviewed changes

jtuple and others added 7 commits July 5, 2019 09:27

ZOOKEEPER-3243: Add server-side request throttling

93648cb

refactor

e74a608

Address spotbugs issues

9c33d0d

fix unit tests

10a3aaa

apache header

01b9798

address review comments

4d0e1f8

deflak unit test

e254c0a

jhuan31 force-pushed the ZOOKEEPER-3243 branch from a7a5b39 to e254c0a Compare July 5, 2019 16:39

fix a shutdown bug

9866f3f

hanm suggested changes Jul 8, 2019

View reviewed changes

fix unit test SessionTrackerTest.testAddSessionAfterSessionExpiry

d361a2d

asfgit closed this in 7b3de52 Jul 8, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ZOOKEEPER-3243: Add server-side request throttling #986

ZOOKEEPER-3243: Add server-side request throttling #986

jhuan31 commented Jun 12, 2019

jhuan31 commented Jun 13, 2019

asfgit commented Jun 14, 2019

eolivelli commented Jun 14, 2019

jhuan31 commented Jun 14, 2019

jhuan31 commented Jun 15, 2019

jhuan31 commented Jun 15, 2019

hanm left a comment

hanm Jun 22, 2019

jhuan31 Jul 1, 2019

hanm Jul 2, 2019

jhuan31 Jul 5, 2019 •

edited

hanm Jul 8, 2019

jhuan31 commented Jul 1, 2019

hanm left a comment

hanm Jul 2, 2019

hanm left a comment

hanm commented Jul 8, 2019

hanm left a comment

jhuan31 commented Jul 8, 2019

hanm commented Jul 8, 2019

ZOOKEEPER-3243: Add server-side request throttling #986

ZOOKEEPER-3243: Add server-side request throttling #986

Conversation

jhuan31 commented Jun 12, 2019

jhuan31 commented Jun 13, 2019

asfgit commented Jun 14, 2019

eolivelli commented Jun 14, 2019

jhuan31 commented Jun 14, 2019

jhuan31 commented Jun 15, 2019

jhuan31 commented Jun 15, 2019

hanm left a comment

Choose a reason for hiding this comment

hanm Jun 22, 2019

Choose a reason for hiding this comment

jhuan31 Jul 1, 2019

Choose a reason for hiding this comment

hanm Jul 2, 2019

Choose a reason for hiding this comment

jhuan31 Jul 5, 2019 • edited

Choose a reason for hiding this comment

hanm Jul 8, 2019

Choose a reason for hiding this comment

jhuan31 commented Jul 1, 2019

hanm left a comment

Choose a reason for hiding this comment

hanm Jul 2, 2019

Choose a reason for hiding this comment

hanm left a comment

Choose a reason for hiding this comment

hanm commented Jul 8, 2019

hanm left a comment

Choose a reason for hiding this comment

jhuan31 commented Jul 8, 2019

hanm commented Jul 8, 2019

jhuan31 Jul 5, 2019 •

edited