[Search Sessions] Cancel the previous session #99658

lizozom · 2021-05-10T17:43:55Z

Summary

This PR makes sure to close search sessions, to free up resources taken in the async-search index, saved objects, and in the future open PITs
To 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 cancel request 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.

Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
Documentation was added for features that require explanation or tutorials
Unit or functional tests were updated or added to match the most common scenarios
Any UI touched in this PR is usable by keyboard only (learn more about keyboard accessibility)
Any UI touched in this PR does not create any new axe failures (run axe in browser: FF, Chrome)
If a plugin configuration key changed, check if it needs to be allowlisted in the cloud and added to the docker list
This renders correctly on smaller devices using a responsive layout. (You can test this in your browser)
This was checked for cross-browser compatibility

For maintainers

This was checked for breaking API changes and was labeled appropriately

elasticmachine · 2021-05-13T10:29:39Z

Pinging @elastic/kibana-app-services (Team:AppServices)

…llation

lizozom · 2021-05-13T18:08:02Z

@elasticmachine merge upstream

lizozom · 2021-05-16T13:17:55Z

@elasticmachine merge upstream

…llation

… session/cancellation

…llation

lizozom · 2021-05-19T15:46:03Z

@elasticmachine merge upstream

lizozom · 2021-05-19T16:26:39Z

@elasticmachine merge upstream

lizozom · 2021-05-20T13:37:30Z

@elasticmachine merge upstream

…llation

… session/cancellation

lizozom · 2021-05-24T07:58:51Z

@elasticmachine merge upstream

…\ expired async search

lizozom · 2021-05-24T12:16:40Z

src/plugins/data/public/search/session/session_service.ts

    this.state.transitions.cancel();
-    if (isStoredSession) {


Replaced deletion with cancellation.
The cleanup task will take care of deletion #99967

… session/cancellation

jloleysens

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.

x-pack/test/api_integration/apis/search/session.ts

jloleysens · 2021-05-25T10:33:48Z

x-pack/test/api_integration/apis/search/session.ts

+          .expect(200);
+
+        const { status } = resp.body.attributes;
+        expect(status).not.to.equal(SearchSessionStatus.CANCELLED);


What is the expected status then?

Either IN_PROGRESS or COMPLETED :-)

jloleysens · 2021-05-25T10:40:02Z

src/plugins/data/public/search/session/session_service.ts

@@ -213,6 +213,10 @@ export class SessionService {
   */
  public start() {
    if (!this.currentApp) throw new Error('this.currentApp is missing');
+
+    // cancel previous session if needed
+    this.cleanupPreviousSession();


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

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?

lukasolson · 2021-05-25T16:20:52Z

src/plugins/data/public/search/session/session_service.test.ts

+      expect(nowProvider.set).toHaveBeenCalled();
+      sessionService.start();
+      expect(sessionService.getSessionId()).not.toBeUndefined();
+      expect(sessionsClient.cancel).toHaveBeenCalled();


Do you want to make sure it's called with the id returned from start()?

lukasolson · 2021-05-25T16:26:34Z

src/plugins/data/public/search/session/sessions_client.ts

@@ -92,6 +92,10 @@ export class SessionsClient {
    });
  }

