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.

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

[data.search] Add search session methods to search service contract #87966

Merged
merged 32 commits into from
Feb 3, 2021

Conversation

lukasolson
Copy link
Member

@lukasolson lukasolson commented Jan 12, 2021

Summary

This PR changes what we register on the route handler context for search. Prior, we were registering a search key that had a session property where all of the session methods lived. After, the session methods are directly on the search key. (For example, what used to be search.session.save is now search.saveSession.) This allows us to simplify the dependencies so that a scoped search session client is passed to each of the search service methods.

This PR also adds the extendSession method to the search service, which extends the session expiration and each individual search request associated with the session.

@lukasolson lukasolson added Feature:Search Querying infrastructure in Kibana WIP Work in progress v8.0.0 Team:AppServices release_note:skip Skip the PR/issue when compiling release notes Project:AsyncSearch Background search, partial results, async search services. v7.12.0 labels Jan 12, 2021
@lukasolson lukasolson self-assigned this Jan 12, 2021
Copy link
Contributor

@lizozom lizozom left a comment

Choose a reason for hiding this comment

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

I'm really happy with how this PR turned out!
Guess that there's nothing really to test here, just the conceptual changes. Right?

@lukasolson lukasolson removed the WIP Work in progress label Jan 14, 2021
@lukasolson lukasolson marked this pull request as ready for review January 14, 2021 22:01
@lukasolson lukasolson requested a review from a team as a code owner January 14, 2021 22:01
@elasticmachine
Copy link
Contributor

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

@lukasolson lukasolson requested a review from a team as a code owner January 15, 2021 21:10
Copy link
Contributor

@Kerry350 Kerry350 left a comment

Choose a reason for hiding this comment

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

Logs and metrics changes LGTM

Copy link
Contributor

@lizozom lizozom left a comment

Choose a reason for hiding this comment

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

LGTM once green :)

Copy link
Contributor

@stratoula stratoula left a comment

Choose a reason for hiding this comment

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

kibana app changes LGTM 🙂

Copy link
Contributor

@lizozom lizozom left a comment

Choose a reason for hiding this comment

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

@lukasolson I just realized that some of this refactoring might be problematic.
We need to be able to actually extend the search requests from the monitoring service, so we'd still need to have access to the search service from the session service. I think we must fix this in this PR, otherwise, we'll end up refactoring again in the next one :-\

@lukasolson
Copy link
Member Author

@elasticmachine merge upstream

Copy link
Contributor

@lizozom lizozom left a comment

Choose a reason for hiding this comment

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

Considering the changes in #89570
This LGTM

find: this.find.bind(this, deps),
update: this.update.bind(this, deps),
extend: this.extend.bind(this, deps),
cancel: this.cancel.bind(this, deps),
Copy link
Contributor

Choose a reason for hiding this comment

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

@lukasolson could you please keep both cancel and delete APIs, so we have some flexibility?

Copy link
Contributor

Choose a reason for hiding this comment

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

@lukasolson
Copy link
Member Author

@lizozom Before I merge this PR, I wanted to check with you... Since this moves some of the logic out of the session service into the search service, it will definitely conflict with your PR [1], and I wanted to be sure we're fine with how these changes will interact with the changes you've made in your PR.

[1] #89570

@lizozom
Copy link
Contributor

lizozom commented Feb 3, 2021

I merged your PR into mine, so feel free to merge first. :)

@kibanamachine
Copy link
Contributor

💚 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 798.2KB 798.4KB +157.0B

History

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

@lukasolson lukasolson merged commit a9273ca into elastic:master Feb 3, 2021
lukasolson added a commit to lukasolson/kibana that referenced this pull request Feb 3, 2021
…lastic#87966)

* [data.search] Add search session methods to search service contract

* Fix types

* Fix tests and switch to cancel

* Update docs

* Fix types/tests

* Fix tests

* Update status of SO before cancelling search requests

* Add API integration test

* Fix types

* Update expiration route to use config defaultExpiration

* Fix test

* Update docs

* New logic for extend

* Remove declare module

* Review feedback

* fix ts

* Remove test that is no longer valid

* Fix undefined bug

* Use DataRequestHandlerContext in maps

* ts

Co-authored-by: Liza K <liza.katz@elastic.co>
lukasolson added a commit that referenced this pull request Feb 3, 2021
…87966) (#90187)

* [data.search] Add search session methods to search service contract

* Fix types

* Fix tests and switch to cancel

* Update docs

* Fix types/tests

* Fix tests

* Update status of SO before cancelling search requests

* Add API integration test

* Fix types

* Update expiration route to use config defaultExpiration

* Fix test

* Update docs

* New logic for extend

* Remove declare module

* Review feedback

* fix ts

* Remove test that is no longer valid

* Fix undefined bug

* Use DataRequestHandlerContext in maps

* ts

Co-authored-by: Liza K <liza.katz@elastic.co>

Co-authored-by: Liza K <liza.katz@elastic.co>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Search Querying infrastructure in Kibana Project:AsyncSearch Background search, partial results, async search services. release_note:skip Skip the PR/issue when compiling release notes v7.12.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants