Skip to content

Re-enable http keepalive on remote storage#3171

Closed
bboreham wants to merge 2 commits intoprometheus:release-1.7from
bboreham:remote-keepalive-1.7
Closed

Re-enable http keepalive on remote storage#3171
bboreham wants to merge 2 commits intoprometheus:release-1.7from
bboreham:remote-keepalive-1.7

Conversation

@bboreham
Copy link
Copy Markdown
Member

Cherry-pick of #3160 as candidate for the 1.7.2 release

Allow HTTP keep-alive setting to be overridden in config, and default it ON for remote storage read/write

// TLSConfig to use to connect to the targets.
TLSConfig TLSConfig `yaml:"tls_config,omitempty"`
// If set, override whether to use HTTP KeepAlive - scraping defaults OFF, remote read/write defaults ON
KeepAlive *bool `yaml:"keep_alive,omitempty"`
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I don't think this should be configurable.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Maybe that should be a comment to #2498 ?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

That issue isn't about remote storage

Copy link
Copy Markdown
Member Author

@bboreham bboreham Sep 14, 2017

Choose a reason for hiding this comment

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

That line makes it configurable for scraping too.

Are you saying it should be non-configurable solely for remote storage? (which would be a change here)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think it should be non-configurable generally.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

OK, so your comment does belong on #2498.

Also you might want to revert this change off master.

@bboreham
Copy link
Copy Markdown
Member Author

Any further thoughts on this? Obviously not still a candidate for 1.7.2.

@bboreham
Copy link
Copy Markdown
Member Author

bboreham commented Oct 5, 2017

#3173 replaces this

@bboreham bboreham closed this Oct 5, 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