Re: [DISCUSS] KIP-601: Configurable socket connection timeout

[Colin McCabe]

On Tue, May 19, 2020, at 03:27, Rajini Sivaram wrote:
> Hi Colin,
> 
> I do agree about the `leastLoadedNode` case. My question was about the
> other cases where we are connecting to a specific node: fetch requests to
> leaders, produce requests to leaders, requests to group coordinators,
> requests to controller etc. It will be good to either quantify that these
> connections are less common and hence less critical in terms of performance
> in typical deployments or describe the impact on these connections from the
> proposed change in default behaviour. It is perfectly fine if connections
> to specific nodes don't benefit from the new timeout, I was looking for
> analysis which says they aren't made any worse either, especially in the
> context of other connection rate limiting/quota work we are proposing like
> KIP-612.
> 

Hi Rajini,

This is a fair point.  In the VOTE thread, I proposed using an exponential 
connection retry backoff to mitigate this problem.  So the first few retries 
would happen quickly, but later retries would take increasingly longer, keeping 
the number of reconnect attempts down.

(This is assuming we're trying to connect to a single fixed node, like the 
controller node)

best,
Colin


>
> Regards,
> 
> Rajini
> 
> 
> On Mon, May 18, 2020 at 8:48 PM Colin McCabe  wrote:
> 
> > Hi Rajini,
> >
> > I think the idea behind the 10 second default is that if you have three
> > Kafka nodes A, B, C (or whatever), and you can't talk to A within 10
> > seconds, you'll try again with B or C, and still have plenty of time left
> > over.  Whereas currently, if your connection hangs while trying to connect
> > to A, you're out of luck-- you'll just hang until the whole request timeout
> > is gone.  So while you could have tried a different node and succeeded, you
> > never got a chance to.
> >
> > So in the common case where you have other nodes that you can connect to,
> > we won't end up trying to reconnect to the same node over and over.  I'll
> > add some more comments in the vote thread.
> >
> > best,
> > Colin
> >
> >
> > On Fri, May 15, 2020, at 14:13, Rajini Sivaram wrote:
> > > Hi Cheng,
> > >
> > > I am fine with the rest of the KIP apart from the 10s default. If no one
> > > else has any concerns about this new default, let's go with it. Please go
> > > ahead and start vote.
> > >
> > > Regards,
> > >
> > > Rajini
> > >
> > >
> > > On Fri, May 15, 2020 at 8:21 PM Cheng Tan  wrote:
> > >
> > > > Dear Rajini,
> > > >
> > > >
> > > > Thanks for the reply.
> > > >
> > > > > e have a lot of these and I want to
> > > > > understand the benefits of the proposed timeout in this case alone.
> > We
> > > > > currently have a request timeout of 30s. Would you consider adding a
> > 10s
> > > > > connection timeout?
> > > >
> > > > A shorter timeout (10s) at the transportation level will help clients
> > > > detect dead nodes faster. “request.timeout.ms” is too general and
> > applies
> > > > to all the requests whose complexity at the application level varies.
> > It’s
> > > > risky to set “request.timeout.ms” to a lower value for detecting dead
> > > > nodes quicker because of the involvement of the application layer.
> > > >
> > > > After “socket.connection.setup.timeout.ms” hits, NetworkClient will
> > fail
> > > > the request in the exact approach as it handles “request.timeout.ms”.
> > > > That is to say, the response will constructed upon a
> > RetriableException.
> > > > Producer, Consumer, and KafkaAdminClient can then perform their retry
> > logic
> > > > as a request timeout happens.
> > > >
> > > > > We have KIP-612 that is proposing to throttle connection set up on
> > the
> > > > one
> > > > > hand and this KIP that is dramatically reducing default connection
> > > > timeout
> > > > > on the other. Not sure if that is a good idea.
> > > >
> > > > The default of the broker connection creation rate limit is
> > Int.MaxValue.
> > > > The KIP also proposes per-IP throttle configuration. Thus, I don’t
> > expect
> > > > the combination of the broker connection throttle and a shorter client
> > > > transportation timeout will have a side effect.
> > > >
> > > > Does the reasons above make sense to you?
> > > >
> > > > Best, - Cheng
> > > >
> > > >
> > > >
> > > >
> > > > > On May 15, 2020, at 4:49 AM, Rajini Sivaram  > >
> > > > wrote:
> > > > >
> > > > > Hi Cheng,
> > > > >
> > > > > Let me rephrase my question. Let's say we didn't have the case of
> > > > > leastLoadedNode. We are only talking about connections to a specific
> > node
> > > > > (i.e. leader or controller). We have a lot of these and I want to
> > > > > understand the benefits of the proposed timeout in this case alone.
> > We
> > > > > currently have a request timeout of 30s. Would you consider adding a
> > 10s
> > > > > connection timeout? And if you did, what would you expect the 10s
> > timeout
> > > > > to do?
> > > > >
> > > > > a) We could fail a request if connection didn't complete within 10s.
> > If
> > > > 

