Fix updating templates. #10526

MaineC · 2015-04-10T08:19:25Z

When putting new templates to an index they are added to the cache
of compiled templates as a side effect of the validate method. When
updating templates they are also validated but the scripts that are
already in the cache never get updated.

@javanna Assigning to you for review as you seem to have written most of the code this PR touches, feel free to re-assign.

javanna · 2015-04-10T08:51:15Z

src/main/java/org/elasticsearch/script/ScriptService.java

-        CompiledScript compiled = cache.getIfPresent(cacheKey);
+        CompiledScript compiled = null;
+        if (! ignoreCache) {
+            cache.getIfPresent(cacheKey);


you are entirely disabling the cache if you don't assign the result here to compiled.

That was supposed to read

compiled = cache.getIf...

Stupid typo on my side that lead to fixing the test as a side effect...

the problem is that when you fix it the test fails ;)

javanna · 2015-04-10T08:58:09Z

Hi @MaineC I don't think the problem stems from the validate method. validate properly caches using the script itself. The problem is that when we actually retrieve the script from the index as part of the search request, we actually look for and cache the script based on its id. That means it's never going to find the already cached version. Also, once a script is cached based on its id it is never going to be updated in the cache.

I would change the caching mechanism to use the retrieved script as cache key in case the script is indexed, so it will match with the one cached as part of validate and it will get cached again once it changes. makes sense?

javanna · 2015-04-10T09:08:28Z

I did some research, this is a regression introduced with #10033 . 1.5.0 and 1.5.1 are affected, meaning that once you executed an indexed script for the first time, that first version is going to stick around in the internal cache although you update the script. We better add some proper test coverage for this :)

MaineC · 2015-04-13T07:42:14Z

@javanna wrt. to your proposal - makes sense, changed the PR accordingly.

I also re-named the "script" parameter in compileInternal to "scriptOrId" to better reflect it's real content.

One thing I was wondering: For indexed scripts we could consistently use their id as part of the cache key. This would save retrieving the string version from the index later (assuming a cache hit). On the flip side it would make it understanding cache keys trickier. Not sure if this is worth the effort?

javanna · 2015-04-16T09:40:39Z

src/test/java/org/elasticsearch/index/query/TemplateQueryTest.java

+        assertEquals(scriptResponse.getVersion(), 2);
+        SearchResponse searchResponse = client().prepareSearch("testindex").setTypes("test").
+                setTemplateName("git01").setTemplateType(ScriptService.ScriptType.INDEXED).setTemplateParams(templateParams).get();
+        assertHitCount(searchResponse, 1);


I would love to have some more coverage here, maybe update a few more times (randomIntBetween(1,10) ?) and verify that updates always make it through, not just once?

Also, can we write a test for plain indexed scripts too (rather than search templates)?

javanna · 2015-04-16T09:42:12Z

looks good, left a couple of comments around testing.

MaineC · 2015-04-16T09:54:48Z

@javanna Thanks for the review and comments - good suggestions, will update accordingly.

As per comments on PR elastic#10526 adding more tests around updating scripts and templates.

javanna · 2015-04-21T09:54:03Z

src/test/java/org/elasticsearch/index/query/TemplateQueryTest.java

