-
Notifications
You must be signed in to change notification settings - Fork 24.6k
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
Allow plugins to define custom operations that they use scripts for #10419
Allow plugins to define custom operations that they use scripts for #10419
Conversation
…ripts for Plugins can now define multiple operations/contexts that they use scripts for. Fine-grained settings can then be used to enable/disable scripts based on each single registered context. Also added a new generic category called `plugins`, which will be used as a default when the context is not specified. This allows us to restore backwards compatibility for plugins on `ScriptService` by restoring the old methods that don't require the script context and making them internally use the `plugins` context, as they can only be called from plugins. Closes elastic#10347
@uboness assigning this to you for review |
@javanna (don't kill me) thinking out load.. going back to the previous enum... would it be better to change that enum to an interface, such that the contract public interface ScriptContext {
enum Std implements ScriptContext {
MAPPING() {
public String key() {
return "mapping";
}
},
SEARCH() {
public String key() {
return "search";
}
},
UPDATE() {
public String key() {
return "update";
}
},
AGGS() {
public String key() {
return "aggs";
}
};
}
String key();
class Plugin implements ScriptContext {
private String key;
public Plugin(String pluginName, String context) {
this.key = pluginName + "_" + context;
}
public String key() {
return key;
}
}
} the |
@uboness makes sense to me, just pushed a new commit. |
* @deprecated create a new {@link org.elasticsearch.script.ScriptContext.Plugin} instance instead | ||
*/ | ||
@Deprecated | ||
Plugin GENERIC_PLUGIN = new Plugin("plugin"); |
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.
why not put this under Standard
and deprecate it as an option?
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.
will do
left a couple of comments |
Pushed another commit. Wondering: shall we restrict |
I like it... hmm... I can't think of any scenario where you'd want to register custom contexts outside of a plugin. outside of a plugin is only within the core codebase and for that we can always extend the std enum. It's easier to start restrictive and open it up if we find the need to.. rather than open it up now and later try to restrict it. So I'd say go for it... |
pushed another commit, restricted the custom script context to Are we happy about using the |
agreed
yea... I chose this one as I wasn't sure how it'd effect 1) the parsing of the settings, 2) would confuse when reading the settings as |
cool @uboness can you go over it one last time please? should be ready |
executed from sandboxed languages as part of `aggregations` and `search` | ||
operations though, as the above defaults still get applied. | ||
executed from sandboxed languages as part of `aggregations`, `search` | ||
and `plugin` operations though, as the above defaults still get applied. | ||
|
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.
shall we add a NOTE:
here about custom plugin contexts?
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.
Plugins are briefly mentioned a few lines above:
Plugins can also define custom operations that they use scripts for instead of using the generic
plugin
category. Those operations can be referred to in the following form:$pluginName_$operation
.
That said I haven't explained how plugins can do it, but we don't really have reference for plugins so maybe this is enough?
Agreed. I'm happy with |
left a small comment on docs (not sure if we want to mention plugin as we don't really mention plugins anywhere else AFAIK)... other than that LGTM |
…ripts for Plugins can now define multiple operations/contexts that they use scripts for. Fine-grained settings can then be used to enable/disable scripts based on each single registered context. Also added a new generic category called `plugin`, which will be used as a default when the context is not specified. This allows us to restore backwards compatibility for plugins on `ScriptService` by restoring the old methods that don't require the script context and making them internally use the `plugin` context, as they can only be called from plugins. Closes #10347 Closes #10419
ping @dadoonet you might want to update plugins to use the plugin category, or even define their own script context. |
From elastic/elasticsearch#10419 We need to adapt river code from es-1.x (1.6.0) Closes #99.
From elastic/elasticsearch#10419 We need to adapt river code from es-1.x (1.6.0) Closes #98.
From elastic/elasticsearch#10419 We need to adapt river code from es-1.x (1.6.0) Closes #98. (cherry picked from commit e0bb184)
Plugins can now define multiple operations/contexts that they use scripts for. Fine-grained settings can then be used to enable/disable scripts based on each single registered context.
Also added a new generic category called
plugins
, which will be used as a default when the context is not specified. This allows us to restore backwards compatibility for plugins onScriptService
by restoring the old methods that don't require the script context and making them internally use theplugins
context, as they can only be called from plugins.Closes #10347