Scripting: Replace advanced and native scripts with ScriptEngine docs #24603

rjernst · 2017-05-10T23:02:55Z

This commit documents how to write a ScriptEngine in order to use
expert internal apis, such as using Lucene directly to find index term
statistics. These documents prepare the way to remove both native
scripts and IndexLookup.

The example java code is actually compiled and tested under a new gradle
subproject for example plugins. This change does not yet breakup
jvm-example into the new examples dir, which should be done separately.

relates #19359
relates #19966

This commit documents how to write a `ScriptEngine` in order to use expert internal apis, such as using Lucene directly to find index term statistics. These documents prepare the way to remove both native scripts and IndexLookup. The example java code is actually compiled and tested under a new gradle subproject for example plugins. This change does not yet breakup jvm-example into the new examples dir, which should be done separately. relates elastic#19359 relates elastic#19966

nik9000 · 2017-05-11T01:50:33Z

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

@@ -38,7 +38,9 @@
    /**
     * The extension for file scripts in this language.
     */
-    String getExtension();
+    default String getExtension() {
+        return getType();


nik9000 · 2017-05-11T01:57:48Z

settings.gradle

@@ -76,6 +76,15 @@ List projects = [
  'qa:wildfly'
 ]

+File examplePluginsDir = new File(rootProject.projectDir, 'plugins/examples')


Why not just declare them like all the other plugins? This doesn't seem like it is better than having the plugin in :plugins:examples:advanced-scoring.

And it is different from every other project because the path doesn't match the name, which I'd prefer not to do just to keep things consistent.

I did this for two reasons:

Otherwise all the places that list plugins (eg smoke-test-plugins) via the gradle project :plugins must add a bunch of logic we must copy every time we want to get all the plugins.

I have come around to hating having to list every plugin/module, as it is both cumbersome to change, as well as error prone because moving/creating a new module results in a cryptic error message from gradle until it is added (thinking that the project is standalone instead of our multi project build).

Those are both fine points. Is there any way to accomplish them without using the funny project name?

For the first point, no. It works without having to affect anything iterating the plugins precisely because it is not a subproject of :plugins.

Honestly, I don't see it as that big of a deal. This is what settings.gradle is for, to control how the multi project build is laid out. There is nothing that says project's directories must match their directory layout. The only reason I didn't add a top level example-plugins directory is I think the root directory already has far too many things in it.

Maybe just make a directory called example-plugins instead of keeping it in the plugins directory? I like that the names all line up. Exception for buildSrc, but buildSrc is known special and weird.

As I said, i think the root project is already too crowded. I don't want to add more directories there, I want to remove them.

Sorry, I spaced out and didn't read the second half of your paragraph. I should probably sleep soon!

I really want the names to line up if I can have it. If i can't fine.

What about putting it as a subdirectory of docs?

I think having it as a subdirectory of docs will be confusing, because this is compilable and testable code, not ascii docs.

I really want the names to line up if I can have it.

The names already don't line up, see for example the core hack we have around the same spot in settings.gradle. I really dont' see why the directory name matching the project name matters. These aren't things we ever have to deal with, other than in gradle, and gradle doesn't care what the names are, only that there is a dir associated with each project. cding into the plugins/examples dir still allows running tests as normal.

nik9000 · 2017-05-11T01:59:52Z

...-expert-scoring/src/main/java/org/elasticsearch/example/expertscript/ExpertScriptPlugin.java

+        @Override
+        public void close() {}
+    }
+    // end::expert_engine


Maybe you should include LeafScript in the tag?

I think this is a good enough tweak I will try to move these into LeafSearchScript.

I pushed adf95e6.

nik9000 · 2017-05-11T02:01:04Z

docs/reference/modules/scripting/engine.asciidoc

+}
+--------------------------------------------------
+// CONSOLE
+// TEST[skip:we don't have an expert script plugin installed to test this]


I mean, we totally could though. We already install all the plugins, I think we'd just need to fix the loop we use for it and this'd be installed too.

