-
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] Move background session service to data enhanced plugin #84837
Conversation
Pinging @elastic/kibana-app-services (Team:AppServices) |
@@ -37,7 +36,7 @@ export class SessionsClient { | |||
this.http = deps.http; | |||
} | |||
|
|||
public get(sessionId: string): Promise<SavedObject<BackgroundSessionSavedObjectAttributes>> { |
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.
@Dosant, you'll noticed I removed references to the BackgroundSession
stuff here since it was moved to xpack. The corresponding routes are also now only available in Basic+, which probably means this file would make more sense being moved to xpack, but I'm not exactly sure how you want to go about it. Do you have any thoughts about it?
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 think we should move those to x-pack too
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 think we can move session_client
implementation into x-pack but will have to leave interface in OSS (decoupled from BackgroundSessionSavedObjectAttributes
).
But I don't think we should move sessionService
into x-pack because of how OSS apps rely on it.
If you agree, please be free to open an issue and assign on me.
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.
Overall LGTM once green.
I wonder if we want to offer a more similar API on OSS\Xpack, with the OSS functions returning a falsy value (or an error?).
@@ -55,8 +56,12 @@ export class EnhancedDataServerPlugin implements Plugin<void, void, SetupDepende | |||
deps.data.__enhance({ | |||
search: { | |||
defaultStrategy: ENHANCED_ES_SEARCH_STRATEGY, | |||
sessionService: new BackgroundSessionService(), |
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.
🔥
@@ -109,7 +106,7 @@ export class SearchService implements Plugin<ISearchSetup, ISearchStart> { | |||
private defaultSearchStrategyName: string = ES_SEARCH_STRATEGY; | |||
private searchStrategies: StrategyMap = {}; | |||
private coreStart?: CoreStart; | |||
private sessionService: BackgroundSessionService = new BackgroundSessionService(); | |||
private sessionService: ISessionService = new SessionService(); |
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.
Since the server and client types are so different, maybe we should name the types noting they are Client \ Server ?
@elasticmachine merge upstream |
@elasticmachine merge upstream |
@elasticmachine merge upstream |
💚 Build SucceededMetrics [docs]Module Count
Distributable file count
Page load bundle
History
To update your PR or re-run it, just comment with: |
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
…elastic#84837) * [data.search] Move search method inside session service and add tests * Move background session service to data_enhanced plugin * Fix types Co-authored-by: Kibana Machine <[email protected]>
…#84837) (#85152) * [data.search] Move search method inside session service and add tests * Move background session service to data_enhanced plugin * Fix types Co-authored-by: Kibana Machine <[email protected]> Co-authored-by: Kibana Machine <[email protected]>
@@ -55,7 +54,7 @@ export class SessionsClient { | |||
restoreState: Record<string, unknown>; | |||
urlGeneratorId: string; | |||
sessionId: string; | |||
}): Promise<SavedObject<BackgroundSessionSavedObjectAttributes>> { | |||
}): Promise<SavedObject> { |
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.
Is the intention to use this client in the sessions management UI?
I can do that, but it looks like the client methods are less type-aware after this PR
Summary
Requires #84715.
This PR moves the majority of background sessions support to the
data_enhanced
plugin so that we can, in a follow-up PR, add support for integrating with security.As part of this effort, the following changes were made:
An OSS
SessionService
was introduced, that simply invokes the correspondingSearchStrategy
. TheBackgroundSessionService
was moved todata_enhanced
, as well as the registering of the saved object type as well as routes that handle managing the saved objects.The
SessionService
now also manages its own dependencies rather than expecting them to be passed in as part of thesearch
method in theSearchService
.Checklist
Delete any items that are not applicable to this PR.