Re: [DISCUSS] KIP-601: Configurable socket connection timeout

[Rajini Sivaram]

Hi Colin,

I do agree about the `leastLoadedNode` case. My question was about the
other cases where we are connecting to a specific node: fetch requests to
leaders, produce requests to leaders, requests to group coordinators,
requests to controller etc. It will be good to either quantify that these
connections are less common and hence less critical in terms of performance
in typical deployments or describe the impact on these connections from the
proposed change in default behaviour. It is perfectly fine if connections
to specific nodes don't benefit from the new timeout, I was looking for
analysis which says they aren't made any worse either, especially in the
context of other connection rate limiting/quota work we are proposing like
KIP-612.

Regards,

Rajini


On Mon, May 18, 2020 at 8:48 PM Colin McCabe  wrote:

> Hi Rajini,
>
> I think the idea behind the 10 second default is that if you have three
> Kafka nodes A, B, C (or whatever), and you can't talk to A within 10
> seconds, you'll try again with B or C, and still have plenty of time left
> over.  Whereas currently, if your connection hangs while trying to connect
> to A, you're out of luck-- you'll just hang until the whole request timeout
> is gone.  So while you could have tried a different node and succeeded, you
> never got a chance to.
>
> So in the common case where you have other nodes that you can connect to,
> we won't end up trying to reconnect to the same node over and over.  I'll
> add some more comments in the vote thread.
>
> best,
> Colin
>
>
> On Fri, May 15, 2020, at 14:13, Rajini Sivaram wrote:
> > Hi Cheng,
> >
> > I am fine with the rest of the KIP apart from the 10s default. If no one
> > else has any concerns about this new default, let's go with it. Please go
> > ahead and start vote.
> >
> > Regards,
> >
> > Rajini
> >
> >
> > On Fri, May 15, 2020 at 8:21 PM Cheng Tan  wrote:
> >
> > > Dear Rajini,
> > >
> > >
> > > Thanks for the reply.
> > >
> > > > e have a lot of these and I want to
> > > > understand the benefits of the proposed timeout in this case alone.
> We
> > > > currently have a request timeout of 30s. Would you consider adding a
> 10s
> > > > connection timeout?
> > >
> > > A shorter timeout (10s) at the transportation level will help clients
> > > detect dead nodes faster. “request.timeout.ms” is too general and
> applies
> > > to all the requests whose complexity at the application level varies.
> It’s
> > > risky to set “request.timeout.ms” to a lower value for detecting dead
> > > nodes quicker because of the involvement of the application layer.
> > >
> > > After “socket.connection.setup.timeout.ms” hits, NetworkClient will
> fail
> > > the request in the exact approach as it handles “request.timeout.ms”.
> > > That is to say, the response will constructed upon a
> RetriableException.
> > > Producer, Consumer, and KafkaAdminClient can then perform their retry
> logic
> > > as a request timeout happens.
> > >
> > > > We have KIP-612 that is proposing to throttle connection set up on
> the
> > > one
> > > > hand and this KIP that is dramatically reducing default connection
> > > timeout
> > > > on the other. Not sure if that is a good idea.
> > >
> > > The default of the broker connection creation rate limit is
> Int.MaxValue.
> > > The KIP also proposes per-IP throttle configuration. Thus, I don’t
> expect
> > > the combination of the broker connection throttle and a shorter client
> > > transportation timeout will have a side effect.
> > >
> > > Does the reasons above make sense to you?
> > >
> > > Best, - Cheng
> > >
> > >
> > >
> > >
> > > > On May 15, 2020, at 4:49 AM, Rajini Sivaram  >
> > > wrote:
> > > >
> > > > Hi Cheng,
> > > >
> > > > Let me rephrase my question. Let's say we didn't have the case of
> > > > leastLoadedNode. We are only talking about connections to a specific
> node
> > > > (i.e. leader or controller). We have a lot of these and I want to
> > > > understand the benefits of the proposed timeout in this case alone.
> We
> > > > currently have a request timeout of 30s. Would you consider adding a
> 10s
> > > > connection timeout? And if you did, what would you expect the 10s
> timeout
> > > > to do?
> > > >
> > > > a) We could fail a request if connection didn't complete within 10s.
> If
> > > we
> > > > always expect connections to succeed within 10s, this would be
> considered
> > > > reasonable behaviour. But this would be changing the current default,
> > > which
> > > > allows you up to 30 seconds to connect and process a request.
> > > > b) We retry the connection. What would be the point? We were waiting
> in a
> > > > queue for connecting, but we decide to stop and join the back of the
> > > queue.
> > > >
> > > > We have KIP-612 that is proposing to throttle connection set up on
> the
> > > one
> > > > hand and this KIP that is dramatically reducing default connection
> > > timeout
> > > > on the other. Not sure if that is a good idea.
> > > >
> > > >
> > > > On Fri, May 

Re: [DISCUSS] KIP-601: Configurable socket connection timeout

[Colin McCabe]

Hi Rajini,

I think the idea behind the 10 second default is that if you have three Kafka 
nodes A, B, C (or whatever), and you can't talk to A within 10 seconds, you'll 
try again with B or C, and still have plenty of time left over.  Whereas 
currently, if your connection hangs while trying to connect to A, you're out of 
luck-- you'll just hang until the whole request timeout is gone.  So while you 
could have tried a different node and succeeded, you never got a chance to.

So in the common case where you have other nodes that you can connect to, we 
won't end up trying to reconnect to the same node over and over.  I'll add some 
more comments in the vote thread.

best,
Colin


On Fri, May 15, 2020, at 14:13, Rajini Sivaram wrote:
> Hi Cheng,
> 
> I am fine with the rest of the KIP apart from the 10s default. If no one
> else has any concerns about this new default, let's go with it. Please go
> ahead and start vote.
> 
> Regards,
> 
> Rajini
> 
> 
> On Fri, May 15, 2020 at 8:21 PM Cheng Tan  wrote:
> 
> > Dear Rajini,
> >
> >
> > Thanks for the reply.
> >
> > > e have a lot of these and I want to
> > > understand the benefits of the proposed timeout in this case alone. We
> > > currently have a request timeout of 30s. Would you consider adding a 10s
> > > connection timeout?
> >
> > A shorter timeout (10s) at the transportation level will help clients
> > detect dead nodes faster. “request.timeout.ms” is too general and applies
> > to all the requests whose complexity at the application level varies.  It’s
> > risky to set “request.timeout.ms” to a lower value for detecting dead
> > nodes quicker because of the involvement of the application layer.
> >
> > After “socket.connection.setup.timeout.ms” hits, NetworkClient will fail
> > the request in the exact approach as it handles “request.timeout.ms”.
> > That is to say, the response will constructed upon a RetriableException.
> > Producer, Consumer, and KafkaAdminClient can then perform their retry logic
> > as a request timeout happens.
> >
> > > We have KIP-612 that is proposing to throttle connection set up on the
> > one
> > > hand and this KIP that is dramatically reducing default connection
> > timeout
> > > on the other. Not sure if that is a good idea.
> >
> > The default of the broker connection creation rate limit is Int.MaxValue.
> > The KIP also proposes per-IP throttle configuration. Thus, I don’t expect
> > the combination of the broker connection throttle and a shorter client
> > transportation timeout will have a side effect.
> >
> > Does the reasons above make sense to you?
> >
> > Best, - Cheng
> >
> >
> >
> >
> > > On May 15, 2020, at 4:49 AM, Rajini Sivaram 
> > wrote:
> > >
> > > Hi Cheng,
> > >
> > > Let me rephrase my question. Let's say we didn't have the case of
> > > leastLoadedNode. We are only talking about connections to a specific node
> > > (i.e. leader or controller). We have a lot of these and I want to
> > > understand the benefits of the proposed timeout in this case alone. We
> > > currently have a request timeout of 30s. Would you consider adding a 10s
> > > connection timeout? And if you did, what would you expect the 10s timeout
> > > to do?
> > >
> > > a) We could fail a request if connection didn't complete within 10s. If
> > we
> > > always expect connections to succeed within 10s, this would be considered
> > > reasonable behaviour. But this would be changing the current default,
> > which
> > > allows you up to 30 seconds to connect and process a request.
> > > b) We retry the connection. What would be the point? We were waiting in a
> > > queue for connecting, but we decide to stop and join the back of the
> > queue.
> > >
> > > We have KIP-612 that is proposing to throttle connection set up on the
> > one
> > > hand and this KIP that is dramatically reducing default connection
> > timeout
> > > on the other. Not sure if that is a good idea.
> > >
> > >
> > > On Fri, May 15, 2020 at 1:26 AM Cheng Tan  wrote:
> >
> >
>

