Begin to break templates away from scripts #23744

nik9000 · 2017-03-24T21:43:33Z

First: I'm genuinely sorry to whoever reviews this. It is big and will need to be looked at carefully.

This starts the process of breaking scripts and templates. I've intentionally preserved abstractions that will be lost eventually in an attempt to keep this change as small as I figured I could get away with while still enabling us to change things later. I believe I've preserved 95% of backwards compatibility. This is what I know I've broken:

The plugin interface
The transport client with regard to templates
The stored template APIs during the 5.x to 6.0 upgrade. They should work just fine outside of the upgrade.

Here are some things I intentionally didn't change that we probably will want to change later:

Internally we use the Script object to refer to templates. This is pretty silly and the new TemplateService doesn't require it. I didn't change this to keep the PR small. It should be a followup.
I believe as a followup to that we should be able to remove some things from Script.
TemplateService.Backend is the same as ScriptEngineService. I didn't change this to keep the PR small. It should be a followup.

Things I didn't change that we might want to change at some point but we'll need to be super careful about so we don't break backwards compatibility:

File templates are still loaded from the config/scripts directory every few seconds.
Templates stored in the cluster state are still stored in the ScriptMetaData.
Statistics related to templates and scripts are still collected together.

Originally I'd planned to backport this to 5.x as well as commit to master but right now I don't have any appetite for it.

nik9000 · 2017-03-24T21:43:57Z

@dakrone this is what I told you about earlier. I don't quite have all the tests passing, but I'm oh so very close.

nik9000 · 2017-03-24T22:01:54Z

So my goal here is to keep this as small as I could. I know there is a lot more to do here but I didn't want to copy and paste and then de-duplicate because it is so hard to find the duplicates when you do that. So instead I made two classes for the behaviors that I thought scripts and templates shared and relied on them in ScriptService and TemplateService. Namely CachingCompiler (bad name) which provides all the behaviors around when to compile and when to cache and ScriptPermits which implements the permissions checking for scripts and templates.

It ain't perfect, but it is a start.

nik9000 · 2017-03-24T22:08:14Z

