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

tcp_sink: don't hang on flush if reconnecting #92

Merged
merged 4 commits into from
Apr 1, 2020
Merged

Conversation

charlievieth
Copy link
Contributor

@charlievieth charlievieth commented Mar 28, 2020

This fixes a bug where the TCP Sink would hang indefinitely on Flush()
if it was unable to reconnect.

It also adds the SinkOption interface for configuring the TCP Sink,
previously could only be done via environment variables (it still
defaults to using env vars). This changes the function signature of
NewTCPStatsdSink(), but the change should not be breaking since the new
"opts" argument is variadic.

This also significantly increases the code coverage of the TCP Sink
tests.

@charlievieth charlievieth force-pushed the cev/flush branch 2 times, most recently from b55deff to cf95fa4 Compare March 28, 2020 08:05
twoism
twoism previously approved these changes Mar 28, 2020
This fixes a bug where the TCP Sink would hang indefinitely on Flush()
if it was unable to reconnect.

It also adds the SinkOption interface for configuring the TCP Sink,
previously could only be done via environment variables (it still
defaults to using env vars). This changes the function signature of
NewTCPStatsdSink(), but the change should not be breaking since the new
"opts" argument is variadic.

This also significantly increases the code coverage of the TCP Sink
tests.
@charlievieth
Copy link
Contributor Author

charlievieth commented Apr 1, 2020

Failing on go1.12 due to a subtle timing issue with the test. That I can't replicate locally.

@charlievieth charlievieth merged commit 079323d into master Apr 1, 2020
@charlievieth charlievieth deleted the cev/flush branch April 7, 2020 19:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants