Hi Ewen, thanks for the comments. Very good points! Please see replies inline.
On 4/13/15, 11:19 PM, "Ewen Cheslack-Postava" <e...@confluent.io> wrote: >Jiangjie, > >Great start. I have a couple of comments. > >Under the motivation section, is it really true that the request will >never >be completed? Presumably if the broker goes down the connection will be >severed, at worst by a TCP timeout, which should clean up the connection >and any outstanding requests, right? I think the real reason we need a >different timeout is that the default TCP timeouts are ridiculously long >in >this context. Yes, when broker is completely down the request should be cleared as you said. The case we encountered looks like the broker was just not responding but TCP connection was still alive though. > >My second question is about whether this is the right level to tackle the >issue/what user-facing changes need to be made. A related problem came up >in https://issues.apache.org/jira/browse/KAFKA-1788 where producer records >get stuck indefinitely because there's no client-side timeout. This KIP >wouldn't fix that problem or any problems caused by lack of connectivity >since this would only apply to in flight requests, which by definition >must >have been sent on an active connection. > >I suspect both types of problems probably need to be addressed separately >by introducing explicit timeouts. However, because the settings introduced >here are very much about the internal implementations of the clients, I'm >wondering if this even needs to be a user-facing setting, especially if we >have to add other timeouts anyway. For example, would a fixed, generous >value that's still much shorter than a TCP timeout, say 15s, be good >enough? If other timeouts would allow, for example, the clients to >properly >exit even if requests have not hit their timeout, then what's the benefit >of being able to configure the request-level timeout? That is a very good point. We have three places that we might be able to enforce timeout for a message send: 1. Before append to accumulator - handled by metadata timeout on per message level. 2. Batch of messages inside accumulator - no timeout mechanism now. 3. Request of batches after messages leave the accumulator - we have a broker side timeout but no client side timeout for now. My current proposal only address (3) but not (2). Honestly I do not have a very clear idea about what should we do with (2) right now. But I am with you that we should not expose too many configurations to users. What I am thinking now to handle (2) is when user call send, if we know that a partition is offline, we should throw exception immediately instead of putting it into accumulator. This would protect further memory consumption. We might also want to fail all the batches in the dequeue once we found a partition is offline. That said, I feel timeout might not be quite applicable to (2). Do you have any suggestion on this? > >I know we have a similar setting, max.in.flights.requests.per.connection, >exposed publicly (which I just discovered is missing from the new producer >configs documentation). But it looks like the new consumer is not exposing >that option, using a fixed value instead. I think we should default to >hiding these implementation values unless there's a strong case for a >scenario that requires customization. For producer, max.in.flight.requests.per.connection really matters. If people do not want to have reorder of messages, they have to use max.in.flight.requests.per.connection=1. On the other hand, if throughput is more of a concern, it could be set to higher. For the new consumer, I checked the value and I am not sure if the hard coded max.in.flight.requests.per.connection=100 is the right value. Without the response to the previous request, what offsets should be put into the next fetch request? It seems to me the value will be one natively regardless of the setting unless we are sending fetch request to different partitions, which does not look like the case. Anyway, it looks to be a separate issue orthogonal to the request timeout. > >In other words, since the only user-facing change was the addition of the >setting, I'm wondering if we can avoid the KIP altogether by just choosing >a good default value for the timeout. The problem is that we have a server side request timeout exposed as a public configuration. We cannot set the client timeout smaller than that value, so a hard coded value probably won¹t work here. > >-Ewen > >On Mon, Apr 13, 2015 at 2:35 PM, Jiangjie Qin <j...@linkedin.com.invalid> >wrote: > >> Hi, >> >> I just created a KIP to add a request timeout to NetworkClient for new >> Kafka clients. >> >> >>https://cwiki.apache.org/confluence/display/KAFKA/KIP-19+-+Add+a+request+ >>timeout+to+NetworkClient >> >> Comments and suggestions are welcome! >> >> Thanks. >> >> Jiangjie (Becket) Qin >> >> > > >-- >Thanks, >Ewen
