You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by GitBox <gi...@apache.org> on 2021/10/21 15:39:48 UTC
[GitHub] [kafka] ocadaruma opened a new pull request #11422: KAFKA-9648: Add configuration to adjust listen backlog size for Acceptor
ocadaruma opened a new pull request #11422:
URL: https://github.com/apache/kafka/pull/11422
https://issues.apache.org/jira/browse/KAFKA-9648
* This PR implements [KIP-764](https://cwiki.apache.org/confluence/display/KAFKA/KIP-764%3A+Configurable+backlog+size+for+creating+Acceptor)
- Add new KafkaConfig `socket.listen.backlog.size` which is passed to [ServerSocket#bind](https://docs.oracle.com/en/java/javase/11/docs/api/java.base/java/net/ServerSocket.html#bind(java.net.SocketAddress,int)) to adjust the max length of the queue of incoming connections.
* Please refer the wiki page for the motivation.
### Committer Checklist (excluded from commit message)
- [ ] Verify design and implementation
- [ ] Verify test coverage and CI build status
- [ ] Verify documentation (including upgrade notes)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] dajac commented on pull request #11422: KAFKA-9648: Add configuration to adjust listen backlog size for Acceptor
Posted by GitBox <gi...@apache.org>.
dajac commented on pull request #11422:
URL: https://github.com/apache/kafka/pull/11422#issuecomment-949374252
@ocadaruma Thanks for the PR. We can start reviewing the PR. However, we have to wait a bit to merge it to trunk as the KIP was not accepted in time for AK 3.1.0 and we don't have a release branch yet.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] dajac merged pull request #11422: KAFKA-9648: Add configuration to adjust listen backlog size for Acceptor
Posted by GitBox <gi...@apache.org>.
dajac merged pull request #11422:
URL: https://github.com/apache/kafka/pull/11422
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] ocadaruma commented on a change in pull request #11422: KAFKA-9648: Add configuration to adjust listen backlog size for Acceptor
Posted by GitBox <gi...@apache.org>.
ocadaruma commented on a change in pull request #11422:
URL: https://github.com/apache/kafka/pull/11422#discussion_r749271976
##########
File path: core/src/main/scala/kafka/network/SocketServer.scala
##########
@@ -660,7 +662,7 @@ private[kafka] class Acceptor(val endPoint: EndPoint,
serverChannel.socket().setReceiveBufferSize(recvBufferSize)
try {
- serverChannel.socket.bind(socketAddress)
+ serverChannel.socket.bind(socketAddress, listenBacklogSize)
Review comment:
@dajac Added a test. Please take a look.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] ocadaruma commented on a change in pull request #11422: KAFKA-9648: Add configuration to adjust listen backlog size for Acceptor
Posted by GitBox <gi...@apache.org>.
ocadaruma commented on a change in pull request #11422:
URL: https://github.com/apache/kafka/pull/11422#discussion_r753128390
##########
File path: core/src/test/scala/unit/kafka/network/SocketServerTest.scala
##########
@@ -1854,6 +1854,23 @@ class SocketServerTest {
})
}
+ @Test
+ def testListenBacklogSize(): Unit = {
+ val backlogSize = 128
+ props.put("socket.listen.backlog.size", backlogSize.toString)
+
+ // TCP listen backlog size is the max count of pending connections (i.e. connections such that
+ // 3-way handshake is done at kernel level and waiting to be accepted by the server application.
+ // From client perspective, such connections should be visible as already "connected")
+ // Hence, we can check if listen backlog size is properly configured by trying to connect the server
+ // without starting acceptor thread.
+ withTestableServer(KafkaConfig.fromProps(props), { testableServer =>
+ 1 to backlogSize foreach { _ =>
+ assertTrue(connect(testableServer).isConnected)
Review comment:
As far as I checked, `isConnected` should be true here immediately.
- connect() =>
- https://github.com/adoptium/jdk11u/blob/jdk-11.0.13%2B8/src/java.base/share/classes/java/net/Socket.java#L304 =>
- https://github.com/adoptium/jdk11u/blob/jdk-11.0.13%2B8/src/java.base/share/classes/java/net/Socket.java#L454 =>
- https://github.com/adoptium/jdk11u/blob/jdk-11.0.13%2B8/src/java.base/share/classes/java/net/Socket.java#L617
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] ocadaruma commented on a change in pull request #11422: KAFKA-9648: Add configuration to adjust listen backlog size for Acceptor
Posted by GitBox <gi...@apache.org>.
ocadaruma commented on a change in pull request #11422:
URL: https://github.com/apache/kafka/pull/11422#discussion_r753121483
##########
File path: core/src/test/scala/unit/kafka/network/SocketServerTest.scala
##########
@@ -1854,6 +1854,23 @@ class SocketServerTest {
})
}
+ @Test
+ def testListenBacklogSize(): Unit = {
+ val backlogSize = 128
+ props.put("socket.listen.backlog.size", backlogSize.toString)
+
+ // TCP listen backlog size is the max count of pending connections (i.e. connections such that
+ // 3-way handshake is done at kernel level and waiting to be accepted by the server application.
+ // From client perspective, such connections should be visible as already "connected")
+ // Hence, we can check if listen backlog size is properly configured by trying to connect the server
+ // without starting acceptor thread.
+ withTestableServer(KafkaConfig.fromProps(props), { testableServer =>
+ 1 to backlogSize foreach { _ =>
+ assertTrue(connect(testableServer).isConnected)
+ }
+ }, false)
+ }
Review comment:
In my opinion it doesn't make much sense, because:
- Actually, linux kernel rounds up the backlog value to the next power of two so specified backlog value here and actual backlog size may differ. It's OS-dependent. (In this particular test, I set the value to 128 so I think extra connection would be refused at least on Linux though)
- So I think asserting exact value doesn't make much sense. Just asserting clients can connect up to at least specified value is enough.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] dajac commented on pull request #11422: KAFKA-9648: Add configuration to adjust listen backlog size for Acceptor
Posted by GitBox <gi...@apache.org>.
dajac commented on pull request #11422:
URL: https://github.com/apache/kafka/pull/11422#issuecomment-974040226
Triggered a new Jenkins run. I'll merge once we get a clean build.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] ocadaruma commented on pull request #11422: KAFKA-9648: Add configuration to adjust listen backlog size for Acceptor
Posted by GitBox <gi...@apache.org>.
ocadaruma commented on pull request #11422:
URL: https://github.com/apache/kafka/pull/11422#issuecomment-949269755
Let me mention @rajinisivaram, @mimaison, @dajac here as you participated in the vote for KIP-764.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] dajac commented on pull request #11422: KAFKA-9648: Add configuration to adjust listen backlog size for Acceptor
Posted by GitBox <gi...@apache.org>.
dajac commented on pull request #11422:
URL: https://github.com/apache/kafka/pull/11422#issuecomment-974237986
Build looks good. Failed tests are not related.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] ocadaruma commented on pull request #11422: KAFKA-9648: Add configuration to adjust listen backlog size for Acceptor
Posted by GitBox <gi...@apache.org>.
ocadaruma commented on pull request #11422:
URL: https://github.com/apache/kafka/pull/11422#issuecomment-949387306
@dajac I see. Thank you!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] dajac commented on pull request #11422: KAFKA-9648: Add configuration to adjust listen backlog size for Acceptor
Posted by GitBox <gi...@apache.org>.
dajac commented on pull request #11422:
URL: https://github.com/apache/kafka/pull/11422#issuecomment-974239248
@ocadaruma Thanks for your contribution!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] ocadaruma commented on pull request #11422: KAFKA-9648: Add configuration to adjust listen backlog size for Acceptor
Posted by GitBox <gi...@apache.org>.
ocadaruma commented on pull request #11422:
URL: https://github.com/apache/kafka/pull/11422#issuecomment-974007000
@dajac Thanks for checking. Replied to the comments.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] dajac commented on a change in pull request #11422: KAFKA-9648: Add configuration to adjust listen backlog size for Acceptor
Posted by GitBox <gi...@apache.org>.
dajac commented on a change in pull request #11422:
URL: https://github.com/apache/kafka/pull/11422#discussion_r753096281
##########
File path: core/src/test/scala/unit/kafka/network/SocketServerTest.scala
##########
@@ -1854,6 +1854,23 @@ class SocketServerTest {
})
}
+ @Test
+ def testListenBacklogSize(): Unit = {
+ val backlogSize = 128
+ props.put("socket.listen.backlog.size", backlogSize.toString)
+
+ // TCP listen backlog size is the max count of pending connections (i.e. connections such that
+ // 3-way handshake is done at kernel level and waiting to be accepted by the server application.
+ // From client perspective, such connections should be visible as already "connected")
+ // Hence, we can check if listen backlog size is properly configured by trying to connect the server
+ // without starting acceptor thread.
+ withTestableServer(KafkaConfig.fromProps(props), { testableServer =>
+ 1 to backlogSize foreach { _ =>
+ assertTrue(connect(testableServer).isConnected)
Review comment:
Naive question: Is `isConnected` immediately true or is there a risk that it is not true yet when the assertion is ran?
##########
File path: core/src/test/scala/unit/kafka/network/SocketServerTest.scala
##########
@@ -1854,6 +1854,23 @@ class SocketServerTest {
})
}
+ @Test
+ def testListenBacklogSize(): Unit = {
+ val backlogSize = 128
+ props.put("socket.listen.backlog.size", backlogSize.toString)
+
+ // TCP listen backlog size is the max count of pending connections (i.e. connections such that
+ // 3-way handshake is done at kernel level and waiting to be accepted by the server application.
+ // From client perspective, such connections should be visible as already "connected")
+ // Hence, we can check if listen backlog size is properly configured by trying to connect the server
+ // without starting acceptor thread.
+ withTestableServer(KafkaConfig.fromProps(props), { testableServer =>
+ 1 to backlogSize foreach { _ =>
+ assertTrue(connect(testableServer).isConnected)
+ }
+ }, false)
+ }
Review comment:
Would it make sense to add an extra connection and to verify that `isConnected` is false?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] ocadaruma commented on a change in pull request #11422: KAFKA-9648: Add configuration to adjust listen backlog size for Acceptor
Posted by GitBox <gi...@apache.org>.
ocadaruma commented on a change in pull request #11422:
URL: https://github.com/apache/kafka/pull/11422#discussion_r746164256
##########
File path: core/src/main/scala/kafka/network/SocketServer.scala
##########
@@ -660,7 +662,7 @@ private[kafka] class Acceptor(val endPoint: EndPoint,
serverChannel.socket().setReceiveBufferSize(recvBufferSize)
try {
- serverChannel.socket.bind(socketAddress)
+ serverChannel.socket.bind(socketAddress, listenBacklogSize)
Review comment:
Good point.
Let me try that.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] dajac commented on a change in pull request #11422: KAFKA-9648: Add configuration to adjust listen backlog size for Acceptor
Posted by GitBox <gi...@apache.org>.
dajac commented on a change in pull request #11422:
URL: https://github.com/apache/kafka/pull/11422#discussion_r746011805
##########
File path: core/src/main/scala/kafka/network/SocketServer.scala
##########
@@ -660,7 +662,7 @@ private[kafka] class Acceptor(val endPoint: EndPoint,
serverChannel.socket().setReceiveBufferSize(recvBufferSize)
try {
- serverChannel.socket.bind(socketAddress)
+ serverChannel.socket.bind(socketAddress, listenBacklogSize)
Review comment:
Could we add a unit test which verifies that the configuration is correctly applied to the socket?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org