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

cmd/tsdbrelay: Drain relay response so that connection is eligible for reuse. #2034

Closed
wants to merge 1 commit into from

Conversation

alienth
Copy link
Contributor

@alienth alienth commented Mar 12, 2017

tsdbrelay is not reusing connections for its relay calls. DefaultTransport will not allow a connection to be reused unless the body has been read. This change will use the common pattern of reading 512 bytes from the resp.Body to try and ensure the connection is available for reuse. This is intentionally capped at 512 bytes to avoid waiting for cases of large responses.

Some discussion on this if curious: https://groups.google.com/forum/#!search/golang$20json$20decode$20$20io.Copy/golang-nuts/4Rr8BYVKrAI/ZrJJFTNleekJ

Confirmed in testing that this addresses lack of connection reuse.

Partially addresses #2018.

@kylebrandt @captncraig @gbrayut

@alienth alienth changed the title cmd/tsdbrelay: Drain relay response so that connection is elegible for reuse. cmd/tsdbrelay: Drain relay response so that connection is eligible for reuse. Mar 12, 2017
@gbrayut
Copy link
Contributor

gbrayut commented Mar 13, 2017

The code has separate paths for bosun, relays, and metadata... do we need the same io.CopyN added to these:

https:/alienth/bosun/blob/8b2135b1a85f9dd075ae222482ca3cbf0cd7cb8f/cmd/tsdbrelay/main.go#L253

https:/alienth/bosun/blob/8b2135b1a85f9dd075ae222482ca3cbf0cd7cb8f/cmd/tsdbrelay/main.go#L389

@alienth
Copy link
Contributor Author

alienth commented Mar 13, 2017

@gbrayut updated.

Copy link
Contributor

@gbrayut gbrayut left a comment

Choose a reason for hiding this comment

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

Tested on ny-gbraylx01
LGTM

@gbrayut
Copy link
Contributor

gbrayut commented Mar 13, 2017

Found some more cases in metadata/collect/etc where this occurs. This PR is superseded by #2036

@gbrayut gbrayut closed this Mar 13, 2017
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