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

feat(http): add config parameters for HTTP timeouts #20971

Merged
merged 5 commits into from
Mar 16, 2021

Conversation

danxmoran
Copy link
Contributor

@danxmoran danxmoran commented Mar 16, 2021

Closes #20769

I left the defaults for the read- and write-timeout parameters as 0, as I assume those will be workload-dependent.

Flag: "http-bind-address",
Default: o.HttpBindAddress,
Desc: "bind address for the REST HTTP API",
},
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved this to be with the other HTTP params

@danxmoran danxmoran changed the base branch from dm-tls-strict-ciphers-20762 to master March 16, 2021 14:45
@danxmoran danxmoran force-pushed the dm-influxd-http-timeouts-20769 branch from 21dbeec to a076c87 Compare March 16, 2021 14:47
CHANGELOG.md Show resolved Hide resolved
cmd/influxd/launcher/cmd.go Show resolved Hide resolved
@lesam
Copy link
Contributor

lesam commented Mar 16, 2021

Isn't IdleTimeout an implicit upper bound on the Read/Write timeouts?

Also how does this interact with the query timeout?

@danxmoran
Copy link
Contributor Author

As I understand it, IdleTimeout is separate from read/write. It tracks the time between when a response is sent and the next request arrives.

I left read&write timeouts un-set to avoid breaking existing workloads that write huge payloads / run long-running queries. I could set them to something huge like time.Hour if we're comfortable assuming nobody would ever want to run a legit request that takes that long.

As far as I've seen, 2.x doesn't have a specific query-timeout (or any other request-specific timeouts). It would be good to restore those options at some point.

@danxmoran
Copy link
Contributor Author

@lesam
Copy link
Contributor

lesam commented Mar 16, 2021

You're right, I was looking at the wrong set of docs.

Copy link
Contributor

@lesam lesam left a comment

Choose a reason for hiding this comment

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

I agree explicit query timeouts would be nice - but we don't want too many knobs that interact in weird ways.

@danxmoran danxmoran merged commit c5edd90 into master Mar 16, 2021
@danxmoran danxmoran deleted the dm-influxd-http-timeouts-20769 branch March 16, 2021 20:54
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.

Support setting HTTP timeouts for influxd
3 participants