[data.search] Add search session methods to search service contract#87966
[data.search] Add search session methods to search service contract#87966lukasolson merged 32 commits intoelastic:masterfrom
Conversation
lizozom
left a comment
There was a problem hiding this comment.
I'm really happy with how this PR turned out!
Guess that there's nothing really to test here, just the conceptual changes. Right?
x-pack/plugins/data_enhanced/server/search/session/session_service.test.ts
Outdated
Show resolved
Hide resolved
x-pack/plugins/data_enhanced/server/search/session/session_service.ts
Outdated
Show resolved
Hide resolved
x-pack/plugins/data_enhanced/server/search/session/session_service.ts
Outdated
Show resolved
Hide resolved
|
Pinging @elastic/kibana-app-services (Team:AppServices) |
Kerry350
left a comment
There was a problem hiding this comment.
Logs and metrics changes LGTM
stratoula
left a comment
There was a problem hiding this comment.
kibana app changes LGTM 🙂
lizozom
left a comment
There was a problem hiding this comment.
@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 :-\
|
@elasticmachine merge upstream |
94672b4 to
99b4ca0
Compare
| find: this.find.bind(this, deps), | ||
| update: this.update.bind(this, deps), | ||
| extend: this.extend.bind(this, deps), | ||
| cancel: this.cancel.bind(this, deps), |
There was a problem hiding this comment.
@lukasolson could you please keep both cancel and delete APIs, so we have some flexibility?
|
@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 |
|
I merged your PR into mine, so feel free to merge first. :) |
💚 Build SucceededMetrics [docs]Page load bundle
History
To update your PR or re-run it, just comment with: |
…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>
…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>
Summary
This PR changes what we register on the route handler context for search. Prior, we were registering a
searchkey that had asessionproperty where all of the session methods lived. After, the session methods are directly on thesearchkey. (For example, what used to besearch.session.saveis nowsearch.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
extendSessionmethod to the search service, which extends the session expiration and each individual search request associated with the session.