-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
Conversation
There was a problem hiding this 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?
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) |
There was a problem hiding this 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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM once green :)
There was a problem hiding this 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 🙂
There was a problem hiding this 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 :-\
@elasticmachine merge upstream |
There was a problem hiding this 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
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.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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
search
key that had asession
property where all of the session methods lived. After, the session methods are directly on thesearch
key. (For example, what used to besearch.session.save
is 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
extendSession
method to the search service, which extends the session expiration and each individual search request associated with the session.