Store indexed scripts in the cluster state instead of the `.scripts` index #17650

martijnvg · 2016-04-11T13:34:46Z

This PR also cleans up the cyclic dependency ScriptService has at the moment. By moving scripts to the cluster state, the Client dependency was no longer needed.

Open questions:

How to deal with scripts already stored in the .scripts index? Do we want to provide support for moving indexed scripts to the cluster state? Upon upgrading to 5.x the .scripts index will not be removed. We can provide a script that can be executed just after the upgrade. Or do we want to provide something that automatically moves scripts from the .script index to the cluster state, if so how?
The script.indexed.* settings haven't been renamed yet to script.stored.*. Are we okay with making a hard break here? Or do we want to deprecate script.indexed.* settings first. How everything is coded right now this doesn't seem to be straight forward.
(ScriptType.INDEXED enum has been renamed, but not its script type parameter)

PR for #16651

martijnvg · 2016-04-18T10:55:25Z

I also added a soft limit for scripts based on @colings86 suggestion.

martijnvg · 2016-04-18T12:20:34Z

The script.indexed.* settings haven't been renamed yet to script.stored.. Are we okay with making a hard break here? Or do we want to deprecate script.indexed. settings first. How everything is coded right now this doesn't seem to be straight forward. (ScriptType.INDEXED enum has been renamed, but not its script type parameter)

A node will not start if there are unknown settings, so during upgrading or test driving 5.x this will bubble up quickly.

clintongormley · 2016-04-19T08:32:44Z

i'm OK with a hard break here, as long as we provide a utility that the user can run to move scripts from indexed to stored. This could even be a shell script that the user can copy and paste from the migration docs.

colings86 · 2016-04-19T08:59:54Z

...ava/org/elasticsearch/rest/action/admin/cluster/storedscripts/RestPutStoredScriptAction.java

+    public void handleRequest(final RestRequest request, final RestChannel channel, Client client) {
+        PutStoredScriptRequest putRequest = new PutStoredScriptRequest(getScriptLang(request), request.param("id"));
+        putRequest.script(request.content());
+        client.admin().cluster().putStoredScript(putRequest, new AcknowledgedRestListener<>(channel));


Should these Rest Actions not forward the request to the Transport Actions so that we make sure that are always handled in the same way? I thought in general the rest actions usually forward to the transport actions so the REST API and Java API are guaranteed to work the same?

Actually, ignore this, the rest actions are actually just forwarding to the transport actions

colings86 · 2016-04-19T09:03:22Z

@martijnvg I left a couple of consistency comments but otherwise I think it looks good.

martijnvg · 2016-04-20T12:47:27Z

@colings86 I've addressed your comment and added docs.

martijnvg · 2016-04-20T12:49:13Z

docs/reference/migration/migrate_5_0/scripting.asciidoc

+
+for doc in helpers.scan(es, index=".scripts", preserve_order=True):
+	es.put_script(lang=doc['_type'], id=doc['_id'], body=doc['_source'])
+-----------------------------------


@honzakral Would you like to verify if this script is okay? I did need to add preserve_order=True, otherwise the scan helper would fail, because it uses search_type=scan which has been removed in 5.0.

I will do an updated version of the client for 5.0 where I will replace the search_type=scan with sort: _doc - it's already implemented in master so no need for it here.

Otherwise LGTM

thx, I will check if the script snippet without preserve_order=True.

colings86 · 2016-04-21T07:31:53Z

docs/reference/migration/migrate_5_0/scripting.asciidoc

+
+The response should include all your scripts from the `.scripts` index.
+After you have verified that all your scripts have been moved, optionally as a last step,
+you can delete the `.scripts` index as Elasticsearch no longer uses it.


I think it might be a good idea to add a note to the Java API section of the breaking changes doc meantioning the change of method names in the client from IndexScripts to StoredScripts?

colings86 · 2016-04-21T07:32:18Z

@martijnvg I left some more comments but I think its very close

martijnvg · 2016-04-21T09:35:18Z

@colings86 I've updated the PR and addressed your comments.

colings86 · 2016-04-21T10:52:16Z

LGTM

…the `.scripts` index. Also added max script size soft limit for stored scripts. Closes elastic#16651

awislowski · 2016-04-28T12:07:10Z

core/src/main/java/org/elasticsearch/script/ScriptMetaData.java

+                        builder.copyCurrentStructure(parser);
+                    }
+                    break;
+                case "template":


bodies in both cases are the same, why not write just:
case "script":
case "template":
.....

Good point. It has been removed: 22dc06f

Mpdreamz · 2016-05-01T20:45:23Z

@martijnvg small request the current exception message when es.script.indexed=true is passed is:

