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

[ES|QL] Force ES client timeout after 2 minutes #168929

Merged
merged 9 commits into from
Oct 18, 2023
Merged

[ES|QL] Force ES client timeout after 2 minutes #168929

Changes from 7 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
f8e002a
[ES|QL] Force es client timeout
stratoula Oct 16, 2023
5e82432
Adds some comments
stratoula Oct 16, 2023
920044f
Fixes typo
stratoula Oct 16, 2023
1907617
Merge branch 'main' into settings-esql
stratoula Oct 16, 2023
7e9ae73
Merge branch 'main' into settings-esql
stratoula Oct 16, 2023
7b06ea5
Abort after 2mins
stratoula Oct 17, 2023
1fa2007
Merge branch 'main' into settings-esql
stratoula Oct 17, 2023
238d839
Cleanup
stratoula Oct 18, 2023
b8632d1
Merge branch 'main' into settings-esql
stratoula Oct 18, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 17 additions & 1 deletion src/plugins/data/server/search/strategies/esql_search/esql_search_strategy.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,8 @@ import type { Logger } from '@kbn/core/server';
import { getKbnServerError, KbnServerError } from '@kbn/kibana-utils-plugin/server';
import type { ISearchStrategy } from '../../types';

const ES_TIMEOUT_IN_MS = 120000;

export const esqlSearchStrategyProvider = (
logger: Logger,
useInternalUser: boolean = false
Expand All @@ -23,6 +25,17 @@ export const esqlSearchStrategyProvider = (
* @returns `Observable<IEsSearchResponse<any>>`
*/
search: (request, { abortSignal, ...options }, { esClient, uiSettingsClient }) => {
const abortController = new AbortController();
// We found out that there are cases where we are not aborting correctly
// For this reasons we want to manually cancel he abort signal after 2 mins

abortSignal?.addEventListener('abort', () => {
abortController.abort();
});

// Also abort after two mins
setTimeout(() => abortController.abort(), ES_TIMEOUT_IN_MS);

// Only default index pattern type is supported here.
// See ese for other type support.
if (request.indexType) {
Expand All @@ -41,8 +54,11 @@ export const esqlSearchStrategyProvider = (
},
},
{
signal: abortSignal,
signal: abortController.signal,
meta: true,
// we don't want the ES client to retry (default value is 3)
maxRetries: 0,
requestTimeout: ES_TIMEOUT_IN_MS,
Copy link
Member

Choose a reason for hiding this comment

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

I think if we're already manually timing out (using the AbortController), we don't need to set this requestTimeout value here. My concern is that if it has already been overridden to something lower than 2 mins, then we will be overriding it here to actually allow longer-than-configured queries.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are right Lukas, I just forgot to remove it :)

}
);
return {
Expand Down