-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
Support a timeout for running queries in the query manager #6081
Conversation
7680d76
to
0765537
Compare
@@ -19,6 +19,9 @@ const ( | |||
// DefaultMaxRemoteWriteConnections is the maximum number of open connections | |||
// that will be available for remote writes to another host. | |||
DefaultMaxRemoteWriteConnections = 3 | |||
|
|||
// DefaultQueryTimeout is the default timeout for executing a query. | |||
DefaultQueryTimeout = 5 * time.Minute |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should default to no timeout unless it's configured to be set.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I changed the timeout to 0 seconds which should result in no timeout.
43f972c
to
63fd3aa
Compare
Include an interrupt iterator at the top level to interrupt the fill iterator if it is producing too many points. Fixes #6075.
63fd3aa
to
79fe449
Compare
@@ -8,6 +8,7 @@ | |||
- [#5939](https://github.com/influxdata/influxdb/issues/5939): Support viewing and killing running queries. | |||
- [#6073](https://github.com/influxdata/influxdb/pulls/6073): Iterator stats | |||
- [#6079](https://github.com/influxdata/influxdb/issues/6079): Limit the maximum number of concurrent queries. | |||
- [#6075](https://github.com/influxdata/influxdb/issues/6075): Limit the maximum running time of a query. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this be [#6081](https://github.com/influxdata/influxdb/pull/6081)
? Looks like some previous ones in that list aren't quite right either.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a bit confused with how we do this and we should probably standardize it. I've been using the issue that the PR fixes while some include the PR. If there is no issue for the PR, then I include the PR link.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jsternberg That's correct. Use the issue fixed. If there is no issue, use the PR.
LGTM 👍 |
Include an interrupt iterator at the top level to interrupt the fill
iterator if it is producing too many points.
Fixes #6075.