Update by Query is modified to accept short `script` parameter. #26841

pozhidaevak · 2017-09-29T17:01:17Z

This change adding support of both long and short form of script parameter to _update_by_query request.
It should close the issue #24898 .

Changes in code:
static function RestUpdateByQueryAction:parseScript() is modified to accept Object parameter.
If Object is an instance of Map then it is a long form of script and old version of this function will be executed.
If Object is an instance of String then Script object will be created by using Script(String) constructor.

All tests are passed or ignored. However, gradle check is failed with the next message (I think it is not connected with my changes * What went wrong:
Execution failed for task ':docs:integTestCluster#installAnalysisIcuPlugin'.

A problem occurred starting process 'command '/Users/anton/Private/Coding/elasticsearch/docs/build/cluster/integTestCluster node0/elasticsearch-7.0.0-alpha1-SNAPSHOT/bin/elasticsearch-plugin''
`

…s issue elastic#24898

elasticmachine · 2017-09-29T17:01:19Z

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

nik9000

I left some minor requests for changes but it looks good to me! Thanks for doing this and I'm sorry for leading you astray with my mistaken comments earlier.

nik9000 · 2017-10-10T13:46:36Z

modules/reindex/src/main/java/org/elasticsearch/index/reindex/RestUpdateByQueryAction.java

+        assert config != null : "Script should not be null";
+
+        if (config instanceof String) { 
+                return new Script((String) config);


Can you fix the indentation on this to line up?

nik9000 · 2017-10-10T13:47:46Z

modules/reindex/src/main/java/org/elasticsearch/index/reindex/RestUpdateByQueryAction.java

-        return new Script(type, lang, script, params);
+            return new Script(type, lang, script, params);
+        }
+        else {


We usually don't put a newline between } and else. Can you undo?

nik9000 · 2017-10-10T13:48:42Z

@elasticmachine, test this please.

nik9000 · 2017-10-10T13:51:11Z

Can you add a test for this? I'd add it to qa/smoke-test-reindex-with-all-modules/src/test/resources/rest-api-spec/test/reindex/update_by_query/10_script.yml.

pozhidaevak · 2017-10-10T13:57:41Z

@nik9000, thanks for comments.
I'll fix mistakes, add tests and commit it to this branch

pozhidaevak · 2017-10-10T17:08:46Z

I created a test and tried to launch it, but I it's not executing. So, I've created a topic on elastic forum with description of the error

pozhidaevak · 2017-10-10T18:43:45Z

@nik9000, Thanks for help with testing. I pushed formatting changes and a new test

nik9000 · 2017-10-11T22:05:09Z

Thanks @pozhidaevak! I've merged to master and will cherry-pick into 6.x and push that after the tests pass for me locally.

I didn't think to ask you before, but would you like to write the documentation update for this? This page is generated by docs/reference/doc/update-by-query.asciidoc. The instructions for building the docs are here or you can just make the change blind and I'll check it if you submit a PR. All the snippets in the docs are checked as part of the build. So if you add a snippet you can test that it works with gradle docs:check. That'll test all the snippets in the docs but that only takes a few minutes. If your snippet fails it'll print out reproduction steps you can use to rerun just that snippet.

Update by Query is modified to accept short `script` parameter. Closes issue #24898

pozhidaevak · 2017-10-12T07:26:15Z

@nik9000, yah, I’ll add a documentation. Do I need to mention full version from which short form of script is accepted by API? If so, what is the version?

nik9000 · 2017-10-12T14:06:02Z

@pozhidaevak you don't need to mention the version in the docs because I'll only backport your documentation change only to the branches that have the change. When the targeted version is released we'll flip "current" version of the docs to the version with your change. Folks that browse older versions of the docs won't get docs for your change.

The targeted version for this is 6.1/7.0. The master branch that you developed it against is 7.0 and the 6.x branch that I backported it to is planned to be 6.1.

* master: (35 commits) Create weights lazily in filter and filters aggregation (#26983) Use a dedicated ThreadGroup in rest sniffer (#26897) Fire global checkpoint sync under system context Update by Query is modified to accept short `script` parameter. (#26841) Cat shards bytes (#26952) Add support for parsing inline script (#23824) (#26846) Change default value to true for transpositions parameter of fuzzy query (#26901) Adding unreleased 5.6.4 version number to Version.java Rename TCPTransportTests to TcpTransportTests (#26954) Fix NPE for /_cat/indices when no primary shard (#26953) [DOCS] Fixed indentation of the definition list. Fix formatting in channel close test Check for closed connection while opening Clarify systemd overrides [DOCS] Plugin Installation for Windows (#21671) Painless: add tests for cached boxing (#24163) Don't detect source's XContentType in DocumentParser.parseDocument() (#26880) Fix handling of paths containing parentheses Allow only a fixed-size receive predictor (#26165) Add Homebrew instructions to getting started ...

* 6.x: (32 commits) Use a dedicated ThreadGroup in rest sniffer (#26897) Fire global checkpoint sync under system context Update by Query is modified to accept short `script` parameter. (#26841) Cat shards bytes (#26952) Adding unreleased 5.6.4 version number to Version.java Rename TCPTransportTests to TcpTransportTests (#26954) Fix NPE for /_cat/indices when no primary shard (#26953) [DOCS] Fixed indentation of the definition list. Check for closed connection while opening Clarify systemd overrides [DOCS] Plugin Installation for Windows (#21671) Painless: add tests for cached boxing (#24163) Don't detect source's XContentType in DocumentParser.parseDocument() (#26880) Return List instead of an array from settings (#26903) Emit deprecation warning for variable size predictor Fix handling of paths containing parentheses Deprecate variable-size receive predictor Add Homebrew instructions to getting started ingest: Fix bug that prevent date_index_name processor from accepting timestamps specified as a json number Scripting: Fix expressions to temporarily support filter scripts (#26824) ...

pozhidaevak · 2017-10-12T19:35:39Z

@nik9000, thanks for explaining.
I also think it will be useful to update docs/reference/modules/scripting/using.asciidoc.

I think I've found an inconsistency between code and documentation in the code we have painless as the default language. From Script.java:

public static final String DEFAULT_SCRIPT_LANG = "painless";
//some code is omitted

// this constructor is used to create simple script
public Script(String idOrCode) {
        this(DEFAULT_SCRIPT_TYPE, DEFAULT_SCRIPT_LANG, idOrCode, Collections.emptyMap(), Collections.emptyMap());
    }

But in the documentation it is stated (from using.asciidoc):

The default language may be changed in the elasticsearch.yml config file by
setting script.default_lang to the appropriate language.

However, it needs to be checked by running some tests...

rjernst · 2017-10-12T20:29:54Z

script.default_lang was removed back in 5.0. So those docs are just leftover and should be removed.

* master: (356 commits) Do not set SO_LINGER on server channels (elastic#26997) Fix inconsistencies in the rest api specs for *_script (elastic#26971) fix inconsistencies in the rest api specs for cat.snapshots (elastic#26996) Add docs on full_id parameter in cat nodes API [TEST] Add test that replicates versioned updates with random flushes Use internal searcher for all indexing related operations in the engine Reformat paragraph in template docs to 80 columns Clarify settings and template on create index Fix reference to TcpTransport in documentation Allow Uid#decodeId to decode from a byte array slice (elastic#26987) Fix a typo in the similarity docs (elastic#26970) Use separate searchers for "search visibility" vs "move indexing buffer to disk (elastic#26972) Create weights lazily in filter and filters aggregation (elastic#26983) Use a dedicated ThreadGroup in rest sniffer (elastic#26897) Fire global checkpoint sync under system context Update by Query is modified to accept short `script` parameter. (elastic#26841) Cat shards bytes (elastic#26952) Add support for parsing inline script (elastic#23824) (elastic#26846) Change default value to true for transpositions parameter of fuzzy query (elastic#26901) Adding unreleased 5.6.4 version number to Version.java ...

Update by Query is modified to accept short script parameter. Close…

3b07cfb

…s issue elastic#24898

nik9000 requested changes Oct 10, 2017

View reviewed changes

Minor formating and style changes

316ef86

Adding a test to verify short form of script parameter

1b9a6b2

nik9000 approved these changes Oct 11, 2017

View reviewed changes

nik9000 merged commit cee9640 into elastic:master Oct 11, 2017

nik9000 pushed a commit that referenced this pull request Oct 11, 2017

Update by Query is modified to accept short script parameter. (#26841)

b0136a1

Update by Query is modified to accept short `script` parameter. Closes issue #24898

nik9000 added :Reindex API >enhancement v6.1.0 v7.0.0 labels Oct 12, 2017

nik9000 mentioned this pull request Oct 16, 2017

Update docs about script parameter #27010

Merged

lcawl added :Distributed/CRUD A catch all label for issues around indexing, updating and getting a doc by id. Not search. and removed :Reindex API labels Feb 13, 2018

dakrone mentioned this pull request Mar 20, 2018

Update by query should support short script form and doc #24898

Closed

colings86 added the v7.0.0-beta1 label Feb 7, 2019

colings86 removed the v7.0.0 label Feb 7, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update by Query is modified to accept short `script` parameter. #26841

Update by Query is modified to accept short `script` parameter. #26841

pozhidaevak commented Sep 29, 2017

elasticmachine commented Sep 29, 2017

nik9000 left a comment

nik9000 Oct 10, 2017

nik9000 Oct 10, 2017

nik9000 commented Oct 10, 2017

nik9000 commented Oct 10, 2017

pozhidaevak commented Oct 10, 2017

pozhidaevak commented Oct 10, 2017

pozhidaevak commented Oct 10, 2017

nik9000 commented Oct 11, 2017

pozhidaevak commented Oct 12, 2017

nik9000 commented Oct 12, 2017

pozhidaevak commented Oct 12, 2017

rjernst commented Oct 12, 2017 •

edited

Loading

Update by Query is modified to accept short script parameter. #26841

Update by Query is modified to accept short script parameter. #26841

Conversation

pozhidaevak commented Sep 29, 2017

elasticmachine commented Sep 29, 2017

nik9000 left a comment

Choose a reason for hiding this comment

nik9000 Oct 10, 2017

Choose a reason for hiding this comment

nik9000 Oct 10, 2017

Choose a reason for hiding this comment

nik9000 commented Oct 10, 2017

nik9000 commented Oct 10, 2017

pozhidaevak commented Oct 10, 2017

pozhidaevak commented Oct 10, 2017

pozhidaevak commented Oct 10, 2017

nik9000 commented Oct 11, 2017

pozhidaevak commented Oct 12, 2017

nik9000 commented Oct 12, 2017

pozhidaevak commented Oct 12, 2017

rjernst commented Oct 12, 2017 • edited Loading

Update by Query is modified to accept short `script` parameter. #26841

Update by Query is modified to accept short `script` parameter. #26841

rjernst commented Oct 12, 2017 •

edited

Loading