Re: [DISCUSS] KIP-601: Configurable socket connection timeout

[Cheng Tan]

Dear Rajini,

Thanks for all the feedbacks. They are very helpful for me to do the 
brainstorming. I’ve incorporated our discuss in the KIP and started a voting 
thread. 

Best, - Cheng Tan

> On May 15, 2020, at 2:13 PM, Rajini Sivaram  wrote:
> 
> Hi Cheng,
> 
> I am fine with the rest of the KIP apart from the 10s default. If no one
> else has any concerns about this new default, let's go with it. Please go
> ahead and start vote.
> 
> Regards,
> 
> Rajini
> 
> 
> On Fri, May 15, 2020 at 8:21 PM Cheng Tan  wrote:
> 
>> Dear Rajini,
>> 
>> 
>> Thanks for the reply.
>> 
>>> e have a lot of these and I want to
>>> understand the benefits of the proposed timeout in this case alone. We
>>> currently have a request timeout of 30s. Would you consider adding a 10s
>>> connection timeout?
>> 
>> A shorter timeout (10s) at the transportation level will help clients
>> detect dead nodes faster. “request.timeout.ms” is too general and applies
>> to all the requests whose complexity at the application level varies.  It’s
>> risky to set “request.timeout.ms” to a lower value for detecting dead
>> nodes quicker because of the involvement of the application layer.
>> 
>> After “socket.connection.setup.timeout.ms” hits, NetworkClient will fail
>> the request in the exact approach as it handles “request.timeout.ms”.
>> That is to say, the response will constructed upon a RetriableException.
>> Producer, Consumer, and KafkaAdminClient can then perform their retry logic
>> as a request timeout happens.
>> 
>>> We have KIP-612 that is proposing to throttle connection set up on the
>> one
>>> hand and this KIP that is dramatically reducing default connection
>> timeout
>>> on the other. Not sure if that is a good idea.
>> 
>> The default of the broker connection creation rate limit is Int.MaxValue.
>> The KIP also proposes per-IP throttle configuration. Thus, I don’t expect
>> the combination of the broker connection throttle and a shorter client
>> transportation timeout will have a side effect.
>> 
>> Does the reasons above make sense to you?
>> 
>> Best, - Cheng
>> 
>> 
>> 
>> 
>>> On May 15, 2020, at 4:49 AM, Rajini Sivaram 
>> wrote:
>>> 
>>> Hi Cheng,
>>> 
>>> Let me rephrase my question. Let's say we didn't have the case of
>>> leastLoadedNode. We are only talking about connections to a specific node
>>> (i.e. leader or controller). We have a lot of these and I want to
>>> understand the benefits of the proposed timeout in this case alone. We
>>> currently have a request timeout of 30s. Would you consider adding a 10s
>>> connection timeout? And if you did, what would you expect the 10s timeout
>>> to do?
>>> 
>>> a) We could fail a request if connection didn't complete within 10s. If
>> we
>>> always expect connections to succeed within 10s, this would be considered
>>> reasonable behaviour. But this would be changing the current default,
>> which
>>> allows you up to 30 seconds to connect and process a request.
>>> b) We retry the connection. What would be the point? We were waiting in a
>>> queue for connecting, but we decide to stop and join the back of the
>> queue.
>>> 
>>> We have KIP-612 that is proposing to throttle connection set up on the
>> one
>>> hand and this KIP that is dramatically reducing default connection
>> timeout
>>> on the other. Not sure if that is a good idea.
>>> 
>>> 
>>> On Fri, May 15, 2020 at 1:26 AM Cheng Tan  wrote:
>> 
>> 

Re: [DISCUSS] KIP-601: Configurable socket connection timeout

[Rajini Sivaram]

Hi Cheng,

I am fine with the rest of the KIP apart from the 10s default. If no one
else has any concerns about this new default, let's go with it. Please go
ahead and start vote.

