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.

Sign up for GitHub

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

Jump to bottom

SQL: request timeout is not honoured #72151

Closed
bpintea opened this issue Apr 23, 2021 · 5 comments · Fixed by #79360
Closed

SQL: request timeout is not honoured #72151

bpintea opened this issue Apr 23, 2021 · 5 comments · Fixed by #79360
Labels
:Analytics/SQL SQL querying >bug Team:QL (Deprecated) Meta label for query languages team

Comments

@bpintea
Copy link
Contributor

bpintea commented Apr 23, 2021

All requests, initial or subsequent paginations, use the page_timeout, whose value's either taken from the config/request or default.
Expected is to have the request_timeout config's value used for the initial request timeout.
@elasticmachine elasticmachine added the Team:QL (Deprecated) Meta label for query languages team label Apr 23, 2021
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-ql (Team:QL)

@bpintea
Copy link
Contributor Author

bpintea commented Apr 23, 2021

Loosely related #52207.

@bpintea
Copy link
Contributor Author

bpintea commented Apr 23, 2021

This parameter isn't shown into the translation either, worth looking into adding it.

@Luegg
Copy link
Contributor

Luegg commented Oct 5, 2021 •
edited
Loading

From what I've found in the docs, the expected behavior of the timeout parameters in SqlQueryRequest should have the following effects:

  • requestTimeout: equivalent to timeout in the _search API: "Specifies the period of time to wait for a response from each shard. If no response is received before the timeout expires, the request fails and returns an error." Currently, this parameter is used as the keepAlive configuration in scroll requests to retain the scroll context for the specified amount of time.
  • waitForCompletionTimeout: Time to wait until the request becomes an async query.
  • pageTimeout: How long to retain the search context for subsequent scroll requests (scroll in /_search). The docs are a bit unclear here, but I think this is the common use of this parameter in ODBC (see e.g. https://docs.microsoft.com/en-us/sql/odbc/microsoft/setting-options-programmatically-for-the-access-driver?view=sql-server-ver15 ). Currently, this parameter becomes the timeout for search requests triggered by the query.
  • keepAlive: How long to retain the query result for async (and stored sync) requests.

So, the current state seems to be that requestTimeout and pageTimeout are swapped in Querier.

@astefan
Copy link
Contributor

astefan commented Oct 5, 2021

This seems to be the case indeed @Luegg.

@Luegg Luegg mentioned this issue Oct 18, 2021
Merged
elasticsearchmachine pushed a commit that referenced this issue Oct 27, 2021
SQL: fix use of requestTimeout and pageTimeout query parameters (#79360)
9e66494 
Resolves #72151 The _sql endpoint offers a `page_timeout` parameter for
customizing how long scroll contexts should be kept open (if needed) and
a `request_timeout` parameter which the docs describe as "Timeout before
the request fails.". Currently, the value of the `page_timeout`
parameter is used as the `timeout` in subsequent _search requests and
not as the timeout in the `scroll` configuration. For the `scroll`
configuration, SQL uses the `request_timeout` parameter. This PR
addresses the issue by swapping the uses of `page_timeout` and
`request_timeout` in querier. Additionally, the PR removes some unused
artifacts that might have caused some confusion: - The `timeout` and
`keepAlive` fields in `Querier`. Instead, `Querier` directly uses the
according fields in `SqlConfiguration`. - The `SqlConfiguration`
parameter from `ScrollCursor.clear`, it's not used but required an
instance of `SqlConfiguration` with all default values. - One overloaded
constructor of `SqlConfiguration` that was only used for calling
`ScrollCursor.clear` (and some tests) and used default values for an
(arbitrary?) subset of the fields. - The fields related to async
requests in `SqlConfiguration`. I'm a bit unsure about this one but the
fields are never read and it does not seem like an SQL specific concern.
The whole creation of the async tasks is handled in
`TransportSqlQueryAction` and the downstream components do not require
the information.
Luegg pushed a commit to Luegg/elasticsearch that referenced this issue Oct 27, 2021
@Luegg
SQL: fix use of requestTimeout and pageTimeout query parameters (elas…
f024625 
…tic#79360)

Resolves elastic#72151 The _sql endpoint offers a `page_timeout` parameter for
customizing how long scroll contexts should be kept open (if needed) and
a `request_timeout` parameter which the docs describe as "Timeout before
the request fails.". Currently, the value of the `page_timeout`
parameter is used as the `timeout` in subsequent _search requests and
not as the timeout in the `scroll` configuration. For the `scroll`
configuration, SQL uses the `request_timeout` parameter. This PR
addresses the issue by swapping the uses of `page_timeout` and
`request_timeout` in querier. Additionally, the PR removes some unused
artifacts that might have caused some confusion: - The `timeout` and
`keepAlive` fields in `Querier`. Instead, `Querier` directly uses the
according fields in `SqlConfiguration`. - The `SqlConfiguration`
parameter from `ScrollCursor.clear`, it's not used but required an
instance of `SqlConfiguration` with all default values. - One overloaded
constructor of `SqlConfiguration` that was only used for calling
`ScrollCursor.clear` (and some tests) and used default values for an
(arbitrary?) subset of the fields. - The fields related to async
requests in `SqlConfiguration`. I'm a bit unsure about this one but the
fields are never read and it does not seem like an SQL specific concern.
The whole creation of the async tasks is handled in
`TransportSqlQueryAction` and the downstream components do not require
the information.
elasticsearchmachine pushed a commit that referenced this issue Oct 27, 2021
SQL: fix use of requestTimeout and pageTimeout query parameters (#79360…
9bf3a32 
…) (#79915)

Resolves #72151 The _sql endpoint offers a `page_timeout` parameter for
customizing how long scroll contexts should be kept open (if needed) and
a `request_timeout` parameter which the docs describe as "Timeout before
the request fails.". Currently, the value of the `page_timeout`
parameter is used as the `timeout` in subsequent _search requests and
not as the timeout in the `scroll` configuration. For the `scroll`
configuration, SQL uses the `request_timeout` parameter. This PR
addresses the issue by swapping the uses of `page_timeout` and
`request_timeout` in querier. Additionally, the PR removes some unused
artifacts that might have caused some confusion: - The `timeout` and
`keepAlive` fields in `Querier`. Instead, `Querier` directly uses the
according fields in `SqlConfiguration`. - The `SqlConfiguration`
parameter from `ScrollCursor.clear`, it's not used but required an
instance of `SqlConfiguration` with all default values. - One overloaded
constructor of `SqlConfiguration` that was only used for calling
`ScrollCursor.clear` (and some tests) and used default values for an
(arbitrary?) subset of the fields. - The fields related to async
requests in `SqlConfiguration`. I'm a bit unsure about this one but the
fields are never read and it does not seem like an SQL specific concern.
The whole creation of the async tasks is handled in
`TransportSqlQueryAction` and the downstream components do not require
the information.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
:Analytics/SQL SQL querying >bug Team:QL (Deprecated) Meta label for query languages team
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

SQL: fix use of requestTimeout and pageTimeout query parameters Luegg/elasticsearch
4 participants
@astefan @Luegg @bpintea @elasticmachine