New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
KAFKA-7730: Limit number of active connections per listener in brokers (KIP-402) #6034
KAFKA-7730: Limit number of active connections per listener in brokers (KIP-402) #6034
Conversation
f9abafe
to
e590b09
Compare
e590b09
to
0d0f7db
Compare
0d0f7db
to
0e15283
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm having trouble understanding the way the different connection limits work in relation to each other...
- if I exceeded the per-IP limit, we'll throw an exception, log an "info" and close the connection
- if I exceed the per-broker limit, we'll wait for an open slot, but on the next processor iteration, we'll close a connection and get the open slot, so this is a relatively short block.
- if I exceed the per-listener limit but not the per-broker limit, we'll wait for an open slot, but since the maxConnectionsExceeded method only checks the per-broker limit, we may potentially wait for quite a while until a client disconnects from our socket.
I'm not clear if the three different behaviors are intentional. Especially the third one which could be intentional but could also be a bug :)
Can you explain the behavior a bit? I've read the KIP, but I didn't find this detail.
[~gwenshap] The behaviours are intentional - not necessarily correct though :-)
|
Doesn't take per-listener limits into account. Doesn't it mean that if we have a limit on SSL listener but not on the broker, we'll never close excess connections on that listener? I feel like I'm missing something. |
|
The asymmetric behavior where we accept connections up to the listener limits and disconnect connections based on the broker limits (but done separately on each processor for each listener) makes the interactions really complex to reason about. If you have a broker limit, an internal (protected) listener and 2 external listeners (not totally unreasonable scenario), thats a lot of combinations to consider. I think I'm missing something about the reasoning for the different behaviors. |
@gwenshap Agree that connection limit handling is complex and the fact that we are handling different limits in different ways makes implementation and debugging more complex. Not sure if it makes usage more complex though since the limits serve different purposes. Listener limits require the user to understand the number of applications connecting to each listener and connections per-broker for those. While broker limits should be configured based on the total number of connections and total memory reserved for those connections (e.g. SSL buffers).
|
Thanks for the patient explanation. I think it is reasonable to want both high limit to protect the broker that is aggressively enforced, and also "QoS" limits to allocate connections to listeners and allow for something like "fast lane" listeners for important apps. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Code looks right, tests are awesome. Great work :)
Left a comment regarding the docs: I think the feature requires more explanation, especially on how broker and listener limits are different.
Feel free to merge after you add explanation, or wait for me to merge tomorrow morning :)
@@ -572,6 +574,8 @@ object KafkaConfig { | |||
val MaxConnectionsPerIpDoc = "The maximum number of connections we allow from each ip address. This can be set to 0 if there are overrides " + | |||
"configured using " + MaxConnectionsPerIpOverridesProp + " property" | |||
val MaxConnectionsPerIpOverridesDoc = "A comma-separated list of per-ip or hostname overrides to the default maximum number of connections. An example value is \"hostName:100,127.0.0.1:200\"" | |||
val MaxConnectionsDoc = "The maximum number of connections we allow for each listener at any time. This limit is applied in addition to any per-ip " + | |||
"limits configured using " + MaxConnectionsPerIpProp + ". The config name may be prefixed with the listener prefix to specify different limits for different listeners." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe add an explanation of how max.connections is enforced vs how listener.max.connections, and how we expect max.connections to be relatively high and set based on broker capacity, while listener.max.connections is to allocate connections to applications and should be set based on application requirements.
It took me few extra explanations to get it, so I think a bit more of explanation here will help.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gwenshap Thanks for the review, I have updated the doc.
LGTM. Thank you. |
…s (KIP-402) Adds a new listener config `max.connections` to limit the number of active connections on each listener. The config may be prefixed with listener prefix. This limit may be dynamically reconfigured without restarting the broker. This is one of the PRs for KIP-402 (https://cwiki.apache.org/confluence/display/KAFKA/KIP-402%3A+Improve+fairness+in+SocketServer+processors). Note that this is currently built on top of PR apache#6022 Author: Rajini Sivaram <rajinisivaram@googlemail.com> Reviewers: Gwen Shapira <cshapi@gmail.com> Closes apache#6034 from rajinisivaram/KAFKA-7730-max-connections
Adds a new listener config
max.connections
to limit the number of active connections on each listener. The config may be prefixed with listener prefix. This limit may be dynamically reconfigured without restarting the broker.This is one of the PRs for KIP-402 (https://cwiki.apache.org/confluence/display/KAFKA/KIP-402%3A+Improve+fairness+in+SocketServer+processors). Note that this is currently built on top of PR #6022
Committer Checklist (excluded from commit message)