-
Notifications
You must be signed in to change notification settings - Fork 8.6k
Upgrade ES client to 9.0.0-alpha.4 #213375
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
Changes from all commits
08bfae4
f5331db
a6914f0
5768f66
44111e1
95bfd88
fef6073
81978d7
a99c19e
8e9e4b1
46f07b8
758c576
f63c1c1
9dd76fd
6e6b15e
b7071cf
3ab6cd4
29c34a6
4e7485b
770d5c9
35d1b51
c7a8249
02380db
2d83fc2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -194,8 +194,7 @@ export const executeUpdate = async <T>( | |
| refresh, | ||
| document: rawUpsert._source, | ||
| ...(version ? decodeRequestVersion(version) : {}), | ||
| // @ts-expect-error | ||
| require_alias: true, | ||
| querystring: { require_alias: true }, | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @JoshMock, here's a missing query parameter in the spec.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks, I opened elastic/elasticsearch-specification#3954 to add it (plus a bunch of others that were also missing). |
||
| }; | ||
|
|
||
| const { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -47,13 +47,13 @@ export class SearchCursorPit extends SearchCursor { | |
| index: this.indexPatternTitle, | ||
| keep_alive: scroll.duration(taskInstanceFields), | ||
| ignore_unavailable: true, | ||
| // @ts-expect-error ignore_throttled is not in the type definition, but it is accepted by es | ||
| ignore_throttled: includeFrozen ? false : undefined, // "true" will cause deprecation warnings logged in ES | ||
| ...(includeFrozen ? { querystring: { ignore_throttled: false } } : {}), // "true" will cause deprecation warnings logged in ES | ||
| }, | ||
| { | ||
| signal: this.abortController.signal, | ||
| requestTimeout: scroll.duration(taskInstanceFields), | ||
| maxRetries: 0, | ||
| // @ts-expect-error not documented in the types. Is this still supported? | ||
| maxConcurrentShardRequests, | ||
|
Comment on lines
+56
to
57
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Elasticsearch 8.9+ supports it, but it was never added to the specification. I've opened elastic/elasticsearch-specification#4142. |
||
| } | ||
| ); | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -115,11 +115,12 @@ export function registerFavoritesUsageCollection({ | |||||
| []) as estypes.AggregationsStringTermsBucket[]; | ||||||
|
|
||||||
| typesBuckets.forEach((bucket) => { | ||||||
| favoritesUsage[bucket.key] = { | ||||||
| total: bucket.stats.sum, | ||||||
| total_users_spaces: bucket.stats.count, | ||||||
| avg_per_user_per_space: bucket.stats.avg, | ||||||
| max_per_user_per_space: bucket.stats.max, | ||||||
| const stats = bucket.stats as estypes.AggregationsStatsAggregate; | ||||||
| favoritesUsage[`${bucket.key}`] = { | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: I think this line should not have been changed
Suggested change
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's caused by the new definition of export type FieldValue = long | double | string | boolean | null;https://github.com/elastic/elasticsearch-js/blob/main/src/api/types.ts#L2937 |
||||||
| total: stats.sum, | ||||||
| total_users_spaces: stats.count, | ||||||
| avg_per_user_per_space: stats.avg!, | ||||||
| max_per_user_per_space: stats.max!, | ||||||
| }; | ||||||
| }); | ||||||
|
|
||||||
|
|
||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -80,16 +80,18 @@ export async function getSavedObjectsCounts( | |
| const nonExpectedTypes: string[] = []; | ||
|
|
||
| const perType = buckets.map((perTypeEntry) => { | ||
| if (perTypeEntry.key !== MISSING_TYPE_KEY && !soTypes.includes(perTypeEntry.key)) { | ||
| const key = perTypeEntry.key as string; | ||
| if (key !== MISSING_TYPE_KEY && !soTypes.includes(key)) { | ||
| // If the breakdown includes any SO types that are not expected, highlight them in the nonExpectedTypes list. | ||
| nonExpectedTypes.push(perTypeEntry.key); | ||
| nonExpectedTypes.push(key); | ||
| } | ||
|
|
||
| return { key: perTypeEntry.key, doc_count: perTypeEntry.doc_count }; | ||
| }); | ||
|
|
||
| return { | ||
| total: body.total, | ||
| // @ts-expect-error `FieldValue` types now claim that bucket keys can be `null` | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oof, can this be true?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I checked and it cannot actually be true. null values are not indexed by Elasticsearch, and can't be bucket keys. Even the So, for bucket keys, we could use |
||
| per_type: perType, | ||
| non_expected_types: nonExpectedTypes, | ||
| others: body.aggregations?.types?.sum_other_doc_count ?? 0, | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -166,12 +166,20 @@ export const enhancedEsSearchStrategyProvider = ( | |
| throw new KbnSearchError(`"params.index" is required when performing a rollup search`, 400); | ||
| } | ||
|
|
||
| // eslint-disable-next-line @typescript-eslint/naming-convention | ||
| const { ignore_unavailable, preference, ...params } = { | ||
| ...querystring, | ||
| ...request.params, | ||
| index: request.params.index, | ||
| }; | ||
|
|
||
| try { | ||
| const esResponse = await client.rollup.rollupSearch( | ||
| { | ||
| ...querystring, | ||
| ...request.params, | ||
| index: request.params.index, | ||
| ...params, | ||
| // Not defined in the spec, and the client places it in the body. | ||
| // This workaround allows us to force it as a query parameter. | ||
| querystring: { ...params.querystring, ignore_unavailable, preference }, | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It doesn't seem to matter, but it looks like prior to this change,
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. cc @pquentin
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Many Elasticsearch APIs accept the same parameters as query parameters and in the body, one reason being that by default, query strings are limited to 4096 characters. Clients prefer the body in this case, and I know the JS client changed the way it works in 9.0.0-alpha.4: elastic/elasticsearch-js#2584 (comment). So this is likely fine and expected. What API is this though? rollup_search does not seem to include |
||
| }, | ||
| { | ||
| signal: options?.abortSignal, | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -253,7 +253,7 @@ function getWrappedEqlSearchFn(opts: WrapEsClientOpts) { | |
| requestTimeout ? ` and ${requestTimeout}ms requestTimeout` : '' | ||
| }` | ||
| ); | ||
| const result = (await originalEqlSearch.call(opts.esClient, params, { | ||
| const result = (await originalEqlSearch.call(opts.esClient.eql, params, { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Seems weird that this would have worked before!
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yes... I think that the internals changed to address the body vs. query parameters... but maybe @JoshMock can provide a more accurate reason.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This one's a bit tricky. 🤔 It may be because, in 8.x, I wasn't thinking about that side effect when making this change, but now I'm wondering if there's a better pattern to avoid this being a breaking change. |
||
| ...searchOptions, | ||
| ...(requestTimeout | ||
| ? { | ||
|
|
||

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.
require_aliasis accepted by theIndexRequestbut not theCreateRequest, so I went with thequerystringpassthrough for both here.