ZOOKEEPER-3503: Add server side large request throttling#1051
Closed
jhuan31 wants to merge 2 commits intoapache:masterfrom
Closed
ZOOKEEPER-3503: Add server side large request throttling#1051jhuan31 wants to merge 2 commits intoapache:masterfrom
jhuan31 wants to merge 2 commits intoapache:masterfrom
Conversation
jhuan31
force-pushed
the
ZOOKEEPER-3503
branch
2 times, most recently
from
aa972bf to
b854c35
Compare
hanm
requested changes
Aug 12, 2019
zookeeper-server/src/main/java/org/apache/zookeeper/server/ZooKeeperServer.java
Outdated
Show resolved
Hide resolved
| return length > largeRequestThreshold; | ||
| } | ||
|
|
||
| private boolean testRequestSize(int length, boolean increment) { |
Contributor
There was a problem hiding this comment.
having a boolean parameter in a function seems an anti pattern to me.
Is there a case when we don't want increment?
Author
There was a problem hiding this comment.
We check while receiving the packet and after we receive the whole packet. We increment only after we receive the whole packet. I refactor the code. Let me know whether it is easier to read.
Contributor
There was a problem hiding this comment.
yes, i think although the new code is a little bit verbose it's easier to understand.
Contributor
There was a problem hiding this comment.
One nit - we could use void return type instead of boolean as these check function will never return false. Though, current form is ok to me too.
| * The size threshold after which a request is considered a large request | ||
| * and is checked against the large request byte limit. | ||
| */ | ||
| private volatile int largeRequestThreshold = |
Contributor
There was a problem hiding this comment.
I think it's necessary to do a validation of the values on these two newly added property, and throw argument exceptions if they are off the chart (and update doc to reflect the valid ranges.).
Author
There was a problem hiding this comment.
I add some basic checking and printing the current settings to the log. It's really hard to say what the valid ranges should be. Also I feel throwing an exception is a little too harsh. I just disable the large request throttling if the values are wrong. What do you think?
Contributor
There was a problem hiding this comment.
looks good to me, as this feature is not about correctness so even if it's misconfigured it's ok to have the cluster running with warning messages printed in log.
jhuan31
changed the title
hanm
approved these changes
Aug 14, 2019
nkalmar
reviewed
Aug 23, 2019
zookeeper-server/src/main/java/org/apache/zookeeper/server/ZooKeeperServer.java
Outdated
Show resolved
Hide resolved
jhuan31
force-pushed
the
ZOOKEEPER-3503
branch
2 times, most recently
from
ddf2bae to
5045558
Compare
Contributor
|
this is ready to land. just needs a rebase to resolve the documentation conflict. @jhuan31 |
jhuan31
force-pushed
the
ZOOKEEPER-3503
branch
from
5045558 to
5f44a45
Compare
Author
|
Thank you for the reminder @hanm ! Just rebased |
hanm
approved these changes
Sep 16, 2019
stickyhipp
pushed a commit
to stickyhipp/zookeeper
that referenced
this pull request
Aug 19, 2020
With this change, a ZooKeeper server has two new settings: zookeeper.largeRequestThreshold zookeeper.largeRequestMaxBytes Any request that is larger than largeRequestThreshold is considered a large request, and will only be allowed if the number of bytes associated with inflight large requests is currently below the largeRequestMaxBytes limit. This check is performed in the connection layer based on the length header of a request, before allocating the necessary byte buffer and reading data off the TCP socket. This ensures the limit is enforced before allocating data that's ultimately just going to discarded. Whenever a large request is allowed, its size is added to an atomic counter which represents the number of large request bytes inflight and this counter is the one tested against the max. Whenever a large request is completed or dropped, the counter is decremented as necessary. Author: Jie Huang <jiehuang@fb.com> Author: Joseph Blomstedt <jdb@fb.com> Reviewers: Michael Han <hanm@apache.org>, Norbert Kalmar <nkalmar@apache.org> Closes apache#1051 from jhuan31/ZOOKEEPER-3503
RokLenarcic
pushed a commit
to RokLenarcic/zookeeper
that referenced
this pull request
Aug 31, 2022
With this change, a ZooKeeper server has two new settings: zookeeper.largeRequestThreshold zookeeper.largeRequestMaxBytes Any request that is larger than largeRequestThreshold is considered a large request, and will only be allowed if the number of bytes associated with inflight large requests is currently below the largeRequestMaxBytes limit. This check is performed in the connection layer based on the length header of a request, before allocating the necessary byte buffer and reading data off the TCP socket. This ensures the limit is enforced before allocating data that's ultimately just going to discarded. Whenever a large request is allowed, its size is added to an atomic counter which represents the number of large request bytes inflight and this counter is the one tested against the max. Whenever a large request is completed or dropped, the counter is decremented as necessary. Author: Jie Huang <jiehuang@fb.com> Author: Joseph Blomstedt <jdb@fb.com> Reviewers: Michael Han <hanm@apache.org>, Norbert Kalmar <nkalmar@apache.org> Closes apache#1051 from jhuan31/ZOOKEEPER-3503
RokLenarcic
pushed a commit
to RokLenarcic/zookeeper
that referenced
this pull request
Sep 3, 2022
With this change, a ZooKeeper server has two new settings: zookeeper.largeRequestThreshold zookeeper.largeRequestMaxBytes Any request that is larger than largeRequestThreshold is considered a large request, and will only be allowed if the number of bytes associated with inflight large requests is currently below the largeRequestMaxBytes limit. This check is performed in the connection layer based on the length header of a request, before allocating the necessary byte buffer and reading data off the TCP socket. This ensures the limit is enforced before allocating data that's ultimately just going to discarded. Whenever a large request is allowed, its size is added to an atomic counter which represents the number of large request bytes inflight and this counter is the one tested against the max. Whenever a large request is completed or dropped, the counter is decremented as necessary. Author: Jie Huang <jiehuang@fb.com> Author: Joseph Blomstedt <jdb@fb.com> Reviewers: Michael Han <hanm@apache.org>, Norbert Kalmar <nkalmar@apache.org> Closes apache#1051 from jhuan31/ZOOKEEPER-3503
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
With this change, a ZooKeeper server has two new settings:
zookeeper.largeRequestThreshold
zookeeper.largeRequestMaxBytes
Any request that is larger than largeRequestThreshold is considered a large request, and will only be allowed if the number of bytes associated with inflight large requests is currently below the largeRequestMaxBytes limit.
This check is performed in the connection layer based on the length header of a request, before allocating the necessary byte buffer and reading data off the TCP socket. This ensures the limit is enforced before allocating data that's ultimately just going to discarded.
Whenever a large request is allowed, its size is added to an atomic counter which represents the number of large request bytes inflight and this counter is the one tested against the max. Whenever a large request is completed or dropped, the counter is decremented as necessary.