KAFKA-5674: max.connections.per.ip minimum should be 0#3610
Conversation
|
Refer to this link for build results (access rights to CI server needed): |
|
Refer to this link for build results (access rights to CI server needed): |
|
@granthenke @andrasbeni @peter-toth could you please have a look? |
There was a problem hiding this comment.
Hi @viktorsomogyi,
- You need at least 2 network interface (including the loopback) to get this part tested.
- I would use NetworkInterface.getNetworkInterfaces() to find an IP that is different to one assigned to "localhost". (InetAddress.getLocalHost may return with the IP of "localhost" on some weird configurations.)
There was a problem hiding this comment.
On a second thought, leaving it as it is. Using NetworkInterface makes it overly complicated as for instance on my laptop for instance there are many interfaces which makes it hard and cumbersome to choose the right one (and I don't expect this to be different on build servers either).
I think InetAddress.getLocalHost is good as supposing a correct machine configuration, it will return the right address.
a8dee0b to
8140b4e
Compare
|
Refer to this link for build results (access rights to CI server needed): |
|
Refer to this link for build results (access rights to CI server needed): |
granthenke
left a comment
granthenke
left a comment
There was a problem hiding this comment.
Overall looks good. Just one small comment. Thanks for the contribution.
There was a problem hiding this comment.
Do we need to add localPort? If we can use ephemeral ports thats preferred.
8140b4e to
9d82637
Compare
|
Refer to this link for build results (access rights to CI server needed): |
|
Refer to this link for build results (access rights to CI server needed): |
3da41cb to
34af60d
Compare
|
retest this please |
1 similar comment
|
retest this please |
34af60d to
ac5c1aa
Compare
|
retest this please |
ac5c1aa to
fe6f106
Compare
fe6f106 to
b657553
Compare
|
@hachikuji would you mind reviewing this? It has been submitted a while ago and I rebased it yesterday on trunk. I'd like to get some feedback whether this feature makes sense and is eligible to be merged or anything else should be done. |
hachikuji
left a comment
hachikuji
left a comment
There was a problem hiding this comment.
Thanks, I think this makes sense. Do you think it's worth mentioning the change in the upgrade notes?
Change-Id: Id9ba5d9205afc5e296c231e4e165a0089d846cc7
|
@hachikuji thanks for your quick response. I've added the upgrade notes under the 1.2 notes (hopefully this is the right place). Please let me know in case I should have added it elsewhere or if there are any grammatical mistakes in it. |
| .define(SocketReceiveBufferBytesProp, INT, Defaults.SocketReceiveBufferBytes, HIGH, SocketReceiveBufferBytesDoc) | ||
| .define(SocketRequestMaxBytesProp, INT, Defaults.SocketRequestMaxBytes, atLeast(1), HIGH, SocketRequestMaxBytesDoc) | ||
| .define(MaxConnectionsPerIpProp, INT, Defaults.MaxConnectionsPerIp, atLeast(1), MEDIUM, MaxConnectionsPerIpDoc) | ||
| .define(MaxConnectionsPerIpProp, INT, Defaults.MaxConnectionsPerIp, atLeast(0), MEDIUM, MaxConnectionsPerIpDoc) |
There was a problem hiding this comment.
Not related to this PR. Can we also update the doc string of MaxConnectionsPerIpOverridesDoc.
Currently it is not clear about the format of the config string.
some thing like this:
A comma separated list of per-ip or hostname overrides to the default maximum number of connections. Example value is : hostName:100,127.0.0.1:200.
hachikuji
left a comment
hachikuji
left a comment
There was a problem hiding this comment.
LGTM. Thanks for the patch!
6 participants
No description provided.