+  public cancel(sessionId: string): Promise<void> {
+    return this.http!.post(`/internal/session/${encodeURIComponent(sessionId)}/cancel`);


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.

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?

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?

…llation

kibanamachine · 2021-05-31T09:33:42Z

💚 Build Succeeded

Metrics [docs]

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id	before	after	diff
`data`	817.7KB	818.1KB	+443.0B

History

💔 Build #128114 failed 6e6fcb2
💚 Build #127104 succeeded 74c145e
💚 Build #126868 succeeded e1db913
💛 Build #126844 was flaky a988edd
💔 Build #126833 failed f911163

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

cc @lizozom

jbudz

limits.yml LGTM

Liza K added 2 commits May 10, 2021 20:41

cancel the previous session

12b95ab

fixes

6e870dd

lizozom marked this pull request as ready for review May 13, 2021 10:22

lizozom requested a review from a team as a code owner May 13, 2021 10:22

lizozom self-assigned this May 13, 2021

lizozom added AppServicesSync-7-14 release_note:skip Skip the PR/issue when compiling release notes v7.14.0 v8.0.0 labels May 13, 2021

lizozom added the auto-backport Deprecated - use backport:version if exact versions are needed label May 13, 2021

lizozom mentioned this pull request May 13, 2021

Support for Elasticsearch point-in-time API #80406

Closed

4 tasks

Liza K added 2 commits May 13, 2021 13:52

cancellation

436dd51

Merge branch 'master' of github.com:elastic/kibana into session/cance…

984eba2

…llation

Merge branch 'master' into session/cancellation

f5fdd88

kibanamachine and others added 9 commits May 16, 2021 09:18

Merge branch 'master' into session/cancellation

768d7ed

Merge branch 'master' of github.com:elastic/kibana into session/cance…

77624ff

…llation

cleanup previous session properly

e5904ac

Merge branch 'master' of github.com:elastic/kibana into session/cance…

4f32b09

…llation

Merge branch 'session/cancellation' of github.com:lizozom/kibana into…

f9f1ebb

… session/cancellation

don't fail delete and cancel if item was already cleaned up

44023c4

test

54bb6cd

test

9ffbb03

Merge branch 'master' of github.com:elastic/kibana into session/cance…

6de0b62

…llation

Merge branch 'master' into session/cancellation

a343e13

Merge branch 'master' into session/cancellation

6d96554

kibanamachine and others added 3 commits May 20, 2021 09:37

Merge branch 'master' into session/cancellation

49fe28a

Merge branch 'master' of github.com:elastic/kibana into session/cance…

b3e093a

…llation

Merge branch 'session/cancellation' of github.com:lizozom/kibana into…

f911163

… session/cancellation

kibanamachine and others added 2 commits May 24, 2021 03:58

Merge branch 'master' into session/cancellation

a988edd

ignore resource_not_found_exception when deleting an already cleared …

25445c6

…\ expired async search

lizozom commented May 24, 2021

View reviewed changes

Merge branch 'session/cancellation' of github.com:lizozom/kibana into…

e1db913

… session/cancellation

jloleysens approved these changes May 25, 2021

View reviewed changes

Update x-pack/test/api_integration/apis/search/session.ts

74c145e

Co-authored-by: Jean-Louis Leysens <jloleysens@gmail.com>

lukasolson reviewed May 25, 2021

View reviewed changes

limit

6e6fcb2

lizozom requested a review from a team as a code owner May 30, 2021 16:37

Merge branch 'master' of github.com:elastic/kibana into session/cance…

ee5ecc7

…llation

jbudz approved these changes Jun 1, 2021

View reviewed changes

spalger added v7.15.0 and removed v7.14.0 labels Jun 30, 2021

This was referenced Jul 13, 2021

[Search Sessions] Don’t try deleting errored searches #105434

Merged

[Search Sessions] cleanup error when navigating away from a loading dashboard without saving the session #104309

Closed

[Search] Review searches clean up #105441

Closed

mistic added v7.16.0 and removed v7.15.0 labels Aug 18, 2021

lizozom closed this Oct 7, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Search Sessions] Cancel the previous session #99658

[Search Sessions] Cancel the previous session #99658

lizozom commented May 10, 2021 •

edited

Loading

elasticmachine commented May 13, 2021

lizozom commented May 13, 2021

lizozom commented May 16, 2021

lizozom commented May 19, 2021

lizozom commented May 19, 2021

lizozom commented May 20, 2021

lizozom commented May 24, 2021

lizozom May 24, 2021

jloleysens left a comment

jloleysens May 25, 2021

lizozom May 25, 2021

jloleysens May 25, 2021

lukasolson left a comment

lukasolson May 25, 2021

lukasolson May 25, 2021

lukasolson May 25, 2021

lizozom May 30, 2021

kibanamachine commented May 31, 2021

jbudz left a comment

[Search Sessions] Cancel the previous session #99658

[Search Sessions] Cancel the previous session #99658

Conversation

lizozom commented May 10, 2021 • edited Loading

Summary

Testing

Checklist

For maintainers

elasticmachine commented May 13, 2021

lizozom commented May 13, 2021

lizozom commented May 16, 2021

lizozom commented May 19, 2021

lizozom commented May 19, 2021

lizozom commented May 20, 2021

lizozom commented May 24, 2021

Choose a reason for hiding this comment

jloleysens left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lukasolson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kibanamachine commented May 31, 2021

💚 Build Succeeded

Metrics [docs]

Page load bundle

History

jbudz left a comment

Choose a reason for hiding this comment

lizozom commented May 10, 2021 •

edited

Loading