From 7c048973928e7074daa34172f9eb93701cb2eba9 Mon Sep 17 00:00:00 2001 From: Alexander Reelsen Date: Fri, 2 Dec 2016 10:36:05 +0100 Subject: [PATCH] Watcher: Compile scripts on each invocation (elastic/elasticsearch#4239) Transform and condition scripts were only compiled on its initial creation, so when a new watch is created or when the master node loads all the watches. However changing a script (like a stored one) did not lead to any changes in the in memory watch store and thus the old script was executed again. We do however have a mechanism in Elasticsearch's ScriptService that already does some caching, and should reuse that one. Closes elastic/elasticsearch#4237 Original commit: elastic/x-pack-elasticsearch@477548e237eff1bcc3b677985a489dc5b5a5e2da --- .../watcher/condition/ScriptCondition.java | 6 +- .../script/ExecutableScriptTransform.java | 6 +- .../script/ScriptTransformFactory.java | 3 - .../script/ScriptTransformTests.java | 7 +- .../watcher_painless/50_update_scripts.yaml | 136 ++++++++++++++++++ 5 files changed, 143 insertions(+), 15 deletions(-) create mode 100644 qa/smoke-test-watcher-with-painless/src/test/resources/rest-api-spec/test/watcher_painless/50_update_scripts.yaml diff --git a/elasticsearch/src/main/java/org/elasticsearch/xpack/watcher/condition/ScriptCondition.java b/elasticsearch/src/main/java/org/elasticsearch/xpack/watcher/condition/ScriptCondition.java index cf9036d9706..2ebe2d34279 100644 --- a/elasticsearch/src/main/java/org/elasticsearch/xpack/watcher/condition/ScriptCondition.java +++ b/elasticsearch/src/main/java/org/elasticsearch/xpack/watcher/condition/ScriptCondition.java @@ -32,21 +32,20 @@ public final class ScriptCondition extends Condition { private static final Result UNMET = new Result(null, TYPE, false); private final ScriptService scriptService; - private final CompiledScript compiledScript; private final Script script; public ScriptCondition(Script script) { super(TYPE); this.script = script; scriptService = null; - compiledScript = null; } ScriptCondition(Script script, ScriptService scriptService) { super(TYPE); this.scriptService = scriptService; this.script = script; - compiledScript = scriptService.compile(script, Watcher.SCRIPT_CONTEXT, Collections.emptyMap()); + // try to compile so we catch syntax errors early + scriptService.compile(script, Watcher.SCRIPT_CONTEXT, Collections.emptyMap()); } public Script getScript() { @@ -73,6 +72,7 @@ public final class ScriptCondition extends Condition { if (script.getParams() != null && !script.getParams().isEmpty()) { parameters.putAll(script.getParams()); } + CompiledScript compiledScript = scriptService.compile(script, Watcher.SCRIPT_CONTEXT, Collections.emptyMap()); ExecutableScript executable = scriptService.executable(compiledScript, parameters); Object value = executable.run(); if (value instanceof Boolean) { diff --git a/elasticsearch/src/main/java/org/elasticsearch/xpack/watcher/transform/script/ExecutableScriptTransform.java b/elasticsearch/src/main/java/org/elasticsearch/xpack/watcher/transform/script/ExecutableScriptTransform.java index a88047fd9d1..a6ed5d83f71 100644 --- a/elasticsearch/src/main/java/org/elasticsearch/xpack/watcher/transform/script/ExecutableScriptTransform.java +++ b/elasticsearch/src/main/java/org/elasticsearch/xpack/watcher/transform/script/ExecutableScriptTransform.java @@ -28,13 +28,13 @@ import static org.elasticsearch.xpack.watcher.transform.script.ScriptTransform.T public class ExecutableScriptTransform extends ExecutableTransform { private final ScriptService scriptService; - private final CompiledScript compiledScript; public ExecutableScriptTransform(ScriptTransform transform, Logger logger, ScriptService scriptService) { super(transform, logger); this.scriptService = scriptService; Script script = transform.getScript(); - compiledScript = scriptService.compile(script, Watcher.SCRIPT_CONTEXT, Collections.emptyMap()); + // try to compile so we catch syntax errors early + scriptService.compile(script, Watcher.SCRIPT_CONTEXT, Collections.emptyMap()); } @Override @@ -47,7 +47,6 @@ public class ExecutableScriptTransform extends ExecutableTransform model = new HashMap<>(); @@ -55,6 +54,7 @@ public class ExecutableScriptTransform extends ExecutableTransform { - private final Settings settings; private final ScriptService scriptService; public ScriptTransformFactory(Settings settings, ScriptService scriptService) { super(Loggers.getLogger(ExecutableScriptTransform.class, settings)); - this.settings = settings; this.scriptService = scriptService; } diff --git a/elasticsearch/src/test/java/org/elasticsearch/xpack/watcher/transform/script/ScriptTransformTests.java b/elasticsearch/src/test/java/org/elasticsearch/xpack/watcher/transform/script/ScriptTransformTests.java index 2aef33c4c79..26f4214e20e 100644 --- a/elasticsearch/src/test/java/org/elasticsearch/xpack/watcher/transform/script/ScriptTransformTests.java +++ b/elasticsearch/src/test/java/org/elasticsearch/xpack/watcher/transform/script/ScriptTransformTests.java @@ -25,7 +25,6 @@ import org.elasticsearch.xpack.watcher.support.Variables; import org.elasticsearch.xpack.watcher.transform.Transform; import org.elasticsearch.xpack.watcher.watch.Payload; import org.junit.After; -import org.junit.Before; import java.util.Arrays; import java.util.Collections; @@ -50,12 +49,8 @@ import static org.mockito.Mockito.mock; import static org.mockito.Mockito.when; public class ScriptTransformTests extends ESTestCase { - ThreadPool tp = null; - @Before - public void init() { - tp = new TestThreadPool(ThreadPool.Names.SAME); - } + private final ThreadPool tp = new TestThreadPool(ThreadPool.Names.SAME); @After public void cleanup() { diff --git a/qa/smoke-test-watcher-with-painless/src/test/resources/rest-api-spec/test/watcher_painless/50_update_scripts.yaml b/qa/smoke-test-watcher-with-painless/src/test/resources/rest-api-spec/test/watcher_painless/50_update_scripts.yaml new file mode 100644 index 00000000000..6460af36cb6 --- /dev/null +++ b/qa/smoke-test-watcher-with-painless/src/test/resources/rest-api-spec/test/watcher_painless/50_update_scripts.yaml @@ -0,0 +1,136 @@ +# When a script is specified in a watch, updates should be taken into account +# See https://github.com/elastic/x-plugins/issues/4237 +--- +"Test transform scripts are updated on execution": + - do: + cluster.health: + wait_for_status: yellow + + - do: + put_script: + id: transform-script + lang: painless + body: > + { + "script":"return [ 'email': 'foo@bar.org' ]" + } + + - do: + xpack.watcher.put_watch: + id: "my_watch" + body: > + { + "trigger": { + "schedule": { + "interval": "1h" + } + }, + "input": { + "simple": {} + }, + "transform": { + "script": { + "stored": "transform-script" + } + }, + "actions": { + "my_log": { + "logging": { + "text": "{{ctx}}" + } + } + } + } + + - match: { _id: "my_watch" } + + - do: + xpack.watcher.execute_watch: + id: "my_watch" + + - match: { "watch_record.watch_id": "my_watch" } + - match: { "watch_record.result.transform.payload.email": "foo@bar.org" } + + - do: + put_script: + id: transform-script + lang: painless + body: > + { + "script":"return [ 'email': 'foo@example.org' ]" + } + + - do: + xpack.watcher.execute_watch: + id: "my_watch" + + - match: { "watch_record.watch_id": "my_watch" } + - match: { "watch_record.result.transform.payload.email": "foo@example.org" } + +--- +"Test condition scripts are updated on execution": + - do: + cluster.health: + wait_for_status: yellow + + - do: + put_script: + id: condition-script + lang: painless + body: > + { + "script":"return false" + } + + - do: + xpack.watcher.put_watch: + id: "my_watch" + body: > + { + "trigger": { + "schedule": { + "interval": "1h" + } + }, + "input": { + "simple": {} + }, + "condition": { + "script": { + "stored": "condition-script" + } + }, + "actions": { + "my_log": { + "logging": { + "text": "{{ctx}}" + } + } + } + } + + - match: { _id: "my_watch" } + + - do: + xpack.watcher.execute_watch: + id: "my_watch" + + - match: { "watch_record.watch_id": "my_watch" } + - match: { "watch_record.result.condition.met": false } + + - do: + put_script: + id: condition-script + lang: painless + body: > + { + "script":"return true" + } + + - do: + xpack.watcher.execute_watch: + id: "my_watch" + + - match: { "watch_record.watch_id": "my_watch" } + - match: { "watch_record.result.condition.met": true } +