+        index("testindex", "test", "1", jsonBuilder().startObject().field("searchtext", "dev1").endObject());
+        refresh();
+
+        for (int i = 1; i < randomIntBetween(2, 11); i++) {


I don't think it is a good idea to call randomInBetween from here? We should save the result to a variable outside of the loop.

m(

Good catch.

javanna · 2015-04-21T09:56:04Z

I left a small comments around tests, LGTM besides that

Closes elastic#10397 When putting new templates to an index they are added to the cache of compiled templates as a side effect of the validate method. When updating templates they are also validated but the scripts that are already in the cache never get updated. As per comments on PR elastic#10526 adding more tests around updating scripts and templates.

…not-recompiled Closes #10397: Fix updating indexed search templates.

See #10526 for more details.

See elastic#10526 for more details.

MaineC assigned javanna Apr 10, 2015

javanna reviewed Apr 10, 2015
View reviewed changes

javanna added :Core/Infra/Scripting Scripting abstractions, Painless, and Mustache >bug v1.6.0 v2.0.0-beta1 review labels Apr 10, 2015

javanna mentioned this pull request Apr 10, 2015

indexed script not properly updated when used for sort #10517

Closed

javanna reviewed Apr 16, 2015
View reviewed changes

javanna mentioned this pull request Apr 20, 2015

Added a new script construct #10649

Merged

MaineC pushed a commit to MaineC/elasticsearch that referenced this pull request Apr 21, 2015

Add more tests around updating scripts.

3842605

As per comments on PR elastic#10526 adding more tests around updating scripts and templates.

MaineC force-pushed the bug-fix/10397-update-templates-not-recompiled branch from 3842605 to 8125941 Compare April 21, 2015 08:51

MaineC pushed a commit to MaineC/elasticsearch that referenced this pull request Apr 21, 2015

Add more tests around updating scripts.

8125941

As per comments on PR elastic#10526 adding more tests around updating scripts and templates.

javanna reviewed Apr 21, 2015
View reviewed changes

MaineC force-pushed the bug-fix/10397-update-templates-not-recompiled branch from 7656986 to acf8961 Compare April 22, 2015 08:59

MaineC force-pushed the bug-fix/10397-update-templates-not-recompiled branch from acf8961 to a9d540a Compare April 22, 2015 09:00

MaineC pushed a commit that referenced this pull request Apr 22, 2015

Merge pull request #10526 from MaineC/bug-fix/10397-update-templates-…

f857f9e

…not-recompiled Closes #10397: Fix updating indexed search templates.

MaineC merged commit f857f9e into elastic:master Apr 22, 2015

kevinkluge removed the review label Apr 22, 2015

MaineC pushed a commit that referenced this pull request Apr 24, 2015

Backport of fix for #10397

b9c15a6

See #10526 for more details.

MaineC pushed a commit that referenced this pull request Apr 24, 2015

Backport of fix for #10397

68571e3

See #10526 for more details.

MaineC added the v1.5.2 label Apr 24, 2015

clintongormley added :Indexed Scripts/Templates and removed :Core/Infra/Scripting Scripting abstractions, Painless, and Mustache labels Jun 8, 2015

mute pushed a commit to mute/elasticsearch that referenced this pull request Jul 29, 2015

Backport of fix for elastic#10397

62046f4

See elastic#10526 for more details.

clintongormley mentioned this pull request Sep 27, 2015

can NOT do template query on updated search template #13803

Closed

clintongormley added :Core/Infra/Scripting Scripting abstractions, Painless, and Mustache and removed :Indexed Scripts/Templates labels Feb 13, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix updating templates. #10526

Fix updating templates. #10526

MaineC commented Apr 10, 2015

javanna Apr 10, 2015

MaineC Apr 10, 2015

javanna Apr 10, 2015

javanna commented Apr 10, 2015

javanna commented Apr 10, 2015

MaineC commented Apr 13, 2015

javanna Apr 16, 2015

javanna commented Apr 16, 2015

MaineC commented Apr 16, 2015

javanna Apr 21, 2015

MaineC Apr 21, 2015

javanna commented Apr 21, 2015

Fix updating templates. #10526

Fix updating templates. #10526

Conversation

MaineC commented Apr 10, 2015

javanna Apr 10, 2015

Choose a reason for hiding this comment

MaineC Apr 10, 2015

Choose a reason for hiding this comment

javanna Apr 10, 2015

Choose a reason for hiding this comment

javanna commented Apr 10, 2015

javanna commented Apr 10, 2015

MaineC commented Apr 13, 2015

javanna Apr 16, 2015

Choose a reason for hiding this comment

javanna commented Apr 16, 2015

MaineC commented Apr 16, 2015

javanna Apr 21, 2015

Choose a reason for hiding this comment

MaineC Apr 21, 2015

Choose a reason for hiding this comment

javanna commented Apr 21, 2015