Adding TCP keepalives support for the broker's connection #408
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| @@ -12,6 +12,10 @@ type Config struct { | |||
| DialTimeout time.Duration // How long to wait for the initial connection to succeed before timing out and returning an error (default 30s). | |||
| ReadTimeout time.Duration // How long to wait for a response before timing out and returning an error (default 30s). | |||
| WriteTimeout time.Duration // How long to wait for a transmit to succeed before timing out and returning an error (default 30s). | |||
|
|
|||
| // KeepAlive specifies the keep-alive period for an active network connection. | |||
| // If zero, keep-alives are not enabled. | |||
There was a problem hiding this comment.
"not enabled" == "disabled"
Also, please mention that the default is 0 (disabled).
|
A few nits, but generally LGTM. You're welcome to add a line to the changelog if you wish. |
| @@ -78,6 +79,7 @@ func TestClientDoesntCachePartitionsForTopicsWithErrors(t *testing.T) { | |||
| seedBroker.Returns(metadataResponse) | |||
|
|
|||
| config := NewConfig() | |||
| config.Net.KeepAlive = 12 * time.Millisecond | |||
There was a problem hiding this comment.
Not sure whether this is a useful addition given that we don't actually test it.
|
LGTM apart from the nitpicks. |
|
@wvanbergen or @eapache - Does the new config setting need a validator ? https://github.com/Shopify/sarama/blob/master/config.go#L187 The other nits were addressed. :) |
|
Assuming that a setting <0 is incoherent then yes, a validator is important - good catch! |
eapache
added a commit
that referenced
this pull request
Apr 6, 2015
Adding TCP keepalives support for the broker's connection
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Addresses issue #407
Our Google Compute Engine applications have a couple of under utilized topics/partitions that only receive messages once an hour or so. Google Compute Engine's networking automatically kills any connection thats been idle for more than 10mins. This causes the Sarama clients on GCE to return this error
read tcp 10.10.10.10:9093: i/o timeout.From Google Compute Engine's Docs:
Keepalive Refs:
PS thanks for all the hard work! You guys rock!