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

Better categories for scripted operations #10347

Closed
javanna opened this issue Mar 31, 2015 · 7 comments
Closed

Better categories for scripted operations #10347

javanna opened this issue Mar 31, 2015 · 7 comments
Assignees
Labels
:Core/Infra/Scripting Scripting abstractions, Painless, and Mustache >enhancement v1.6.0 v2.0.0-beta1

Comments

@javanna
Copy link
Member

javanna commented Mar 31, 2015

With #10116 we introduced fine-grained script settings. Each time a script is run, it is associated with a context, which holds the operation/api that is making use of the script, provided by the caller.

We currently support 4 operations: search, mapping, update and aggs. We folded suggester and percolator under search though, which might be confusing for users, hence we might want to create proper distinct categories for them.

Even more importantly, plugins may use our ScriptService to expose custom scripted operations, which may not fall into these pre-defined categories. I wonder if we should introduce a new generic category of operations for plugins, which can be enabled/disabled via settings too. This way we may also be able to remove the breaking aspect of this new feature, the problem being that each caller needs to specify which operation it's using the script for, and that is a required argument. Given that we updated all of our internal calls to provide that argument, we could now restore the original methods without the ScriptContext argument and assume that those calls come from plugins, by assigning them the default plugins category.

@javanna javanna added >enhancement discuss :Core/Infra/Scripting Scripting abstractions, Painless, and Mustache labels Mar 31, 2015
@javanna
Copy link
Member Author

javanna commented Mar 31, 2015

hey @uboness @clintongormley can you comment on this please?

@dadoonet
Copy link
Member

dadoonet commented Apr 1, 2015

@javanna for now, plugins I know basically use scripts to modify documents before injection. That was mostly the case for some rivers.
So update works well here. All official plugins have now been updated for 1.6.

Not sure we need something else.

@javanna
Copy link
Member Author

javanna commented Apr 1, 2015

@dadoonet yea it makes sense to use update for now but it's ambiguous because it's not really about the update api, but pre-processing of document before they get indexed. I am for making it explicit that plugins may use scripts and we just don't know exactly for what :)

@uboness
Copy link
Contributor

uboness commented Apr 1, 2015

@javanna thinking a few things:

  • would be nice of the script context would be extensible... so some plugins will also be able to define their context in more fine grained fashion than a simple "capture all" plugin context. Right now the context is an enum, perhaps it should be based on opaque strings (internally we'll sill use the same constants that the enum already captures.
  • we can still have a "capture all" context (say other) that can be configured like all the others and have compile/execute methods overloaded so we'll also have those without the passed in context (in which case the ctx will default to other).... we can perhaps mark it as deprecated and remove it in 2.0 (so from 2.0 ctx will be a required param)?

@javanna
Copy link
Member Author

javanna commented Apr 1, 2015

I agree with @uboness on making things more flexible, maybe we could just use the plugin name though instead of having plugins declare potentially multiple additional contexts (think of conflicting ones as well). Let's see what others think about this aspect.

That said I think we should move on with the first step of introducing an additional category that is specific for plugins, which would also have the advantage of allowing us to restore backwards compatibility on ScriptService. Will work on this.

@uboness
Copy link
Contributor

uboness commented Apr 1, 2015

maybe we could just use the plugin name though instead of having plugins declare potentially multiple additional contexts (think of conflicting ones as well). Let's see what others think about this aspect.

I can see multiple categories used within a single plugin as well... so maybe then we'll have the category be prefixed with the plugin name... as a convention at least?

That said I think we should move on with the first step of introducing an additional category that is specific for plugins, which would also have the advantage of allowing us to restore backwards compatibility on ScriptService. Will work on this.

+1

javanna added a commit to javanna/elasticsearch that referenced this issue Apr 1, 2015
Plugins can make use of scripts and expose custom scripting features. Fine-grained settings introduced with elastic#10116 don't support any custom use of scripts, hence plugins are forced to choose between update, mapping, search and aggs. Introduced a new generic plugins category that can be used by plugins.

Fine-grained settings apply as well: `script.plugins: off` disable any script type, for any language, only when scripts are used from plugins. `script.engine.groovy.inline.plugins: off` disables scripts when used as part of plugins, only for inline scripts and groovy language.

Relates to elastic#10347
javanna added a commit to javanna/elasticsearch that referenced this issue Apr 1, 2015
Plugins can make use of scripts and expose custom scripting features. Fine-grained settings introduced with elastic#10116 don't support any custom use of scripts, hence plugins are forced to choose between update, mapping, search and aggs. Introduced a new generic plugins category that can be used by plugins.

Fine-grained settings apply as well: `script.plugins: off` disable any script type, for any language, only when scripts are used from plugins. `script.engine.groovy.inline.plugins: off` disables scripts when used as part of plugins, only for inline scripts and groovy language.

Relates to elastic#10347
@clintongormley
Copy link

+1 to #10347 (comment)

@javanna javanna self-assigned this Apr 2, 2015
javanna added a commit to javanna/elasticsearch that referenced this issue Apr 3, 2015
…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
javanna added a commit that referenced this issue Apr 8, 2015
…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
@javanna javanna closed this as completed in 7bd7ea8 Apr 8, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Core/Infra/Scripting Scripting abstractions, Painless, and Mustache >enhancement v1.6.0 v2.0.0-beta1
Projects
None yet
Development

No branches or pull requests

4 participants