ZOOKEEPER-3242: Add server side connecting throttling #769
Conversation
jhuan31
force-pushed
the
ZOOKEEPER-3242
branch
from
ea3e751 to
69da78c
Compare
jhuan31
force-pushed
the
ZOOKEEPER-3242
branch
from
69da78c to
f6b2f85
Compare
|
added doc |
jhuan31
force-pushed
the
ZOOKEEPER-3242
branch
from
d811244 to
92725da
Compare
|
here comes the unit tests |
jhuan31
force-pushed
the
ZOOKEEPER-3242
branch
from
92725da to
5b8dd83
Compare
jhuan31
force-pushed
the
ZOOKEEPER-3242
branch
from
5b8dd83 to
a278504
Compare
| throttler.checkLimit(1); | ||
| Assert.assertEquals("All tokens should be used up by now", throttler.getMaxTokens(), throttler.getDeficit()); | ||
|
|
||
| Thread.sleep(110); |
There was a problem hiding this comment.
Have you tried to run this tests multiple times? 100x or 1000x ... ?
Tests with Thread.sleep are usually a smell of flaky tests, especially in Apache infra.
There was a problem hiding this comment.
I just ran the test 1000 times and it failed once. Yeah, the test is flaky. But I don't think it is because of the sleep. The fillTime and the freezeTime in the test are set to some unrealistic numbers so the test can tolerate some inaccuracy in timing. The test failed because the random number generator for random dropping gave 10 numbers less than 0.5 in a row, an event of of probability EXP(0.5, 10). I'm going to use a mock random generator, which I think is good enough for this unit test.
|
Ran the unit test 1000 times w/o failure |
| readResponseCache = new ResponseCache(); | ||
|
|
||
| connThrottle = new BlueThrottle(); |
There was a problem hiding this comment.
This configuration setting block could be a little bit neater by following the existing pattern in this class: create static constants for the configuration option name and the default value.
The existing config setting intBufferStartingSizeBytes is set via static constructor which is not convenient here, but you could overload the constructor of BlueThrottle to override the default settings. In which case you don't need these setters at all.
There was a problem hiding this comment.
Agree. Those setter invocations in the constructor look ugly. Moved them out. But we still need the setters for the JMX interface. And since we are keeping setters, I'd prefer to use them in the unit tests instead of using a constructor with a long list of parameters, i.e, I prefer to explicitly set the refill time to 100 instead of setting the third parameter of the constructor to 100.
| class BlueThrottleWithMockRandom extends BlueThrottle { | ||
| public BlueThrottleWithMockRandom() { | ||
| super(); | ||
| this.rng = new MockRandom(this); |
There was a problem hiding this comment.
You could have another overloaded constructor to inject custom random generator.
|
Merged to master branch. Thanks @jhuan31 ! |
| return true; | ||
| } | ||
|
|
||
| public synchronized boolean checkBlue(long now) { |
There was a problem hiding this comment.
IMO, method checkBlue should have a access level of private since it is only called inside this class.
Author: Jie Huang <[email protected]> Reviewers: [email protected], [email protected] Closes apache#769 from jhuan31/ZOOKEEPER-3242 and squashes the following commits: c3ec81f [Jie Huang] refactoring 86cad39 [Jie Huang] Use a mock random number generator to make the unit test flaky-proof a278504 [Jie Huang] Add unit tests for server-side connection throttling fd96650 [Jie Huang] update doc for server-side connection throttling 2f1ed0b [Jie Huang] Fix FindBugs Warnings a48b0fc [Jie Huang] ZOOKEEPER-3242: Add server side connecting throttling
No description provided.