Cleanup ScriptService & friends in preparation for #6418

[javanna]

While working on #6418, I found myself cleaning some things up around scripting code, which I figured would be better to isolate on a different PR, to make the fine grained settings change easier to review.

• Added NAME constants for each script language, avoiding to repeat the same strings all over the place.
• Simplified compile method signatures by removing a couple of variants. Note that all of these signatures are going to change again with Allow fine-grained script settings #6418 as in order to compile/execute a script the caller will need to specify which operation is attempting to execute the script, info that will be provided as an additional mandatory argument.
• Removed double call to ScriptService#verifyDynamicScripting for every indexed or dynamic script.
• Decreased ScriptService inner classes visibility to private (CacheKey, IndexedScript, ApplySettings)
• Moved ScriptService inner classes to the bottom of the class, I think it makes it more readable.
• Resolved some compiler warnings

[dadoonet]

@javanna Just a reminder here for when the PR is merged to also fix :

• https://github.com/elasticsearch/elasticsearch-lang-javascript
• https://github.com/elasticsearch/elasticsearch-lang-python
• https://github.com/elasticsearch/elasticsearch-lang-mvel

in their es-1.x and master branches.

I believe there will be changes to port there.

I think also that some river plugins might not compile anymore (test part) as if IIRC we are doing some script tests there (couchdb, rabbitmq).

[javanna]

HI @dadoonet not sure what changes you mean that we should port there. In my mind these changes shouldn't break anything in scripting plugins, they shouldn't access the ScriptService directly. As for rivers, that might be the case, but then those compile signatures would be broken anyway for #6418 , which should be ok.

[colings86]

Probably not something for this PR but we now have a convention of using script for inline, script_file for file and script_id for indexed scripts (using the ScriptParameterParser to maintain the consistency). Should we follow that convention here? I can see we may want to keep the inline as query but maybe we should also support script for inline? and file and id can probably be replaced with script_file and script_id?

[javanna]

sounds good to me, patches welcome :)

[colings86]

I opened #9995 for this :)

[dadoonet]

@javanna Ignore me. I got confused by Github diff and thought you removed inner classes we are using in plugins. It's not the case. Sorry for the false alarm.

[javanna]

I think you read my mind @dadoonet I wanted to do what you feared badly :) but I went for not breaking bw comp. I will probably do that on master only later on.

[jpountz]

LGTM

[dadoonet]

@javanna It effectively breaks: See https://github.com/elastic/elasticsearch-river-couchdb/blob/master/src/main/java/org/elasticsearch/river/couchdb/CouchdbRiver.java#L145-145

The method signature changed. I need to fix that I guess. :)

We just need to change

Maps.newHashMap()

to

Maps.<String, Object>newHashMap()

[javanna]

Marked this as breaking. It breaks plugins that make use of scripting by depending on ScriptService (e.g. some rivers that allow to transform documents before indexing them). It doesn't break plugins that implement a scripting engine though.