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

Unify script and template requests across codebase #11164

Merged
merged 1 commit into from May 29, 2015
Merged

Unify script and template requests across codebase #11164

merged 1 commit into from May 29, 2015

Conversation

colings86
Copy link
Contributor

This change unifies the way scripts and templates are specified for all instances in the codebase. It builds on the Script class added previously and adds request building and parsing support as well as the ability to transfer script objects between nodes. It also adds a Template class which aims to provide the same functionality for template APIs.

Note: This PR maintains backwards compatibility with versions 1.5 and before. There will be a separate PR to remove the backwards compatibility in 2.0 once this is merged (this will also include updating the "Breaking changes in 2.0" doc.

Closes #11091
Closes #10810
Closes #10113

private static final ScriptParser PARSER = new ScriptParser();

private String script;
private ScriptType type;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd prefer this to be @Nullable as well... relates to xcontent serialization

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if we do make it nullable @colings86 we have to make sure some value gets provided as part of the canExecuteScript call, where the type is required to decide whether the script can be executed or not.

@colings86 colings86 added review and removed WIP labels May 18, 2015
@@ -360,12 +360,6 @@ Customized scores can be implemented via a script:
Scripts can be inline (as in above example), indexed or stored on disk. For details on the options, see <<modules-scripting, script documentation>>.
Parameters need to be set as follows:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line looks to be redundant

.addRange(date(2, 15), date(3, 15))
.addUnboundedFrom(date(3, 15)))
.execute().actionGet();
.script(new Script("doc['dates'].values")).addUnboundedTo(date(2, 15))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indentation

@markharwood
Copy link
Contributor

LGTM - I've had to skim through a lot of stuff without review on the assumption that

  1. Some changes are formatting only and
  2. much of the "new" code is a cut and paste of old test code renamed to testFooUsingOldScriptApi

@colings86
Copy link
Contributor Author

@markharwood thanks for the review, I've pushed new commit

if (!super.equals(obj))
return false;
if (getClass() != obj.getClass())
return false;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this getClass() test is not necessary, super.equals already takes care of it

@jpountz
Copy link
Contributor

jpountz commented May 29, 2015

This PR is mostly impossible to review due to its size, but I skimmed through the patch and it looks good overall. The main concern is about some exception changes that could trigger serialization errors on clusters that have mixed versions, but that should be easy to fix. Let's not wait to long before pushing, such large pull requests can easily become painful to rebase.

@colings86 colings86 removed the v1.6.0 label May 29, 2015
@jpountz
Copy link
Contributor

jpountz commented May 29, 2015

LGTM, I think we just need to rename scriptObject() to script() on UpdateRequest

This change unifies the way scripts and templates are specified for all instances in the codebase. It builds on the Script class added previously and adds request building and parsing support as well as the ability to transfer script objects between nodes. It also adds a Template class which aims to provide the same functionality for template APIs

Closes #11091
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 v2.0.0-beta1
Projects
None yet
6 participants