Regards,

Rajini


On Fri, May 15, 2020 at 8:21 PM Cheng Tan  wrote:

> Dear Rajini,
>
>
> Thanks for the reply.
>
> > e have a lot of these and I want to
> > understand the benefits of the proposed timeout in this case alone. We
> > currently have a request timeout of 30s. Would you consider adding a 10s
> > connection timeout?
>
> A shorter timeout (10s) at the transportation level will help clients
> detect dead nodes faster. “request.timeout.ms” is too general and applies
> to all the requests whose complexity at the application level varies.  It’s
> risky to set “request.timeout.ms” to a lower value for detecting dead
> nodes quicker because of the involvement of the application layer.
>
> After “socket.connection.setup.timeout.ms” hits, NetworkClient will fail
> the request in the exact approach as it handles “request.timeout.ms”.
> That is to say, the response will constructed upon a RetriableException.
> Producer, Consumer, and KafkaAdminClient can then perform their retry logic
> as a request timeout happens.
>
> > We have KIP-612 that is proposing to throttle connection set up on the
> one
> > hand and this KIP that is dramatically reducing default connection
> timeout
> > on the other. Not sure if that is a good idea.
>
> The default of the broker connection creation rate limit is Int.MaxValue.
> The KIP also proposes per-IP throttle configuration. Thus, I don’t expect
> the combination of the broker connection throttle and a shorter client
> transportation timeout will have a side effect.
>
> Does the reasons above make sense to you?
>
> Best, - Cheng
>
>
>
>
> > On May 15, 2020, at 4:49 AM, Rajini Sivaram 
> wrote:
> >
> > Hi Cheng,
> >
> > Let me rephrase my question. Let's say we didn't have the case of
> > leastLoadedNode. We are only talking about connections to a specific node
> > (i.e. leader or controller). We have a lot of these and I want to
> > understand the benefits of the proposed timeout in this case alone. We
> > currently have a request timeout of 30s. Would you consider adding a 10s
> > connection timeout? And if you did, what would you expect the 10s timeout
> > to do?
> >
> > a) We could fail a request if connection didn't complete within 10s. If
> we
> > always expect connections to succeed within 10s, this would be considered
> > reasonable behaviour. But this would be changing the current default,
> which
> > allows you up to 30 seconds to connect and process a request.
> > b) We retry the connection. What would be the point? We were waiting in a
> > queue for connecting, but we decide to stop and join the back of the
> queue.
> >
> > We have KIP-612 that is proposing to throttle connection set up on the
> one
> > hand and this KIP that is dramatically reducing default connection
> timeout
> > on the other. Not sure if that is a good idea.
> >
> >
> > On Fri, May 15, 2020 at 1:26 AM Cheng Tan  wrote:
>
>

Re: [DISCUSS] KIP-601: Configurable socket connection timeout

[Cheng Tan]

Dear Rajini,


Thanks for the reply. 

> e have a lot of these and I want to
> understand the benefits of the proposed timeout in this case alone. We
> currently have a request timeout of 30s. Would you consider adding a 10s
> connection timeout? 

A shorter timeout (10s) at the transportation level will help clients detect 
dead nodes faster. “request.timeout.ms” is too general and applies to all the 
requests whose complexity at the application level varies.  It’s risky to set 
“request.timeout.ms” to a lower value for detecting dead nodes quicker because 
of the involvement of the application layer.

After “socket.connection.setup.timeout.ms” hits, NetworkClient will fail the 
request in the exact approach as it handles “request.timeout.ms”. That is to 
say, the response will constructed upon a RetriableException. Producer, 
Consumer, and KafkaAdminClient can then perform their retry logic as a request 
timeout happens.

> We have KIP-612 that is proposing to throttle connection set up on the one
> hand and this KIP that is dramatically reducing default connection timeout
> on the other. Not sure if that is a good idea.

The default of the broker connection creation rate limit is Int.MaxValue. The 
KIP also proposes per-IP throttle configuration. Thus, I don’t expect the 
combination of the broker connection throttle and a shorter client 
transportation timeout will have a side effect.

Does the reasons above make sense to you? 

Best, - Cheng




