Settings api for stable plugins#91467
Conversation
|
Pinging @elastic/es-core-infra (Team:Core/Infra) |
|
Hi @pgomulka, I've created a changelog YAML for you. |
grcevski
left a comment
There was a problem hiding this comment.
This looks good to me, I had two minor questions about the interface.
| */ | ||
| @Retention(RetentionPolicy.RUNTIME) | ||
| @Target(ElementType.CONSTRUCTOR) | ||
| public @interface Inject { |
There was a problem hiding this comment.
I'm not fully up to date with all discussions, but I'm wondering if we considered maybe using another term than Inject? It might be confusing to plugin developers that are using their own dependency injection frameworks.
There was a problem hiding this comment.
I agree, it can be confusing for elasticserach developers too. We already have org.elasticsearch.common.inject.Inject.
Any ideas for better name? @SettingsInject @WithSettings?
There was a problem hiding this comment.
maybe @InjectSettings? It is doing dependency injection, but separately to any other frameworks around
There was a problem hiding this comment.
I like @InjectSettings - will update
| */ | ||
| @Retention(RetentionPolicy.RUNTIME) | ||
| @Target(ElementType.METHOD) | ||
| public @interface IntSetting { |
There was a problem hiding this comment.
Any reason we decided to provide max, but not min? Same for long setting.
There was a problem hiding this comment.
good point, if we have max, we can have min too. In fact I am not using max or min in this PR, as it does not contain validation. Perhaps I remove max in this PR, but include max and min in another that will actually use these attributes?
| } | ||
|
|
||
| } catch (InstantiationException | IllegalAccessException | InvocationTargetException e) { | ||
| throw new IllegalStateException("Cannot create instance of " + clazz, e); |
There was a problem hiding this comment.
should this be IllegalArgumentException?
the exception will be thrown upon the settings and plugin component creation. So when calling indices.analyze or creating a mapping
IllegalStateException will result in 500error IllegalArgumentException in 400
Stable plugins do not have a dependency on server, therefore cannot access Settings, NodeSettings or IndexSettings classes. Plugins implementing new stable plugin api will use set of annotations to mark an interface that works a as a facade for settings used by their plugin.
This will allow to validate the values provided against the restrictions defined in the plugin's settings interface
This commit introduces set of annotations in libs/plugin-api that allow to annotate an interface in plugins that will be later injected into a plugin instance. These annotations can possibly be used not only by analysis plugins in the future.
The implementation of the interface generated in server is using dynamic proxy mechanism.
relates #88980