Skip to content
This repository has been archived by the owner on Nov 8, 2022. It is now read-only.

Adds IdleConnTimeout to transport in client #1355

Merged
merged 1 commit into from
Nov 21, 2016

Conversation

IRCody
Copy link
Contributor

@IRCody IRCody commented Nov 16, 2016

This PR is a continuation of the discussion from #1324. These changes require go1.7+ and I wanted to move the discussion around which go versions to support instead of blocking that PR.

Summary of changes:

  • Adds a Connection timeout to client transports.

@intelsdi-x/snap-maintainers

@kindermoumoute
Copy link
Contributor

With Go 1.8 coming very soon, it's a good time for dev to switch to 1.7.3.

Copy link
Contributor

@candysmurf candysmurf left a comment

Choose a reason for hiding this comment

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

great addition. Maybe after go1.6+ is not supported?

@@ -104,9 +105,11 @@ func Username(u string) metaOp {
var (
secureTransport = &http.Transport{
TLSClientConfig: &tls.Config{InsecureSkipVerify: false},
IdleConnTimeout: time.Second,
Copy link
Contributor

Choose a reason for hiding this comment

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

do you like to make this option configurable?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not really. What scenario do you see where it would be beneficial for a user to configure this?

@nanliu
Copy link
Contributor

nanliu commented Nov 18, 2016

Can you drop the go 1.6.x test matrix in .travis.yml, and bump to go 1.7.3 so this can be considered for merging?

@IRCody IRCody force-pushed the transport branch 3 times, most recently from fa804c9 to 65e0449 Compare November 18, 2016 22:21
@candysmurf
Copy link
Contributor

@IRCody, please rebase.

@kindermoumoute kindermoumoute merged commit f827263 into intelsdi-x:master Nov 21, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants