From ac575b68a970b8dec42cce501adcd496051586b0 Mon Sep 17 00:00:00 2001 From: Stuart Tettemer Date: Fri, 20 Mar 2020 15:14:30 -0600 Subject: [PATCH] Scripting: Context script cache unlimited compile (#53769) (#53899) * Adds "unlimited" compilation rate for context script caches * `script.context.${CONTEXT}.max_compilations_rate` = `unlimited` disables compilation rate limiting for `${CONTEXT}`'s script cache Refs: #50152 --- .../org/elasticsearch/script/ScriptCache.java | 11 ++++---- .../elasticsearch/script/ScriptService.java | 8 +++++- .../script/ScriptCacheTests.java | 13 ++++++++++ .../script/ScriptServiceTests.java | 25 +++++++++++++++++-- 4 files changed, 49 insertions(+), 8 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/script/ScriptCache.java b/server/src/main/java/org/elasticsearch/script/ScriptCache.java index a37851cc810..7ab5f6d1317 100644 --- a/server/src/main/java/org/elasticsearch/script/ScriptCache.java +++ b/server/src/main/java/org/elasticsearch/script/ScriptCache.java @@ -40,14 +40,16 @@ public class ScriptCache { private static final Logger logger = LogManager.getLogger(ScriptService.class); + static final Tuple UNLIMITED_COMPILATION_RATE = new Tuple<>(0, TimeValue.ZERO); + private final Cache cache; private final ScriptMetrics scriptMetrics; private final Object lock = new Object(); - // Mutable fields - private long lastInlineCompileTime; - private double scriptsPerTimeWindow; + // Mutable fields, visible for tests + long lastInlineCompileTime; + double scriptsPerTimeWindow; // Cache settings or derived from settings final int cacheSize; @@ -150,8 +152,7 @@ public class ScriptCache { * is discarded - there can never be more water in the bucket than the size of the bucket. */ void checkCompilationLimit() { - if (rate.v1() == 0 && rate.v2().getNanos() == 0) { - // unlimited + if (rate.equals(UNLIMITED_COMPILATION_RATE)) { return; } diff --git a/server/src/main/java/org/elasticsearch/script/ScriptService.java b/server/src/main/java/org/elasticsearch/script/ScriptService.java index ea9f572669f..9f24da46bc5 100644 --- a/server/src/main/java/org/elasticsearch/script/ScriptService.java +++ b/server/src/main/java/org/elasticsearch/script/ScriptService.java @@ -129,10 +129,16 @@ public class ScriptService implements Closeable, ClusterStateApplier { key -> Setting.positiveTimeSetting(key, SCRIPT_GENERAL_CACHE_EXPIRE_SETTING, TimeValue.timeValueMillis(0), Property.NodeScope, Property.Dynamic)); + // Unlimited compilation rate for context-specific script caches + static final String UNLIMITED_COMPILATION_RATE_KEY = "unlimited"; + public static final Setting.AffixSetting> SCRIPT_MAX_COMPILATIONS_RATE_SETTING = Setting.affixKeySetting(CONTEXT_PREFIX, "max_compilations_rate", - key -> new Setting<>(key, "75/5m", MAX_COMPILATION_RATE_FUNCTION, Property.NodeScope, Property.Dynamic)); + key -> new Setting<>(key, "75/5m", + (String value) -> value.equals(UNLIMITED_COMPILATION_RATE_KEY) ? ScriptCache.UNLIMITED_COMPILATION_RATE: + MAX_COMPILATION_RATE_FUNCTION.apply(value), + Property.NodeScope, Property.Dynamic)); private static final Tuple SCRIPT_COMPILATION_RATE_ZERO = new Tuple<>(0, TimeValue.ZERO); diff --git a/server/src/test/java/org/elasticsearch/script/ScriptCacheTests.java b/server/src/test/java/org/elasticsearch/script/ScriptCacheTests.java index dfa94609930..96c724b5720 100644 --- a/server/src/test/java/org/elasticsearch/script/ScriptCacheTests.java +++ b/server/src/test/java/org/elasticsearch/script/ScriptCacheTests.java @@ -52,4 +52,17 @@ public class ScriptCacheTests extends ESTestCase { cache.checkCompilationLimit(); } } + + public void testUnlimitedCompilationRate() { + final Integer size = ScriptService.SCRIPT_GENERAL_CACHE_SIZE_SETTING.get(Settings.EMPTY); + final TimeValue expire = ScriptService.SCRIPT_GENERAL_CACHE_EXPIRE_SETTING.get(Settings.EMPTY); + ScriptCache cache = new ScriptCache(size, expire, ScriptCache.UNLIMITED_COMPILATION_RATE); + long lastInlineCompileTime = cache.lastInlineCompileTime; + double scriptsPerTimeWindow = cache.scriptsPerTimeWindow; + for(int i=0; i < 3000; i++) { + cache.checkCompilationLimit(); + assertEquals(lastInlineCompileTime, cache.lastInlineCompileTime); + assertEquals(scriptsPerTimeWindow, cache.scriptsPerTimeWindow, 0.0); // delta of 0.0 because it should never change + } + } } diff --git a/server/src/test/java/org/elasticsearch/script/ScriptServiceTests.java b/server/src/test/java/org/elasticsearch/script/ScriptServiceTests.java index 7914c905770..4eaa4f406c6 100644 --- a/server/src/test/java/org/elasticsearch/script/ScriptServiceTests.java +++ b/server/src/test/java/org/elasticsearch/script/ScriptServiceTests.java @@ -451,6 +451,24 @@ public class ScriptServiceTests extends ESTestCase { assertEquals(zero, holder.contextCache.get("baz").get().rate); } + public void testCompilationRateUnlimitedContextOnly() throws IOException { + IllegalArgumentException illegal = expectThrows(IllegalArgumentException.class, () -> { + buildScriptService(Settings.builder() + .put(SCRIPT_GENERAL_MAX_COMPILATIONS_RATE_SETTING.getKey(), ScriptService.UNLIMITED_COMPILATION_RATE_KEY) + .build()); + }); + assertEquals("parameter must contain a positive integer and a timevalue, i.e. 10/1m, but was [unlimited]", illegal.getMessage()); + + // Should not throw. + buildScriptService(Settings.builder() + .put(SCRIPT_MAX_COMPILATIONS_RATE_SETTING.getConcreteSettingForNamespace("ingest").getKey(), + ScriptService.UNLIMITED_COMPILATION_RATE_KEY) + .put(SCRIPT_MAX_COMPILATIONS_RATE_SETTING.getConcreteSettingForNamespace("field").getKey(), + ScriptService.UNLIMITED_COMPILATION_RATE_KEY) + .put(SCRIPT_GENERAL_MAX_COMPILATIONS_RATE_SETTING.getKey(), ScriptService.USE_CONTEXT_RATE_KEY) + .build()); + } + public void testCacheHolderChangeSettings() { String fooCompilationRate = "77/5m"; String barCompilationRate = "78/6m"; @@ -460,7 +478,7 @@ public class ScriptServiceTests extends ESTestCase { Settings s = Settings.builder() .put(SCRIPT_GENERAL_MAX_COMPILATIONS_RATE_SETTING.getKey(), compilationRate) .build(); - Set contexts = new HashSet<>(Arrays.asList("foo", "bar", "baz")); + Set contexts = new HashSet<>(Arrays.asList("foo", "bar", "baz", "qux")); ScriptService.CacheHolder holder = new ScriptService.CacheHolder(s, contexts, true); assertNotNull(holder.general); @@ -471,16 +489,19 @@ public class ScriptServiceTests extends ESTestCase { .put(SCRIPT_GENERAL_MAX_COMPILATIONS_RATE_SETTING.getKey(), ScriptService.USE_CONTEXT_RATE_KEY) .put(SCRIPT_MAX_COMPILATIONS_RATE_SETTING.getConcreteSettingForNamespace("foo").getKey(), fooCompilationRate) .put(SCRIPT_MAX_COMPILATIONS_RATE_SETTING.getConcreteSettingForNamespace("bar").getKey(), barCompilationRate) + .put(SCRIPT_MAX_COMPILATIONS_RATE_SETTING.getConcreteSettingForNamespace("qux").getKey(), + ScriptService.UNLIMITED_COMPILATION_RATE_KEY) .build() ); assertNull(holder.general); assertNotNull(holder.contextCache); - assertEquals(3, holder.contextCache.size()); + assertEquals(4, holder.contextCache.size()); assertEquals(contexts, holder.contextCache.keySet()); assertEquals(ScriptService.MAX_COMPILATION_RATE_FUNCTION.apply(fooCompilationRate), holder.contextCache.get("foo").get().rate); assertEquals(ScriptService.MAX_COMPILATION_RATE_FUNCTION.apply(barCompilationRate), holder.contextCache.get("bar").get().rate); + assertEquals(ScriptCache.UNLIMITED_COMPILATION_RATE, holder.contextCache.get("qux").get().rate); assertEquals(ScriptService.SCRIPT_MAX_COMPILATIONS_RATE_SETTING.getDefault(Settings.EMPTY), holder.contextCache.get("baz").get().rate);