Hi Rajini, Thanks for the reply.
> Not sure 10s is a good default because it unnecessarily times out > connections, only to attempt re-connecting to the same broker (except in > the leastLoadedNode case where it would be useful to have a lower timeout). The underlying logic for a connection turn from “connecting” to “connected” is finishing the TCP three-way handshake (done by socketChannel.connect()). So we can say the 10 seconds timeout is for the transportation layer three-way handshake. The request timeout includes more besides the handshakes, such as authentication, server processing the request, and server sending back the response. Thus I think setting the default of socket.connection.setup.timeout.ms a smaller value than request.timeout.ms is reasonable. Do you have any specific reason in mind that 10s is too short? Best, -Chang Tan > 在 2020年5月14日，上午6:44，Rajini Sivaram <rajinisiva...@gmail.com> 写道： > > Hi Cheng, > > 1) Thanks for the update, the KIP now says ` > socket.connections.setup.timeout.ms*`*, which sounds good. > > 2) Not sure 10s is a good default because it unnecessarily times out > connections, only to attempt re-connecting to the same broker (except in > the leastLoadedNode case where it would be useful to have a lower timeout). > Couldn't we just use 30s (request timeout) as the default value, retaining > the current behaviour? Or alternatively, only apply the timeout where it > makes sense to have a lower timeout (i.e. a timeout for leastLoadedNode)? > > Regards, > > Rajini > >> On Thu, May 14, 2020 at 5:00 AM Cheng Tan <c...@confluent.io> wrote: >> >> Hi Rajini, >> >> Thanks for the comments. >> >>> I think >>> they started off as connection timeouts but now include authentication >> time >>> as well. Have we considered using similar configs for this case? >> >> >> The new config I proposed is focusing on the connections to unreachable >> servers. The timeout count won’t involved the authentication. I think >> “socket.” prefix make sense. I’ll change it. Colin mentioned that adding >> the key word “setup” can help people understand that this config only >> applies to the connection setup. What about “socket.connection.setup.ms”? >> >>> The KIP proposes 10s as the default. What does this mean for typical >>> connections like a produce request going to the leader? >> >> The new timeout config applies to each connection. It’s at the >> NetworkClient level and won’t consider the underlying connection logic. >> Specifically, by default, every connection will have 10 seconds to become >> “connected” from “connecting”, which implies the corresponding socket >> channel is now connected (SocketChanel.finishConnect() returns True), no >> matter what the request logic and abstraction is. >> >> Please let me know if these make sense to you or if you have more >> thoughts. Thank you. >> >> Best, -Cheng >> >>>> 在 2020年5月13日，上午7:11，Rajini Sivaram <rajinisiva...@gmail.com> 写道： >>> >>> Hi Cheng, >>> >>> Thanks for the KIP, sounds like a good improvement. A couple of comments: >>> >>> 1) We currently have client connection timeouts on the broker with >> configs >>> named `xxx.socket.timeout.ms` (e.g. controller.socket.timeout.ms). I >> think >>> they started off as connection timeouts but now include authentication >> time >>> as well. Have we considered using similar configs for this case? We may >>> want to prefix the new config with `socket.` anyway - something along the >>> lines of `socket.connection.timeout.ms` if it is just the connection >> time. >>> >>> 2) The KIP proposes 10s as the default. What does this mean for typical >>> connections like a produce request going to the leader? Instead of one >>> connection attempt to the leader, we want three separate connection >>> attempts within the request timeout to the leader? >>> >>> Regards, >>> >>> Rajini >>> >>> >>>> On Thu, May 7, 2020 at 11:51 PM Jose Garcia Sancio < >> jsan...@confluent.io> >>>> wrote: >>>> Cheng, >>>> Thanks for the KIP and the detailed proposal section. LGTM! >>>>>> On Thu, May 7, 2020 at 3:38 PM Cheng Tan <c...@confluent.io> wrote: >>>>>> I think more about the potential wider use cases, I modified the >>>> proposal to target all the connection. Thanks. >>>>> - Best, - Cheng Tan >>>>>> On May 7, 2020, at 1:41 AM, Cheng Tan <c...@confluent.io> wrote: >>>>>> Hi Colin, >>>>>> Sorry for the confusion. I’m proposing to implement timeout in the >>>> NetworkClient.leastLoadedNode() when iterating all the cached node. The >>>> alternative I can think is to implement the timeout in >> NetworkClient.poll() >>>>>> I’d prefer to implement in the leastLoadedNode(). Here’re the reasons: >>>>>> Usually when clients send a request, they will asking the network >>>> client to send the request to a specific node. In this case, the >>>> connection.setup.timeout won’t matter too much because the client >> doesn’t >>>> want to try other nodes for that specific request. The request level >>>> timeout would be enough. The metadata fetcher fetches the nodes status >>>> periodically so the clients can reassign the request to another node >> after >>>> timeout. >>>>>> Consumer, producer, and AdminClient are all using leastLoadedNode() >>>> for metadata fetch, where the connection setup timeout can play an >>>> important role. Unlike other requests can refer to the metadata for node >>>> condition, the metadata requests can only blindly choose a node for >> retry >>>> in the worst scenario. We want to make sure the client can get the >> metadata >>>> smoothly and as soon as possible. As a result, we need this >>>> connection.setup.timeout. >>>>>> Implementing the timeout in poll() or anywhere else might need an >>>> extra iteration of all nodes, which might downgrade the network client >>>> performance. >>>>>> I also updated the KIP content and KIP status. Please let me know if >>>> the above ideas make sense. Thanks. >>>>>> Best, - Cheng Tan >>>>>>> On May 4, 2020, at 5:26 PM, Colin McCabe <cmcc...@apache.org >> <mailto: >>>> cmcc...@apache.org>> wrote: >>>>>>> Hi Cheng, >>>>>>> On the KIP page, it lists this KIP as "draft." It seems like "under >>>> discussion" is appropriate here, right? >>>>>>>> Currently, the initial socket connection timeout is depending on >>>> Linux kernel setting >>>>>>>> tcp_syn_retries. The timeout value is 2 ^ (tcp_sync_retries + 1) - 1 >>>> seconds. For the >>>>>>>> reasons below, we want to control the client-side socket timeout >>>> directly using >>>>>>>> configuration files >>>>>>> Linux is just one example of an OS that Kafka could run on, right? >>>> You could also be running on MacOS, for example. >>>>>>>> I'm proposing to do a lazy socket connection time out. That is, we >>>> only check if >>>>>>>> we need to timeout a socket when we consider the corresponding node >>>> as a >>>>>>>> candidate in the node provider. >>>>>>> The NodeProvider is an AdminClient abstraction, right? Why wouldn't >>>> we implement a connection setup timeout for all clients, not just >>>> AdminClient? >>>>>>> best, >>>>>>> Colin >>>>>>> On Mon, May 4, 2020, at 13:18, Colin McCabe wrote: >>>>>>>> Hmm. A big part of the reason behind the KIP is that the default >>>>>>>> connection timeout behavior of the OS doesn't work for Kafka, right? >>>>>>>> For example, on Linux, if we wait 127 seconds for a connection >>>> attempt >>>>>>>> to time out, we won't get a chance to make another attempt in most >>>>>>>> cases. So I think it makes sense to set a shorter default. >>>>>>>> best, >>>>>>>> Colin >>>>>>>> On Mon, May 4, 2020, at 09:44, Jose Garcia Sancio wrote: >>>>>>>>> Thanks for the KIP Cheng, >>>>>>>>>> The default value will be 10 seconds. >>>>>>>>> I think we should make the default the current behavior. Meaning >> the >>>>>>>>> default should leverage the default connect timeout from the >>>> operating >>>>>>>>> system. >>>>>>>>>> Proposed Changes >>>>>>>>> I don't fully understand this section. It seems like it is mainly >>>>>>>>> focused on the problem with the current implementation. Can you >>>>>>>>> explain how the proposed changes solve the problem? >>>>>>>>> Thanks. >>>>>>>>> -- >>>>>>>>> -Jose >>>> -- >>>> -Jose >>
