-
Notifications
You must be signed in to change notification settings - Fork 8.2k
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
[ILM] Replace legacy ES client with the new client #78416
[ILM] Replace legacy ES client with the new client #78416
Conversation
@elasticmachine merge upstream |
Pinging @elastic/es-ui (Team:Elasticsearch UI) |
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.
Nice job @yuliacech! Thanks for working on this. I did a quick smoke test of ILM and did not notice any regressions. I also assume the API integration tests would have caught any issues.
I left a comment in the code regarding the use of isEsError
.
Also, per the migration docs, I think you can migrate the functional tests from using const client = getService('legacyEs');
to const client = getService('es');
.
I also just want to confirm this work is intended for 7.10
, given FF is today?
return response.ok(); | ||
} catch (e) { | ||
if (lib.isEsError(e)) { | ||
if (e instanceof ResponseError) { |
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 there are other instances of lib.isEsError
being used throughout the server code - do they need to be updated as well?
@elasticmachine merge upstream |
@elasticmachine merge upstream |
Hi @alisonelizabeth, |
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.
Latest changes LGTM. Thanks @yuliacech! I did not test again.
There are some changes in how the new client handles/returns ES errors, so further work is needed to update es_ui_shared.isEsError function (#79678).
This is not blocking, but I wonder if it's worth adding support for the way the new client handles errors soon rather than later, and rename the existing is_es_error
to is_es_error_legacy
(or something to that effect). Once everything is migrated, we could remove the legacy file. Otherwise, it seems like we potentially have to go back and replace if (e.name === 'ResponseError')
with if (lib.isEsError(e))
throughout the code, rather than just updating the file path.
|
||
const indexLifecycleDataEnricher = async ( | ||
indicesList: IndexWithoutIlm[], | ||
// TODO replace deprecated ES client after Index Management is updated |
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.
Thanks for adding this TODO
! It may also be worth adding a comment to #73973 as a reminder to circle back around to this.
@elasticmachine merge upstream |
1 similar comment
@elasticmachine merge upstream |
💚 Build SucceededMetrics [docs]distributable file count
History
To update your PR or re-run it, just comment with: |
thank you for the review, @alisonelizabeth ! I agree about addressing |
* [ILM] Replace legacy ES client with the new client * [ILM] Fix type check errors * [ILM] Fix api integration test * [ILM] Update error handling and integration test for the new client * [ILM] Fix api integration test * [ILM] Fix api integration test * [ILM] Add a todo comment for Index Management plugin extension Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
Summary
Fixes #77906.
This PR removes all usage of
LegacyAPICaller
and replaces it withElasticsearchClient
in ILM server code. There are some changes in how the new client handles/returns ES errors, so further work is needed to updatees_ui_shared.isEsError
function (#79678).Todo
es_ui_shared.isEsError
function [ES UI Shared] Update isEsError function for new ES client #79678