We could, but I'd rather leave that as a separate issue.

We install all the other plugins for these tests so I think this would be the PR to add it.

imotov

LGTM. Left a couple of minor comments.

imotov · 2017-05-11T12:39:34Z

...-expert-scoring/src/main/java/org/elasticsearch/example/expertscript/ExpertScriptPlugin.java

+            if ("pure_df".equals(scriptSource)) {
+                return p -> new SearchScript() {
+                    String field = p.get("field").toString();
+                    String term = p.get("term").toString();


Might be nice to add a check for existence of these parameters for completeness.

I added checks in 1070eb3.

imotov · 2017-05-11T13:05:09Z

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

+     * Return the result as a long. This is used by aggregation scripts over long fields.
+     */
+    default long runAsLong() {
+        return (long) runAsDouble();


I am somewhat conflicted about this. On one side I like this simplicity, on the other side if these default implementation are ever invoked, it most likely means that the script is used in a wrong context or has not been optimized for some contexts. Maybe, we shouldn't provide a default implementation, but instead clearly document in which contexts runAsLong, runAsDouble and run are called from and follow the same model as we do with search vs executable script and throw UnsupportedOperationException if a wrong method is called. I think this will encourage the users to read the comments on the method and implement the right method for their use case.

I agree. I changed runAsLong to default to UnsupportedOperationException and tweaked the docs a tiny bit. Also note that with contexts coming this will all change significantly, as each context will have its own dedicated interface, so no more of this shared interface trying to satisfy multiple use cases.

…#24603) This commit documents how to write a `ScriptEngine` in order to use expert internal apis, such as using Lucene directly to find index term statistics. These documents prepare the way to remove both native scripts and IndexLookup. The example java code is actually compiled and tested under a new gradle subproject for example plugins. This change does not yet breakup jvm-example into the new examples dir, which should be done separately. relates #19359 relates #19966

rjernst added :Core/Infra/Scripting Scripting abstractions, Painless, and Mustache >docs General docs changes v5.5.0 v6.0.0 labels May 10, 2017

rjernst requested review from nik9000 and imotov May 10, 2017 23:03

Merge branch 'master' into replace_native_script_docs

b88a434

nik9000 reviewed May 11, 2017

View reviewed changes

nik9000 approved these changes May 11, 2017

View reviewed changes

simplify LeafSearchScript

adf95e6

imotov approved these changes May 11, 2017

View reviewed changes

rjernst added 2 commits May 11, 2017 10:04

Merge branch 'master' into replace_native_script_docs

a8c326d

feedback

1070eb3

rjernst merged commit c1f1f66 into elastic:master May 11, 2017

rjernst deleted the replace_native_script_docs branch May 11, 2017 19:15

clintongormley added v6.0.0-beta1 and removed v6.0.0 labels Jul 25, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Scripting: Replace advanced and native scripts with ScriptEngine docs #24603

Scripting: Replace advanced and native scripts with ScriptEngine docs #24603

rjernst commented May 10, 2017

nik9000 May 11, 2017

nik9000 May 11, 2017

nik9000 May 11, 2017

rjernst May 11, 2017

nik9000 May 11, 2017

rjernst May 11, 2017

nik9000 May 11, 2017

rjernst May 11, 2017

nik9000 May 11, 2017

rjernst May 11, 2017

rjernst May 11, 2017

nik9000 May 11, 2017

rjernst May 11, 2017

nik9000 May 11, 2017

rjernst May 11, 2017

nik9000 May 11, 2017

rjernst May 11, 2017

nik9000 May 11, 2017

imotov left a comment

imotov May 11, 2017

rjernst May 11, 2017

imotov May 11, 2017

rjernst May 11, 2017

Scripting: Replace advanced and native scripts with ScriptEngine docs #24603

Scripting: Replace advanced and native scripts with ScriptEngine docs #24603

Conversation

rjernst commented May 10, 2017

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

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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

imotov left a comment

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

Choose a reason for hiding this comment