> On May 15, 2020, at 4:49 AM, Rajini Sivaram  wrote:
> 
> Hi Cheng,
> 
> Let me rephrase my question. Let's say we didn't have the case of
> leastLoadedNode. We are only talking about connections to a specific node
> (i.e. leader or controller). We have a lot of these and I want to
> understand the benefits of the proposed timeout in this case alone. We
> currently have a request timeout of 30s. Would you consider adding a 10s
> connection timeout? And if you did, what would you expect the 10s timeout
> to do?
> 
> a) We could fail a request if connection didn't complete within 10s. If we
> always expect connections to succeed within 10s, this would be considered
> reasonable behaviour. But this would be changing the current default, which
> allows you up to 30 seconds to connect and process a request.
> b) We retry the connection. What would be the point? We were waiting in a
> queue for connecting, but we decide to stop and join the back of the queue.
> 
> We have KIP-612 that is proposing to throttle connection set up on the one
> hand and this KIP that is dramatically reducing default connection timeout
> on the other. Not sure if that is a good idea.
> 
> 
> On Fri, May 15, 2020 at 1:26 AM Cheng Tan  wrote:

Re: [DISCUSS] KIP-601: Configurable socket connection timeout

[Rajini Sivaram]

Hi Cheng,

Let me rephrase my question. Let's say we didn't have the case of
leastLoadedNode. We are only talking about connections to a specific node
(i.e. leader or controller). We have a lot of these and I want to
understand the benefits of the proposed timeout in this case alone. We
currently have a request timeout of 30s. Would you consider adding a 10s
connection timeout? And if you did, what would you expect the 10s timeout
to do?

a) We could fail a request if connection didn't complete within 10s. If we
always expect connections to succeed within 10s, this would be considered
reasonable behaviour. But this would be changing the current default, which
allows you up to 30 seconds to connect and process a request.
b) We retry the connection. What would be the point? We were waiting in a
queue for connecting, but we decide to stop and join the back of the queue.

We have KIP-612 that is proposing to throttle connection set up on the one
hand and this KIP that is dramatically reducing default connection timeout
on the other. Not sure if that is a good idea.


On Fri, May 15, 2020 at 1:26 AM Cheng Tan  wrote:

> 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  写道：
> >
> > 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  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  写道：
> >>>
> >>> 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 

Re: [DISCUSS] KIP-601: Configurable socket connection timeout

[Cheng Tan]

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  写道：
> 
> 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  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  写道：
>>> 
>>> 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  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  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 

Re: [DISCUSS] KIP-601: Configurable socket connection timeout

[Rajini Sivaram]

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  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  写道：
> >
> > 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  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  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>> wrote:
> > Hi Cheng,
> > On the KIP page, it lists this KIP as "draft."  

Re: [DISCUSS] KIP-601: Configurable socket connection timeout

[Cheng Tan]

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  写道：
> 
> 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 
>> wrote:
>> Cheng,
>> Thanks for the KIP and the detailed proposal section. LGTM!
 On Thu, May 7, 2020 at 3:38 PM Cheng Tan  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  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>> 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 

Re: [DISCUSS] KIP-601: Configurable socket connection timeout

[Rajini Sivaram]

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 
wrote:

> Cheng,
>
> Thanks for the KIP and the detailed proposal section. LGTM!
>
> On Thu, May 7, 2020 at 3:38 PM Cheng Tan  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  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>> 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.
> > 
> > 

Re: [DISCUSS] KIP-601: Configurable socket connection timeout

[Jose Garcia Sancio]

Cheng,

Thanks for the KIP and the detailed proposal section. LGTM!

On Thu, May 7, 2020 at 3:38 PM Cheng Tan  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  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  >> > 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

Re: [DISCUSS] KIP-601: Configurable socket connection timeout

[Cheng Tan]

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  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 > > 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
 
>>> 
> 

Re: [DISCUSS] KIP-601: Configurable socket connection timeout

[Cheng Tan]

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  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
>>> 
>> 

Re: [DISCUSS] KIP-601: Configurable socket connection timeout

[Colin McCabe]

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
> >
>

Re: [DISCUSS] KIP-601: Configurable socket connection timeout

[Colin McCabe]

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
>

Re: [DISCUSS] KIP-601: Configurable socket connection timeout

[Jose Garcia Sancio]

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

Re: [DISCUSS] KIP-601: Configurable socket connection timeout

[Cheng Tan]

Hi Colin. Thanks for the discussion and feedback. I re-wrote the KIP-601 
proposal following your suggestions. Now the new proposal is ready.

Best, - Cheng Tan

