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

Add OperatorHandler interface #87767

Merged
merged 9 commits into from
Jun 27, 2022

Conversation

grcevski
Copy link
Contributor

This PR relates to #86224 (Operator/File cluster settings). We are introducing a new action handler interface called OperatorHandler, which will be used to implement various operator mode actions to update the cluster state. Since handlers can belong to modules and plugins, we introduce a SPI to declare the handlers a plugin/module provides.

For reference, please see OperatorLifecycleAction.java in #86224.

Since the original PR is too large on its own, I'm separating the individual components of the PR for easier review.

@grcevski grcevski added >non-issue :Core/Infra/Core Core issues without another label Team:Core/Infra Meta label for core/infra team v8.4.0 labels Jun 16, 2022
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra (Team:Core/Infra)

@grcevski
Copy link
Contributor Author

@elasticmachine update branch

* @param service A templated service class to look for providers in plugins
* @return an immutable {@link List} of discovered providers in the plugins/modules
*/
public <T> List<? extends T> loadServiceProviders(Class<T> service) {
Copy link
Member

Choose a reason for hiding this comment

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

Can this be split out into a separate PR? It seems like this is most of this PR, but the title would suggest otherwise.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, sounds good, I'll take this out from this PR and contribute it back once the operator handler interface is in.

@grcevski grcevski requested a review from rjernst June 22, 2022 19:39
Copy link
Member

@rjernst rjernst left a comment

Choose a reason for hiding this comment

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

This looks good at a high level. The three methods, name, transform and dependencies make sense. One general comment on the javadocs are that they should generally have a clear one sentence explanation about what the method does. Currently these methods all have large blocks of text that are a good explanation, but they are too detailed for the quick summary that should be the first, separate, line.

* @param source the operator content as a map
* @return
*/
default XContentParser mapToXContentParser(XContentParserConfiguration config, Map<String, ?> source) {
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't seem to have anything to do with operator config, could it be in a more general place for xcontent utilities?

* @return
*/
@SuppressWarnings("unchecked")
default Map<String, ?> asMap(Object input) {
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't seem like something specific to operator config, could it be in a more appropriate location for utility methods?

*
* @param request the master node request that we base this operator handler on
*/
default void validate(MasterNodeRequest<?> request) {
Copy link
Member

Choose a reason for hiding this comment

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

Does this differ across any operator handler implementations? If not, could it just be a local utility method in the operator code?

@grcevski grcevski merged commit c2d1b22 into elastic:master Jun 27, 2022
@grcevski grcevski deleted the operator/handler_interface branch June 27, 2022 13:51
@grcevski grcevski changed the title Add OperatorHandler and OperatorHandler SPI Add OperatorHandler interface Jun 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Core/Infra/Core Core issues without another label >non-issue Team:Core/Infra Meta label for core/infra team v8.4.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants