-
Notifications
You must be signed in to change notification settings - Fork 24.3k
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
SQL: fix use of requestTimeout and pageTimeout query parameters (#79360)
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.
- Loading branch information
Lukas Wegmann
committed
Oct 27, 2021
1 parent
ee1f71d
commit 9e66494
Showing
22 changed files
with
179 additions
and
109 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
98 changes: 98 additions & 0 deletions
98
...c/internalClusterTest/java/org/elasticsearch/xpack/sql/action/SqlSearchPageTimeoutIT.java
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,98 @@ | ||
| /* | ||
| * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one | ||
| * or more contributor license agreements. Licensed under the Elastic License | ||
| * 2.0; you may not use this file except in compliance with the Elastic License | ||
| * 2.0. | ||
| */ | ||
|
|
||
| package org.elasticsearch.xpack.sql.action; | ||
|
|
||
| import org.elasticsearch.action.index.IndexRequest; | ||
| import org.elasticsearch.action.search.SearchPhaseExecutionException; | ||
| import org.elasticsearch.action.support.WriteRequest; | ||
| import org.elasticsearch.common.settings.Settings; | ||
| import org.elasticsearch.core.TimeValue; | ||
| import org.elasticsearch.search.SearchContextMissingException; | ||
| import org.elasticsearch.search.SearchService; | ||
|
|
||
| import java.util.Arrays; | ||
| import java.util.concurrent.TimeUnit; | ||
|
|
||
| import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertAcked; | ||
| import static org.hamcrest.Matchers.contains; | ||
| import static org.hamcrest.Matchers.instanceOf; | ||
|
|
||
| public class SqlSearchPageTimeoutIT extends AbstractSqlIntegTestCase { | ||
|
|
||
| @Override | ||
| protected Settings nodeSettings(int nodeOrdinal, Settings otherSettings) { | ||
| Settings.Builder settings = Settings.builder().put(super.nodeSettings(nodeOrdinal, otherSettings)); | ||
| // use static low keepAlive interval to ensure obsolete search contexts are pruned soon enough | ||
| settings.put(SearchService.KEEPALIVE_INTERVAL_SETTING.getKey(), TimeValue.timeValueMillis(200)); | ||
| return settings.build(); | ||
| } | ||
|
|
||
| public void testSearchContextIsCleanedUpAfterPageTimeoutForHitsQueries() throws Exception { | ||
| setupTestIndex(); | ||
|
|
||
| SqlQueryResponse response = new SqlQueryRequestBuilder(client(), SqlQueryAction.INSTANCE).query("SELECT field FROM test") | ||
| .fetchSize(1) | ||
| .pageTimeout(TimeValue.timeValueMillis(100)) | ||
| .get(); | ||
|
|
||
| assertEquals(1, response.size()); | ||
| assertTrue(response.hasCursor()); | ||
| assertEquals(1, getNumberOfSearchContexts()); | ||
|
|
||
| assertBusy(() -> assertEquals(0, getNumberOfSearchContexts()), 3, TimeUnit.SECONDS); | ||
|
|
||
| SearchPhaseExecutionException exception = expectThrows( | ||
| SearchPhaseExecutionException.class, | ||
| () -> new SqlQueryRequestBuilder(client(), SqlQueryAction.INSTANCE).cursor(response.cursor()).get() | ||
| ); | ||
|
|
||
| assertThat(Arrays.asList(exception.guessRootCauses()), contains(instanceOf(SearchContextMissingException.class))); | ||
| } | ||
|
|
||
| public void testNoSearchContextForAggregationQueries() throws InterruptedException { | ||
| setupTestIndex(); | ||
|
|
||
| SqlQueryResponse response = new SqlQueryRequestBuilder(client(), SqlQueryAction.INSTANCE).query( | ||
| "SELECT COUNT(*) FROM test GROUP BY field" | ||
| ).fetchSize(1).pageTimeout(TimeValue.timeValueMillis(100)).get(); | ||
|
|
||
| assertEquals(1, response.size()); | ||
| assertTrue(response.hasCursor()); | ||
| assertEquals(0, getNumberOfSearchContexts()); | ||
|
|
||
| Thread.sleep(1000); | ||
|
|
||
| // since aggregation queries do not have a stateful search context, scrolling is still possible after page_timeout | ||
| response = new SqlQueryRequestBuilder(client(), SqlQueryAction.INSTANCE).cursor(response.cursor()).get(); | ||
|
|
||
| assertEquals(1, response.size()); | ||
| } | ||
|
|
||
| private void setupTestIndex() { | ||
| assertAcked(client().admin().indices().prepareCreate("test").get()); | ||
| client().prepareBulk() | ||
| .add(new IndexRequest("test").id("1").source("field", "bar")) | ||
| .add(new IndexRequest("test").id("2").source("field", "baz")) | ||
| .setRefreshPolicy(WriteRequest.RefreshPolicy.IMMEDIATE) | ||
| .get(); | ||
| ensureYellow("test"); | ||
| } | ||
|
|
||
| private long getNumberOfSearchContexts() { | ||
| return client().admin() | ||
| .indices() | ||
| .prepareStats("test") | ||
| .clear() | ||
| .setSearch(true) | ||
| .get() | ||
| .getIndex("test") | ||
| .getTotal() | ||
| .getSearch() | ||
| .getOpenContexts(); | ||
| } | ||
| } |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.