> On Apr 28, 2020, at 2:55 PM, Colin McCabe  wrote:
> 
> 
> Thanks again for the KIP.  This seems like it has been a gap in Kaf

Re: [DISCUSS] KIP-601: Configurable socket connection timeout

[Colin McCabe]

Hi Cheng,

Thanks for the KIP.

> Currently, the initial socket connection timeout is depending on system 
> setting tcp_syn_retries. The actual timeout value is 2 ^ (tcp_sync_retries + 
> 1) - 1 seconds

This section is confusing since it refers to Linux configuration settings 
without mentioning identifying them as such.  I wondered for a second if Kafka 
had a tcp_syn_retries setting (it doesn't.)  You should make it more clear when 
you are referring to an operating-system-specific detail.

> We propose a new common client config: connections.timeout.ms

The name "connections.timeout.ms" seems unclear.  If I had to guess about what 
"connections.timeout.ms" was, I would probably guess that it was some kind of 
idle timeout on the connection as a whole.  I wouldn't think that it only 
applied to connection setup.  How about something like 
"connection.setup.timeout.ms"?

> Currently, when no nodes provided in --boostrap-server option is
> connected, the LeastLoadedNodeProvider will provide an unconnected
> node for the client. The Cluster class shuffled the nodes to balance the
> initial pressure and the LeastLoadedNodeProvider will always provide
> the same node, which is the last node after shuffling. Consequently, 
> though we may provide several bootstrap servers, the client not be able 
> to connect to the cluster if any of the servers in the --bootstrap-server 
> list 
> is offline.

Hmm... I can't follow what this paragraph is trying to say.  What's the 
connection between LeastLoadedProvider and TCP connection setup timeouts?

> ClusterConnectionStates will keep the index of the most recently 
> provided node. Meanwhile, a public API looks like below will be added 
> to simulate the round-robin node picking.

Again, this seems unrelated to the problem described in the motivation section.

I would really suggest creating a separate KIP for this, since it seems 
unrelated to the problem of TCP timeouts.  When KIPs try to do too many things 
at once, they often lose focus and get delayed.

> To Discuss
> 1. We can think about if we want to have different 
> connect.timeout.ms for different clients.
> 2. What would be the default value for connect.timeout.ms (is 10s
> too short?)
> 3. Should the Selector / IdleExpiryManager throw an exception 
> when the initial socket channel connection is timeout? Personally 
> I don't think we need as we will continue trying the rest of the 
> candidate nodes.

It's good to get feedback, but in general everything in a KIP is "To discuss," 
so we should not have a separate section titled "To discuss."  :) 

We also want the KIP to reflect what we actually decided on and implemented, so 
it shouldn't include open questions (unless they are questions about future 
work that is not covered in the scope of the KIP).

To come back to the timeout question, I think you should motivate your decision 
(and it does have to be decided here, not left as an open question), by 
explaining its relationship to other timeouts, and to the underlying TCP 
three-way handshake which it is putting an upper bound on.  10 seconds doesn't 
seem unreasonable in that context.

I believe that the default value of 127 seconds in Linux is set as high as it 
is to compensate for the fact that many applications do not retry connections 
if the initial establishment fails.  That isn't the case for Kafka, which 
motivates a lower default for us than for them.

Question 3 seems to be about whether a connection setup failure should result 
in the node getting moved to an error state (and hence not getting picked a 
second time by LeastLoadedNode, for example).  It seems pretty clear that a 
connection setup error is an error, right?  (Also, phrasing this as "should we 
throw an exception" is unnecessarily implementation specific.)

For rejected alternatives, we should explain why the KIP introduces a new 
timeout configuration rather than terminating an unsuccessful TCP connection 
setup attempt after request.timeout.ms has elapsed.

Also, the "Compatibility, Deprecation, and Migration Plan" should say that 
there is no impact (the section is just blank now)

Thanks again for the KIP.  This seems like it has been a gap in Kafka's error 
handling for a while, and it's good to see a proposal to fix it.

best,
Colin


On Mon, Apr 27, 2020, at 18:48, Cheng Tan wrote:
> Hi developers,
> 
> 
> I’m proposing KIP-601 to support configurable socket connection 
> timeout. 
> https://cwiki.apache.org/confluence/display/KAFKA/KIP-601%3A+Configurable+socket+connection+timeout
>  
> 
> 
> Currently, the initial socket connection timeout is depending on system 
> setting tcp_syn_retries. The actual 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. 
>   • The default value of