oleksiyk / kafka

Apache Kafka 0.9 client for Node
MIT License
297 stars 85 forks source link

Add possibility to specify socket timeout #188

Closed apalchys closed 7 years ago

apalchys commented 7 years ago

This PR adds socket timeout handler for kafka connection.

We faced with an issue when our service actively consumes data from Kafka in multiple node processes: The client "writes" to socket but does not get any event back. As result, promise is not resolved/rejected and we get a pending connection which stops consuming. I was not able to identify the exact root cause of the issue but I think it makes sense to set timeout for socket and add corresponding handler.
I put 30s as default socket timeout value. Let me know if you think it should be adjusted.

oleksiyk commented 7 years ago

I think we should preserve original behaviour and do not set any socket timeout by default, e.g. make options.socketTimeout set to 0 by default, not 30000

apalchys commented 7 years ago

@oleksiyk I thought about it too but as I see it is not possible to set connection options except host/post and ssl. Are you ok if I change _createConnection function to the following?:

return new Connection({
    host: host,
    port: port,
    ssl: this.options.ssl,
    connectionTimeout: this.options.connectionTimeout,
    socketTimeout: this.options.socketTimeout
});
oleksiyk commented 7 years ago

Yes, thats fine.

apalchys commented 7 years ago

@oleksiyk I've updated PR. Please take a look.