Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add keep-alive support to socket_listener & socket_writer #2697

Merged
merged 1 commit into from
Apr 24, 2017
Merged

add keep-alive support to socket_listener & socket_writer #2697

merged 1 commit into from
Apr 24, 2017

Conversation

phemmer
Copy link
Contributor

@phemmer phemmer commented Apr 22, 2017

Closes #2635

Unfortunately I cannot think of any way to add unit tests here. There's no ability to query a socket for its current keep-alive setting. The only way to trigger a socket shut down due to keep-alive is to kill a host, or sever the network. And the only way to observe a keep-alive probe is a packet capture.
{rock} (us) {hard place}

Required for all PRs:

  • CHANGELOG.md updated (we recommend not updating this until the PR has been approved by a maintainer)
  • Sign CLA (if not already signed)

ssl.AddError(fmt.Errorf("unable to enable keep alive: %s", err))
} else {
if err := tcpc.SetKeepAlivePeriod(ssl.KeepAlivePeriod.Duration); err != nil {
ssl.AddError(fmt.Errorf("unable to set keep alive period: %s", err))
Copy link
Contributor Author

@phemmer phemmer Apr 22, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so
    much
        nesting

The only way I can think of to flatten this out a bit would be to break the code out into a function and use returns. Not sure how I feel about that.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you could probably do something like func setKeepAlive(*net.TCPConn, time.Duration) error, might have to add the service address context to the error outside the function. Would probably clean up the else branch when a duration is non-zero. Let me know if you want to make changes or not.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, that was along the lines of the idea, just thought it might be a little odd breaking it out. But I went and implemented it (and pushed to PR) and I think it's clean.

@danielnelson danielnelson added this to the 1.3.0 milestone Apr 24, 2017
@danielnelson danielnelson added the feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin label Apr 24, 2017
@danielnelson danielnelson merged commit 8f5cd6c into influxdata:master Apr 24, 2017
vlamug pushed a commit to vlamug/telegraf that referenced this pull request May 30, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants