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

[pluginSystem] optimize plugins stop execution #164831

Merged
merged 4 commits into from
Aug 27, 2023

Conversation

pgayvallet
Copy link
Contributor

@pgayvallet pgayvallet commented Aug 25, 2023

Summary

Related to #163519

Optimize the way we're shutting down plugins, by calling stop for each plugin as soon as all dependant plugins are stopped, instead of just doing it one-by-one.

@pgayvallet pgayvallet added Feature:Plugins Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc v8.11.0 labels Aug 25, 2023
@pgayvallet pgayvallet marked this pull request as ready for review August 25, 2023 12:51
@pgayvallet pgayvallet requested a review from a team as a code owner August 25, 2023 12:51
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-core (Team:Core)

@pgayvallet pgayvallet added the release_note:skip Skip the PR/issue when compiling release notes label Aug 25, 2023
Copy link
Member

@afharo afharo left a comment

Choose a reason for hiding this comment

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

LGTM. I just added 1 nit


this.log.debug(`Stopping plugin "${pluginName}"...`);
// Stop plugin as soon as all the dependant plugins are stopped.
const pluginStopPromise = Promise.all(dependantPromises).then(async () => {
Copy link
Member

Choose a reason for hiding this comment

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

nit: should we use Promise.allSettled instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Old behavior was to throw on first error, so I just kept it tbh. It may be better to wait for everything, but I was kinda scared to change.

Copy link
Member

Choose a reason for hiding this comment

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

The only difference is that now we are running multiple stops at the same time. So we may cancel ongoing promises.

IMO, it's OK not to continue calling other stops, but I feel uneasy cancelling running calls (despite our stop is not guaranteed to be called policy).

Happy to merge as is, though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fair point. it would be a bit challenging to only cancel the running promises and would over complexify the flow. I will adapt to wait for everything instead and use allSettled 👍

@pgayvallet pgayvallet enabled auto-merge (squash) August 27, 2023 09:56
@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

✅ unchanged

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@pgayvallet pgayvallet merged commit 9f83684 into elastic:main Aug 27, 2023
@kibanamachine kibanamachine added the backport:skip This commit does not require backporting label Aug 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting Feature:Plugins release_note:skip Skip the PR/issue when compiling release notes Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc v8.11.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants