-
Notifications
You must be signed in to change notification settings - Fork 8.6k
[performance] continuous polling #256564
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
[performance] continuous polling #256564
Changes from all commits
1d2ea99
cd165c6
295b9d8
07f1614
79fdfdf
3a1a411
ab6d7b3
78976f3
80f4390
8c4b61a
ef5e33f
25aaafa
0e902f6
f9f7873
b9eebf8
ae81692
a65ef09
13a75ee
1fba93c
73eca8f
6df22f3
fe33d20
f428389
33ca667
6b0ca9c
27188b8
fd208c6
fd32d74
068ec68
2ec331f
6827aa3
2c6a4b6
6a3f881
1abeb1e
50a1d18
2d5e914
8e4eb8e
701c4c6
3a36579
fa87041
f01cc85
3322067
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -16,7 +16,6 @@ import { | |
| fromEvent, | ||
| switchMap, | ||
| takeUntil, | ||
| takeWhile, | ||
| tap, | ||
| throwError, | ||
| timer, | ||
|
|
@@ -49,12 +48,12 @@ export const pollSearch = <Response extends IKibanaSearchResponse>( | |
| }; | ||
|
|
||
| return defer(() => { | ||
| const startTime = Date.now(); | ||
|
|
||
| if (abortSignal?.aborted) { | ||
| throw new AbortError(); | ||
| } | ||
|
|
||
| const startTime = Date.now(); | ||
|
|
||
| const safeCancel = () => | ||
| cancel?.().catch((e) => { | ||
| console.error(e); // eslint-disable-line no-console | ||
|
|
@@ -69,16 +68,17 @@ export const pollSearch = <Response extends IKibanaSearchResponse>( | |
| ); | ||
|
|
||
| return from(search()).pipe( | ||
| expand(() => { | ||
| expand((response) => { | ||
| const elapsedTime = Date.now() - startTime; | ||
| return timer(getPollInterval(elapsedTime)).pipe(switchMap(() => search())); | ||
| return isRunningResponse(response) | ||
| ? timer(getPollInterval(elapsedTime)).pipe(switchMap(() => search())) | ||
| : EMPTY; | ||
|
Comment on lines
+73
to
+75
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Curious as to why we use this vs.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. When I made the changes in this PR, the way it was set up before was making an extra request at the end after the results were already available. But I am not an rxjs guru so open to suggestions |
||
| }), | ||
| tap((response) => { | ||
| if (isAbortResponse(response)) { | ||
| throw new AbortError(); | ||
| } | ||
| }), | ||
| takeWhile<Response>(isRunningResponse, true), | ||
| takeUntil<Response>(aborted$) | ||
| ); | ||
| }); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -14,7 +14,11 @@ export const ENHANCED_ES_SEARCH_STRATEGY = 'ese'; | |
| export interface IAsyncSearchOptions extends SearchSourceSearchOptions { | ||
| /** | ||
| * The number of milliseconds to wait between receiving a response and sending another request | ||
| * If not provided, then a default 1 second interval with back-off up to 5 seconds interval is used | ||
| * If not provided, then it defaults to 0 (no wait time) | ||
| */ | ||
| pollInterval?: number; | ||
| /** | ||
| * The length of time to wait for results before initiating a new poll request. | ||
| */ | ||
| pollLength?: string; | ||
|
Comment on lines
+20
to
+23
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I tried to understand where this is used but I couldn't make out code that makes use of this. As a far as I understand these are per-request options and I don't see
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
take a look at https://github.com/elastic/kibana/pull/256564/changes#diff-238b1c61769eb4806c1ef55dc1afe0c04f79740d0753d307fdf89c6b9e5c9799R71, and also the diagram in the PR description. BTW, this is search code so vis team review is not required (vis team owns esaggs) even though the vis team are technically co-owners of the data plugin. I say this not because I don't welcome it if you'd still like to review but just to take the pressure off :)
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for the context! Looking at the comment in // don't set or override user-configured pollLength, it will be applied server-side
!this.deps.searchConfig.asyncSearch.pollLength && this.protocolSupportsMultiplexing
? DEFAULT_MULTIPLEXING_POLL_LENGTH
: undefined,It looks like
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The reason we have to have client-side logic around this is that we need to react to the protocol the browser is using for the next hop. You can have a setup where the server is on HTTP/1 and the browser is on HTTP/2 with a proxy in the middle. In that case, we should optimize for multiplexing. However, if the user has explicitly configured a It's all in support of this setting configuration flow: flowchart TD
Start[Start] -->|Polling GET| ClientConfig{search.asyncSearch.pollLength set?}
ClientConfig -->|Yes - Number| UseClientConfig[Use config value]
ClientConfig -->|No| Multiplex{HTTP/2 or<br/>HTTP/3? - Needs to be checked client-side}
Multiplex -->|Yes| Use30s[Use 30000ms]
Multiplex -->|No| Undefined[Omitted - functionally zero]
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks! Now I got it, I failed to connect the dots between that flowchart and the code :)
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't blame you! I updated the diagram in the description to hopefully be more clear. LMK if you have any further suggestions |
||
| } | ||
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.
does this mean that we can get incremental results while the search is in progress?
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.
nit: I know the Elasticsearch naming could be taken either way, but we are really talking about "partial results," not "incremental results."
But to your question, there should be no practical change in the behavior from the old
retrieveResultsparam.retrieveResultshas always meant, "retrieve all results that are available whether or not the search is complete."The change is that instead of switching endpoints from the status to the
GETendpoint whenreturnIntermediateResultsis set, we control the behavior with a param to theGETendpoint.