-
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
[skip ci] Data access service #38737
Conversation
Pinging @elastic/kibana-app-arch |
97fb13c
to
9cc45eb
Compare
💔 Build Failed |
9cc45eb
to
3a75978
Compare
💔 Build Failed |
fe78a07
to
116c40f
Compare
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.
apart from some minor comments this looks good, needs test coverage still.
onProgress?: (shards: ShardProgress) => void; | ||
signal?: AbortSignal; | ||
strategy?: string; | ||
sessionId?: string; |
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.
should we allow overriding any of the advanced settings here ?
|
||
export const searchStrategies: Map<string, SearchStrategy> = new Map<string, SearchStrategy>(); | ||
|
||
export function registerSearchStrategy(name: string, strategy: SearchStrategy) { |
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.
add a getter rather than exposing whole 'registry'
import { getEsSearchConfig } from './get_es_search_config'; | ||
|
||
export function registerDefaultSearchStrategy(server: Server) { | ||
registerSearchStrategy('default', async function defaultSearchStrategy( |
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.
nit: i would prefer exporting searchStrategy function here and doing registration init
d6ba843
to
fff3492
Compare
maxConcurrentShardRequests: searchParams.hasOwnProperty('maxConcurrentShardRequests') | ||
? (searchParams as any).maxConcurrentShardRequests | ||
: await getMaxConcurrentShardRequests(request), | ||
ignoreThrottled: searchParams.hasOwnProperty('ignoreThrottled') |
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 wonder if this can get moved to the x-pack search strategy. Maybe getSearchParams
should be part of the search strategy? e.g.
export interface ISearchStrategy {
search: (request: Request, searchArguments: SearchArguments) => Observable<SearchResponse<any>>;
getSearchParams: (
request: Request,
searchParams: SearchParams,
options: SearchOptions
) => SearchParams;
}
Then the x-pack implementation can use the default one plus incorporate some additional settings that are commercial only?
? searchParams.preference | ||
: await getPreference(request, options), | ||
maxConcurrentShardRequests: searchParams.hasOwnProperty('maxConcurrentShardRequests') | ||
? (searchParams as any).maxConcurrentShardRequests |
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.
Any idea why maxConcurrentShardRequests
isn't in the SearchParams type? Is it an oversight in the typing or is this a kibana specific param?
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.
actually I see it's only part of msearch. I guess for search endpoint it's irrelevant.
} | ||
} | ||
|
||
export async function getMaxConcurrentShardRequests(request: Request) { |
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.
this is returning any
type because of config.get
. If we know the shape of return value, maybe it can be typed out explicitly here?
if (maxConcurrentShardRequests !== 0) return maxConcurrentShardRequests; | ||
} | ||
|
||
// TODO: Move to a plugin |
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.
ah, I see this comment now!
Superseded by #43371. |
Summary
Relies on #37563.
This PR introduces the new data access service API. It's the first step towards #34022. Specifically, this PR introduces a new server endpoint (
/api/search
) which accepts raw Elasticsearch search DSL and sends it to Elasticsearch.This PR also introduces both a client- and server-side tool for making such requests.
Client-side API
Server-side API
The same as the client-side API, except that the calls must also take the
request
that initiated the search, to forward credentials to Elasticsearch:Eventually we intend this service/API to be a replacement for the existing Courier service.
Checklist
[ ] This was checked for cross-browser compatibility, including a check against IE11[ ] This was checked for keyboard-only and screenreader accessibilityFor maintainers
[ ] This includes a feature addition or change that requires a release note and was labeled appropriately