[Search Sessions] Cancel the previous session#99658
[Search Sessions] Cancel the previous session#99658lizozom wants to merge 25 commits intoelastic:masterfrom
Conversation
|
Pinging @elastic/kibana-app-services (Team:AppServices) |
|
@elasticmachine merge upstream |
|
@elasticmachine merge upstream |
… session/cancellation
|
@elasticmachine merge upstream |
|
@elasticmachine merge upstream |
|
@elasticmachine merge upstream |
|
@elasticmachine merge upstream |
| }); | ||
| await this.sessionsClient.cancel(sessionId).catch(() => {}); | ||
| this.state.transitions.cancel(); | ||
| if (isStoredSession) { |
There was a problem hiding this comment.
Replaced deletion with cancellation.
The cleanup task will take care of deletion #99967
… session/cancellation
jloleysens
left a comment
There was a problem hiding this comment.
Tested locally and can confirm that the call to "cancel" is happening. Might be worth getting review from @Dosant too since it looks like he is quite familiar with this part of the code base. Left some non-blocker comments and one question.
| .expect(200); | ||
|
|
||
| const { status } = resp.body.attributes; | ||
| expect(status).not.to.equal(SearchSessionStatus.CANCELLED); |
There was a problem hiding this comment.
What is the expected status then?
There was a problem hiding this comment.
Either IN_PROGRESS or COMPLETED :-)
| if (!this.currentApp) throw new Error('this.currentApp is missing'); | ||
|
|
||
| // cancel previous session if needed | ||
| this.cleanupPreviousSession(); |
There was a problem hiding this comment.
not sure I understand the full lifecycle so please correct me - this function calls "cancel" under the hood so is it possible to have a race-condition between creating a new search and the time it takes to cancel the current one so that we cancel the new search?
FWIW, I was not able to reproduce this, purely based on my current understanding.
Co-authored-by: Jean-Louis Leysens <jloleysens@gmail.com>
lukasolson
left a comment
There was a problem hiding this comment.
I'm noticing a funky behavior: If I restore a session then click Refresh, I still see a cancel request go out (although it doesn't appear to be for the restored session). It looks like session.start() gets called twice right in a row.
Also, is there a reason we aren't calling this.state.transitions.cancel() from inside cleanupPreviousSession?
| expect(nowProvider.set).toHaveBeenCalled(); | ||
| sessionService.start(); | ||
| expect(sessionService.getSessionId()).not.toBeUndefined(); | ||
| expect(sessionsClient.cancel).toHaveBeenCalled(); |
There was a problem hiding this comment.
Do you want to make sure it's called with the id returned from start()?
| } | ||
|
|
||
| public cancel(sessionId: string): Promise<void> { | ||
| return this.http!.post(`/internal/session/${encodeURIComponent(sessionId)}/cancel`); |
There was a problem hiding this comment.
Super minor nit: Can we either standardize on /_action or /action? I don't really have a preference (though saved objects are /_action), just want them to be the same format.
There was a problem hiding this comment.
To me, it would make more sense to swallow the 404 errors here in the client rather in the API itself, and only log the errors on the server if it's not a 404. What do you think?
There was a problem hiding this comment.
I didn't intend do to this first, but you can't eliminate the 404 from the console, and it looks bad IMO
https://stackoverflow.com/questions/12915582/eliminate-404-url-error-in-console
What do you think?
💚 Build SucceededMetrics [docs]Page load bundle
History
To update your PR or re-run it, just comment with: cc @lizozom |
Summary
Part of #80406
This PR makes sure to close search sessions, to free up resources taken in the
async-searchindex,saved objects, and in the future openPITsTo simplify the code, we currently rely on the search session service managing only a single simultaneous search session.
If in any point in the future we decide to refactor the search service and support multiple open sessions, this part of the session's lifecycle will need to be adjusted as well.
Testing
To test this feature, try using Discover, Dashboard or Lens and make sure that a
cancelrequest for the previous session is sent to the server every time a new session is started.Checklist
Delete any items that are not applicable to this PR.
For maintainers