core/src/main/java/org/elasticsearch/common/xcontent/XContentFactory.java

    @Deprecated
    public static XContent xContent(BytesReference bytes) {
        XContentType type = xContentType(bytes);
-        if (type == null) {


Oh my - I'm not sure why this is here......

Are you sure we can remove this? (it's not too large of a change, it trades this exception for IllegalArgumentException("Cannot get xcontent for unknown type") )

I honestly think this was a leftover. I'm adding it back.

nik9000 · 2017-03-24T22:14:35Z

core/src/main/java/org/elasticsearch/indices/IndicesService.java

                          MapperRegistry mapperRegistry, NamedWriteableRegistry namedWriteableRegistry,
                          ThreadPool threadPool, IndexScopedSettings indexScopedSettings, CircuitBreakerService circuitBreakerService,
-                          BigArrays bigArrays, ScriptService scriptService, ClusterService clusterService, Client client,
+                          BigArrays bigArrays, ScriptService scriptService, TemplateService templateService, Client client,


This didn't actually need ClusterService....

nik9000 · 2017-03-24T22:16:12Z

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

+ * Manages caching, resource watching, permissions checking, and compilation of scripts (or
+ * templates).
+ */
+public abstract class CachingCompiler<CacheKeyT> implements ClusterStateListener {


I'm not really a fan of the name. It just delegates to actual compilers.

In this file I constantly say "script" when I mean "script or template". I'm not sure of a better way to say that.

nik9000 · 2017-03-24T22:17:31Z

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

     * {@link ScriptMetaData} adding the specified stored script.
     */
-    static ScriptMetaData putStoredScript(ScriptMetaData previous, String id, StoredScriptSource source) {
+    public static ScriptMetaData putStoredScript(ScriptMetaData previous, String id, StoredScriptSource source) {


Public so lang-mustache can use them.

nik9000 · 2017-03-24T22:18:54Z

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

-        scriptSettings = new ScriptSettings(scriptEngineRegistry, scriptContextRegistry);
+        ScriptMetrics scriptMetrics = new ScriptMetrics();
+        // Note that if templateBackend is null this won't register any settings for it
+        scriptSettings = new ScriptSettings(scriptEngineRegistry, templateBackend, scriptContextRegistry);


It is useful for this to be able to handle a null templateBackend so we let this stay null here.

nik9000 · 2017-03-24T22:19:41Z

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

        }
+
+        if (templateBackend == null) {
+            templateBackend = new TemplatesUnsupportedBackend();


But now we have to feed TemplateService something so we feed it a backend that just throws exceptions telling the user they don't have a template backend.

nik9000 · 2017-03-24T22:22:21Z

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

+
+        this.backend = backend;
+        this.scriptPermits = new ScriptPermits(settings, scriptSettings, scriptContextRegistry);
+        this.compiler = new CachingCompiler<String>(settings, env, resourceWatcherService,


We hardcode mustache in so many places I figured we really only need to support a single template language at a time. If we want to support multiple then we can revisit, but for now I went with only one at a time.

nik9000 · 2017-03-25T15:04:03Z

Ok. I've added content-type support.

dakrone

I left a bunch of comments

dakrone · 2017-03-27T15:06:39Z

core/src/main/java/org/elasticsearch/common/xcontent/XContentFactory.java

    @Deprecated
    public static XContent xContent(BytesReference bytes) {
        XContentType type = xContentType(bytes);
-        if (type == null) {


Are you sure we can remove this? (it's not too large of a change, it trades this exception for IllegalArgumentException("Cannot get xcontent for unknown type") )

dakrone · 2017-03-27T15:13:07Z

core/src/main/java/org/elasticsearch/ingest/IngestService.java

-    public IngestService(Settings settings, ThreadPool threadPool,
-                         Environment env, ScriptService scriptService, AnalysisRegistry analysisRegistry,
+    public IngestService(Settings settings, ThreadPool threadPool, Environment env,
+                         org.elasticsearch.script.TemplateService esTemplateService,


It's unfortunate that we have an overlap here in class names :( I'm tempted to suggest renaming it to ScriptTemplateService or the ingest one to IngestTemplateService, not sure how large a change that would be though.

dakrone · 2017-03-27T15:14:05Z

core/src/main/java/org/elasticsearch/ingest/InternalTemplateService.java


-    InternalTemplateService(ScriptService scriptService) {
-        this.scriptService = scriptService;
+    InternalTemplateService(org.elasticsearch.script.TemplateService templateService) {


Yeah this is really ugly and prone to mistakes, I'm in favor of renaming the ingest version to IngestTemplateService if you don't think that'd be too disruptive?

I like the idea, yeah.

dakrone · 2017-03-27T15:16:50Z

core/src/main/java/org/elasticsearch/plugins/ScriptPlugin.java


+    /**
+     * Returns a {@link TemplateService.Backend} if this plugin implements a template backend or null if it doesn't. Note that Elasticsearch
+     * will refuse to start if there is more than one template backend and it is bundled with Mustache. To replace that backend you'd have


If this is the case, why do we support that? It seems like we should only support Mustache and nothing else if we think it may be unstable/untested when removed.

That was pretty much my intent. Templates are in a module for permission isolation reasons but, yeah, we really don't support anything else. You could try but it isn't likely to work.

Would it be good enough to make this comment reflect that better?

Yeah that sounds good to me.

dakrone · 2017-03-27T15:24:59Z

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

+
+        // add file watcher for file scripts and templates
+        scriptsDirectory = env.scriptsFile();
+        if (logger.isTraceEnabled()) {


No need for this protection, I believe the logger.trace method has it already, and the arguments aren't evaluating anything expensive

This was copied from ScriptService. I can drop it though.

dakrone · 2017-03-27T17:05:33Z

...ustache/src/main/java/org/elasticsearch/script/mustache/stored/PutStoredTemplateRequest.java

+            validationException = addValidationError("must specify id for stored search template",
+                    validationException);
+        } else
+            if (id.contains("#")) {


Validate the template doesn't start with _ also I think

dakrone · 2017-03-27T17:06:09Z

...ustache/src/main/java/org/elasticsearch/script/mustache/stored/PutStoredTemplateRequest.java

+            // ignore
+        }
+
+        return "put search template {id [" + id + "], content [" + source + "]}";


Same comment here about following the same pattern for these (regardless of what the pattern ends up being)

dakrone · 2017-03-27T17:07:21Z

...stache/src/main/java/org/elasticsearch/script/mustache/stored/PutStoredTemplateResponse.java

+    @Override
+    public void readFrom(StreamInput in) throws IOException {
+        super.readFrom(in);
+        readAcknowledged(in);


super.readFrom(in) already calls readAcknowledged, you're reading and writing the same boolean twice in these serialization functions

I don't think it does actually. It probably ought to, but it doesn't seem to.

dakrone · 2017-03-27T17:11:54Z

...in/java/org/elasticsearch/script/mustache/stored/TransportPutStoredSearchTemplateAction.java

+    @Override
+    protected void masterOperation(PutStoredTemplateRequest request, ClusterState state,
+            ActionListener<PutStoredTemplateResponse> listener) throws Exception {
+        if (request.content().length() > maxScriptSizeInBytes) {


Don't we already check this on the script service? I think we should delegate it there rather than having the transport layer do that

After this change ScriptService doesn't get involved in stored templates.

ScriptService does this check for scripts, yes, but I couldn't mirror that behavior for templates because these requests are in lang-mustache and TemplateService is in core.

I can do a few things:

Move the template requests to core. We'd talked about not wanting to do that but I'm not 100% clear on the reasoning.

Make a StoredTemplatesService that does all the stored template work and leave it in lang-mustache.

dakrone · 2017-03-27T17:12:27Z

...in/java/org/elasticsearch/script/mustache/stored/TransportPutStoredSearchTemplateAction.java

+        try {
+            templateService.checkCompileBeforeStore(source);
+        } catch (IllegalArgumentException | ScriptException e) {
+            throw new IllegalArgumentException("failed to parse/compile stored search template ["


Same here, I'm pretty sure I've seen the same exception in the script service, are we catching and then basically duplicating the exception message?

In this case we're literally copy and pasting the code for storage. In this case I wanted to duplicate the code because I know the script code is going to drift from the template code significantly and I want us to keep the template code as is while we do it.

nik9000 · 2017-03-27T18:32:39Z

I left a bunch of comments

Thanks for going through it.

They don't help in log4j2. They are a leftover from a bygone age.

Also drop an unnecessary null check.

Move them to the superclass and move them to Optional

rjernst · 2017-03-27T19:29:12Z

@nik9000 Thank you for working on this! I know it is a huge undertaking, and it is sorely needed. I have thought many times about this over the last year, but have never taken the initiative to do the grunt work.

With that said, I think this PR is far too large to adequately review. It changes too many things at once, and ancillary changes exist which could be done as individual PRs. For example, the separation of script caching from the script service could be done on its own.

In general, I think that the way to do this in smaller steps is to start from the consumers. We currently have the single Script class which all consumers use (both java api, as well as internal). I think the very first step is to remove the final from that class, and have a subclass called Template (which can eventually be a separate class, but this is a small step which is digestible). Then, add a compileTemplate method to ScriptService, and migrate uses of Script to Template in manageable chunks. After that, we will know where all the uses are, and you can add deprecations in 5.x to the java api when trying to use the script methods with lang mustache, indicating they should use the template methods. This has the added benefit that the internal migration does not have to happen all in one giant change. Behind the scenes then, in subsequent changes, we can migrate callers of compileTemplate to switch to a new TemplateService, internally migrate stored scripts that are for lang mustache to a separate cluster state location, etc.

I think this last part is incredibly important to maintain backcompat during this split. @jdconrad had a giant PR months ago that tried adding contexts to scripts. However, it was rejected on the premise that the PR was too large too review, and it broke backcompat in many ways. At this point I think maintaining backcompat is something we must strive for, however possible. Having something “not work” when upgrading, even if only while the system is in a mixed version state, breaks with our intention to give stronger backcompat guarantees, especially in providing major version rolling upgrades.

nik9000 · 2017-03-27T20:19:54Z

Fair enough @rjernst. If you think this breaks too many things then I won't do it. I don't think I'm going to find the time or willpower start over your way for a while though.

nik9000 added 17 commits March 2, 2017 17:40

uhg

118de21

I feel dirty

4867848

Closer

085749b

Something I forgot:wq

b5f16fd

Merge branch 'master' into de_script_templates_1

5b82a73

Merge branch 'master' into de_script_templates_1

ea63c2d

Line length

58a5ad9

Line length

e2e7f3d

Merge branch 'master' into de_script_templates_1

c5c9fbf

Fix file templates

88be3a5

More tests

7df0a16

Docs

6b36b43

No more NOCOMMITs!

1471ebd

Last nocommit for real this time

b8f0f08

TemplateService in createComponents

31d733f

Many renames

706ca6d

Migration notes for template/script split

e06738e

nik9000 added :Core/Infra/Scripting Scripting abstractions, Painless, and Mustache >breaking-java review v6.0.0-alpha1 WIP and removed review labels Mar 24, 2017

nik9000 added 2 commits March 24, 2017 17:48

Drop duplicate log message

42fb904

Remove leftover

207e417

nik9000 added review and removed WIP labels Mar 24, 2017

nik9000 commented Mar 24, 2017

View reviewed changes

nik9000 added review and removed WIP labels Mar 25, 2017

Line length

0714889

dakrone requested changes Mar 27, 2017

View reviewed changes

nik9000 added 13 commits March 27, 2017 14:33

Remove logging guards

8d1165e

They don't help in log4j2. They are a leftover from a bygone age.

Remove leftover.

f0d70bc

Rename method

eccf17d

requireNonNull with messages

6f00573

Better comments in checkCompileBeforeStore

ac1b823

Also drop an unnecessary null check.

No extension

9360cfa

Change exception type

558b26b

Some javadoc

8a58c24

More javadocs

d7ff55b

Drop funny parsing for scripts that should only be there for templates

f74d9d6

Move null checks for stored templates or scripts

cdecc35

Move them to the superclass and move them to Optional

Code reuse

83aacce

Javadocs

1d16334

nik9000 added 5 commits March 27, 2017 15:29

StringBuilder

2f1c935

Messages

b6f9cd5

Fix comment

8318393

Fix warning message

7ab675d

Fix broken tests

c9bb0b0

nik9000 closed this Mar 27, 2017

nik9000 mentioned this pull request Mar 28, 2017

Custom Call Signature Per Script Context #20426

Closed

clintongormley removed the v6.0.0-alpha1 label Mar 28, 2017

brusic mentioned this pull request Jun 12, 2017

Get all search templates/scripts #25179

Closed

Begin to break templates away from scripts #23744

Begin to break templates away from scripts #23744

Uh oh!

Conversation

nik9000 commented Mar 24, 2017

Uh oh!

nik9000 commented Mar 24, 2017

Uh oh!

nik9000 commented Mar 24, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nik9000 commented Mar 25, 2017

Uh oh!

dakrone left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nik9000 commented Mar 27, 2017

Uh oh!

rjernst commented Mar 27, 2017

Uh oh!

nik9000 commented Mar 27, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!