System.Exception: Exception in thread "main" java.lang.IllegalArgumentException: unknown setting [script.indexed] did you mean any of [script.inline, script.ingest]?

Where es.script.stored=true is the more logical equivalent to suggest (this of the latest snapshot, have not tested HEAD of master).

clintongormley · 2016-05-02T11:12:40Z

@Mpdreamz yeah i had this as well, it's because of the fuzzy matching we're using on incorrect settings. That said, I think we should add a hardcoded rule when script.indexed is used to recommend script.stored instead.

martijnvg · 2016-05-19T08:58:10Z

@Mpdreamz @clintongormley I like the idea of a hard coded rule, but there is no infrastructure for that. Adding that doesn't seem to be worth the effort for this breaking change.

martijnvg added WIP :Indexed Scripts/Templates v5.0.0-alpha2 labels Apr 11, 2016

martijnvg force-pushed the stored_scripts branch from 8322b8e to 816f53a Compare April 14, 2016 07:31

martijnvg added >breaking review and removed WIP labels Apr 18, 2016

martijnvg force-pushed the stored_scripts branch 2 times, most recently from 10b8a32 to 89b0adc Compare April 18, 2016 10:54

martijnvg force-pushed the stored_scripts branch 5 times, most recently from e9e58a7 to 9e5c5fa Compare April 18, 2016 15:47

colings86 reviewed Apr 19, 2016
View reviewed changes

martijnvg force-pushed the stored_scripts branch 2 times, most recently from a6157bd to 685c95c Compare April 20, 2016 12:46

martijnvg reviewed Apr 20, 2016
View reviewed changes

colings86 reviewed Apr 21, 2016
View reviewed changes

martijnvg force-pushed the stored_scripts branch from 54b3261 to 885c154 Compare April 21, 2016 11:47

martijnvg removed the review label Apr 21, 2016

Changed indexed scripts to be stored in the cluster state instead of …

c5ad2e2

…the `.scripts` index. Also added max script size soft limit for stored scripts. Closes elastic#16651

martijnvg force-pushed the stored_scripts branch from 885c154 to c5ad2e2 Compare April 22, 2016 11:43

martijnvg merged commit c5ad2e2 into elastic:master Apr 22, 2016

clintongormley removed the :Internal label Apr 25, 2016

awislowski reviewed Apr 28, 2016
View reviewed changes

imotov mentioned this pull request Jul 26, 2016

upgrade to 5.0.0-alpha4 brwe/es-token-plugin#1

Merged

This was referenced Jan 9, 2018

Docs: Are there outdated APIs (e.g., AggregatorBuilder) in Java API docs? #28114

Closed

Stored script related docs of Java API are outdated #28144

Closed

clintongormley added :Core/Infra/Scripting Scripting abstractions, Painless, and Mustache and removed :Indexed Scripts/Templates labels Feb 13, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Store indexed scripts in the cluster state instead of the `.scripts` index #17650

Store indexed scripts in the cluster state instead of the `.scripts` index #17650

martijnvg commented Apr 11, 2016 •

edited

Loading

martijnvg commented Apr 18, 2016

martijnvg commented Apr 18, 2016

clintongormley commented Apr 19, 2016

colings86 Apr 19, 2016

colings86 Apr 19, 2016

colings86 commented Apr 19, 2016

martijnvg commented Apr 20, 2016

martijnvg Apr 20, 2016

honzakral Apr 20, 2016

martijnvg Apr 20, 2016

colings86 Apr 21, 2016

colings86 commented Apr 21, 2016

martijnvg commented Apr 21, 2016

colings86 commented Apr 21, 2016

awislowski Apr 28, 2016

martijnvg Apr 28, 2016

Mpdreamz commented May 1, 2016

clintongormley commented May 2, 2016

martijnvg commented May 19, 2016

Store indexed scripts in the cluster state instead of the .scripts index #17650

Store indexed scripts in the cluster state instead of the .scripts index #17650

Conversation

martijnvg commented Apr 11, 2016 • edited Loading

martijnvg commented Apr 18, 2016

martijnvg commented Apr 18, 2016

clintongormley commented Apr 19, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

colings86 commented Apr 19, 2016

martijnvg commented Apr 20, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

colings86 commented Apr 21, 2016

martijnvg commented Apr 21, 2016

colings86 commented Apr 21, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Mpdreamz commented May 1, 2016

clintongormley commented May 2, 2016

martijnvg commented May 19, 2016

Store indexed scripts in the cluster state instead of the `.scripts` index #17650

Store indexed scripts in the cluster state instead of the `.scripts` index #17650

martijnvg commented Apr 11, 2016 •

edited

Loading