Skip to content
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

Elasticsearch legacy compatible api #41468

Closed

Conversation

mshustov
Copy link
Contributor

@mshustov mshustov commented Jul 18, 2019

Summary

There is a plan to migrate from https://github.com/elastic/elasticsearch-js-legacy to the new https://github.com/elastic/elasticsearch-js client. Their interfaces are not compatible and migration will require additional work.
The current implementation of the Elasticsearch service introduced another version of the API. Thus, a plugin migrated to New Platform Elasticsearch Service has to adapt to the new API twice:

  • when switching from Legacy platform API to New Platform API.
  • when switching from New Platform API to elasticsearch-js.
    This PR make New Platform Elasticsearch API compatible with LegacyPlatform to avoid double work for plugins in the future.

Checklist

Use strikethroughs to remove checklist items you don't feel are applicable to this PR.

For maintainers

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@mshustov mshustov added Feature:New Platform release_note:skip Skip the PR/issue when compiling release notes review Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc v7.4.0 labels Jul 22, 2019
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-platform

@mshustov mshustov marked this pull request as ready for review July 22, 2019 09:09
@mshustov mshustov requested review from a team as code owners July 22, 2019 09:09
@mshustov mshustov requested a review from azasypkin July 22, 2019 09:09
*/
public asScoped(request?: KibanaRequest | LegacyRequest | FakeRequest) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we tag it as a breaking change? I found only Security plugin uses this API, although I could miss a plugin that is in progress of the migration.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes we definitely should. There are at least 3 or 4 new in-development plugins. Let's add it to this issue #40768

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@mshustov mshustov added release_note:breaking release_note:plugin_api_changes Contains a Plugin API changes section for the breaking plugin API changes section. and removed release_note:skip Skip the PR/issue when compiling release notes labels Jul 23, 2019
@elasticmachine
Copy link
Contributor

💔 Build Failed

@azasypkin
Copy link
Member

ACK: Reviewing...

Copy link
Member

@azasypkin azasypkin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes in Spaces and Security plugins look good to me assuming this is what we really want. But before we move forward with these changes can you please clarify the following things for me?

  • What is the main motivation behind these changes and what problem are we trying to solve here? Issue description is a bit ters on that.

  • If the current core ElasticsearchService API conflicts with the one we'll have once we migrate to the new elasticsearch-js lib, then how the new API will look like? I thought that once we advance with more and more "scoped" or "context-bound" services provided by the core we'll end up with something like callAsInternalUser and callAsCurrentUser anyway, and consumers won't need to call asScoped manually (most of the time at least). But I'm out of the loop here, sorry if you need to explain that multiple times 🙈

  • In general, why the core public API is going to change with the new version of the 3rd-party lib we use under the hood (elasticsearch-js lib)? Aren't we trying to augment consumers from raw elasticsearch-js lib and provide stable interfaces/contracts that don't neceserrily need to change if underlying lib changes? Are we planning to incorporate new major versions of elasticsearch-js client only within new major versions of Kibana as well? As far as I understand elasticsearch-js is still relatively independent or we have plans to converge APIs of ElasticSearchService and elasticsearch-js somehow?

I had a brief conversation with @restrry over Slack and I've got that with these changes we're trying to simplify migration of the legacy plugins to the new platform. But it's still not immediately clear to me in what ways these changes do that since we already have a legacy elasticsearch plugin that serves exactly that purpose. I mean people will still need to change the way they receive cluster client(s), probably adapt to new error types, update tests to use new platform mocks (while migrating Security to NP that caused the main pain I believe cause we had lots of tests to update), etc.

So the only win I see here is that we'll keep same method names and their signatures (for callWithRequest only, callAsInternalUser and callWithInternalUser have the same signature already), but it doesn't feel like a huge one in the grand scheme of things unless I'm missing something.

Having said that, I'm not opposed to what you've done here, just want to have a better understanding of the plan you have 🙂

Thanks a lot!

@mshustov
Copy link
Contributor Author

closed until we agree on the final design and outline the whole migration plan.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:New Platform release_note:plugin_api_changes Contains a Plugin API changes section for the breaking plugin API changes section. Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants