[Background search] Change polling behavior#244760
Conversation
|
Pinging @elastic/kibana-data-discovery (Team:DataDiscovery) |
lukasolson
left a comment
There was a problem hiding this comment.
I think we still want to poll until all pending search requests are completed, otherwise we can get into a situation like this: A dashboard with two panels, one with a search request that takes 5 seconds, another that takes 10 minutes. The first completes after 5s, but doesn't continue polling. After 1 minute of not polling, this gets cleaned up and so if we send to background afterwards, there will be errors.
In practice this doesn't seem to actually happen (from my tests), and I think it's because ES actually only cleans up completed searches every hour. But I think it's still better to be safe than rely on an undocumented behavior in ES.
What are your thoughts?
that sounds good, i'll update to keep the polling until everything has finished in the search session |
|
|
||
| const schedulePollSearches = () => { | ||
| return timer(KEEP_ALIVE_COMPLETED_SEARCHES_INTERVAL).pipe( | ||
| return interval(KEEP_ALIVE_COMPLETED_SEARCHES_INTERVAL).pipe( |
There was a problem hiding this comment.
i've changed it from timer + repeat to just use interval
There was a problem hiding this comment.
I'm curious, does timer + repeat wait until the request is complete before scheduling the next poll? I'm slightly worried that interval will allow multiple requests to happen at the same time in a slow network.
There was a problem hiding this comment.
I checked that now and you are right, interval doesn't wait for the promises to resolve but timer + repeat do
| takeUntil(this.disableSaveAfterSearchesExpire$.pipe(filter((disable) => disable))), | ||
| takeUntil(stopOnFinishedState$) |
There was a problem hiding this comment.
this is the main change, now on top of the existing behavior we stop when the search session is in a completed status
There was a problem hiding this comment.
I think we can also clean up some more from the SessionService at this point - do we still need the disableSaveAfterSearchesExpire$ observable for anything? Which I think also means we can get rid of the notTouchedTimeout (from that file as well as deprecate and/or remove the config). Let me know if you can think of something I'm missing.
sounds good - do we need to do anything special to remove the config or should we just go ahead and remove it? updated in 70438ea |
lukasolson
left a comment
There was a problem hiding this comment.
Code looks good, but when I'm running it, it looks like shorter requests aren't polled while others are still in progress. For example, if I have two panels on a dashboard, one that stalls for 1s, and another that stalls for 5m, I don't see the 1s one being polled after it completes.
| }), | ||
| repeat(), | ||
| takeUntil(this.disableSaveAfterSearchesExpire$.pipe(filter((disable) => disable))) | ||
| takeUntil(pollingError$.pipe(filter((hasError) => hasError))), |
There was a problem hiding this comment.
Nit:
| takeUntil(pollingError$.pipe(filter((hasError) => hasError))), | |
| takeUntil(pollingError$.pipe(filter(Boolean))), |
yeah, i've seen this one specifically happen in dashboards for all the scenarios, not only on this branch but on main too, somehow it doesn't poll because it goes through this condition i don't know if it was behaving like this before, i guess yes, but right now we try to create the polling subscription before we enable the storage for dashboards, that means that it automatically opts-out of the polling |
| stopSearchSessionIntegration = startDashboardSearchSessionIntegration( | ||
| { | ||
| ...dashboardApi, | ||
| searchSessionId$, | ||
| }, | ||
| dashboardInternalApi, | ||
| searchSessionSettings, | ||
| (searchSessionId: string) => searchSessionId$.next(searchSessionId), | ||
| searchSessionGenerationInProgress$ | ||
| ); |
There was a problem hiding this comment.
moved this before the continue/start - when the session id changes we kick in some polling to keep searches alive while others are still running, but for that we need to have called enableStorage before changing the id.
It was happening the other way around here, this call that I moved is the one that calls enableStorage, since we were calling it after start that meant that we never polled for embeddables, now we do
TL;DR
Before: 1. Start, 2. Enable storage (We don't poll, we don't have storage enabled)
After: 1. Enable storage, 2. Start (We poll, we have storage enabled)
💚 Build Succeeded
Metrics [docs]Async chunks
Page load bundle
History
cc @AlexGPlay |
nreese
left a comment
There was a problem hiding this comment.
kibana presentation changes LGTM
code review only
…donly * commit 'bb1f55fa520b30ceb923af069ef403b24dcb1606': (52 commits) [CPS][Maps] Support CPS Picker in Maps (elastic#246382) [APM] Migrate the Transaction Overview tests to Scout/Playwright/Component/API tests (elastic#245972) [Cases] Change nested field search to be case insensitive (elastic#246643) [ES|QL] PromQL parser initial implementation (elastic#246552) [Agent Builder] Adds keyboard shortcut and toggle behavior to AI Agent button (elastic#246659) Retry on "all shards failed" from ES (elastic#246533) [Streams] Test enable wired streams flow (elastic#246113) [Agent Builder] Fast-follow bugfixes for MCP Tool type (elastic#246665) [Entity Store][API] Fix snake case on CRUD API List response (elastic#246003) [ResponseOps][Slack] Simplify channel configuration (elastic#245423) Add Canonical Name Badge to Documentation (elastic#246647) [Streams] Add simulation filtering by conditions (elastic#245400) [o11y AI] Add `get_hosts` tool (elastic#246541) [agent builder] create_visualization: support heatmap and regionmap (elastic#246671) [AI Infra] Chat experience: Selection modal title change (elastic#246683) [Background search] Change polling behavior (elastic#244760) [ES|QL ] Common Lookup Join Fields Are Not Listed First (elastic#246582) Add missing `dynamic: false` (elastic#246685) [Metrics in Discover] Unskip metrics api test (elastic#246593) [ES|QL] Show next actions after simple field assignment in RERANK ON Clause (elastic#246676) ...
## Summary Closes elastic#230909 Right now after a search has finished we keep polling it every 30 seconds, this was because search sessions allowed to save them after they finished, but with background search that's not the case, so we don't need to poll. ### Checklist Check the PR satisfies following conditions. Reviewers should verify this PR satisfies this list as well. - [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios - [x] The PR description includes the appropriate Release Notes section, and the correct `release_note:*` label is applied per the [guidelines](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process) - [x] Review the [backport guidelines](https://docs.google.com/document/d/1VyN5k91e5OVumlc0Gb9RPa3h1ewuPE705nRtioPiTvY/edit?usp=sharing) and apply applicable `backport:*` labels.
Summary
Closes #230909
Right now after a search has finished we keep polling it every 30 seconds, this was because search sessions allowed to save them after they finished, but with background search that's not the case, so we don't need to poll.
Checklist
Check the PR satisfies following conditions.
Reviewers should verify this PR satisfies this list as well.
release_note:*label is applied per the guidelinesbackport:*labels.