From 6a2bbfdfce615984d0390d93c44bfe603918e666 Mon Sep 17 00:00:00 2001 From: Stuart Tettemer Date: Wed, 18 Mar 2020 14:42:50 -0600 Subject: [PATCH] Scripting: Increase ingest script cache defaults * Adds ability for contexts to specify their own defaults. * Context defaults are applied if no context-specific or general setting exists. * See 070ea7e for settings keys. * Increases the per-context default for the `ingest` context. * Cache size is doubled, 200 compared to default of 100 * Cache expiration is unchanged at no expiration * Cache max compilation is quintupled, 375/5m instead of 75/5m Refs: #50152 --- .../elasticsearch/script/IngestScript.java | 6 +- .../elasticsearch/script/ScriptContext.java | 28 ++++- .../elasticsearch/script/ScriptService.java | 54 +++++--- .../script/ScriptServiceTests.java | 118 ++++++++++++++++-- 4 files changed, 173 insertions(+), 33 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/script/IngestScript.java b/server/src/main/java/org/elasticsearch/script/IngestScript.java index f357394ed31f0..ffd3475fc52c3 100644 --- a/server/src/main/java/org/elasticsearch/script/IngestScript.java +++ b/server/src/main/java/org/elasticsearch/script/IngestScript.java @@ -20,6 +20,9 @@ package org.elasticsearch.script; +import org.elasticsearch.common.collect.Tuple; +import org.elasticsearch.common.unit.TimeValue; + import java.util.Map; /** @@ -30,7 +33,8 @@ public abstract class IngestScript { public static final String[] PARAMETERS = { "ctx" }; /** The context used to compile {@link IngestScript} factories. */ - public static final ScriptContext CONTEXT = new ScriptContext<>("ingest", Factory.class); + public static final ScriptContext CONTEXT = new ScriptContext<>("ingest", Factory.class, + 200, TimeValue.timeValueMillis(0), new Tuple<>(375, TimeValue.timeValueMinutes(5))); /** The generic runtime parameters for the script. */ private final Map params; diff --git a/server/src/main/java/org/elasticsearch/script/ScriptContext.java b/server/src/main/java/org/elasticsearch/script/ScriptContext.java index 081a26d1e511a..abdcc99a1a3b1 100644 --- a/server/src/main/java/org/elasticsearch/script/ScriptContext.java +++ b/server/src/main/java/org/elasticsearch/script/ScriptContext.java @@ -19,6 +19,9 @@ package org.elasticsearch.script; +import org.elasticsearch.common.collect.Tuple; +import org.elasticsearch.common.unit.TimeValue; + import java.lang.reflect.Method; /** @@ -68,8 +71,18 @@ public final class ScriptContext { /** A class that is an instance of a script. */ public final Class instanceClazz; - /** Construct a context with the related instance and compiled classes. */ - public ScriptContext(String name, Class factoryClazz) { + /** The default size of the cache for the context if not overridden */ + public final int cacheSizeDefault; + + /** The default expiration of a script in the cache for the context, if not overridden */ + public final TimeValue cacheExpireDefault; + + /** The default max compilation rate for scripts in this context. Script compilation is throttled if this is exceeded */ + public final Tuple maxCompilationRateDefault; + + /** Construct a context with the related instance and compiled classes with caller provided cache defaults */ + public ScriptContext(String name, Class factoryClazz, int cacheSizeDefault, TimeValue cacheExpireDefault, + Tuple maxCompilationRateDefault) { this.name = name; this.factoryClazz = factoryClazz; Method newInstanceMethod = findMethod("FactoryType", factoryClazz, "newInstance"); @@ -90,6 +103,17 @@ public ScriptContext(String name, Class factoryClazz) { + factoryClazz.getName() + "] for script context [" + name + "]"); } instanceClazz = newInstanceMethod.getReturnType(); + + this.cacheSizeDefault = cacheSizeDefault; + this.cacheExpireDefault = cacheExpireDefault; + this.maxCompilationRateDefault = maxCompilationRateDefault; + } + + /** Construct a context with the related instance and compiled classes with defaults for cacheSizeDefault, cacheExpireDefault and + * maxCompilationRateDefault */ + public ScriptContext(String name, Class factoryClazz) { + // cache size default, cache expire default, max compilation rate are defaults from ScriptService. + this(name, factoryClazz, 100, TimeValue.timeValueMillis(0), new Tuple<>(75, TimeValue.timeValueMinutes(5))); } /** Returns a method with the given name, or throws an exception if multiple are found. */ diff --git a/server/src/main/java/org/elasticsearch/script/ScriptService.java b/server/src/main/java/org/elasticsearch/script/ScriptService.java index 66ed3fee0d6ca..4c01e9aa15b9b 100644 --- a/server/src/main/java/org/elasticsearch/script/ScriptService.java +++ b/server/src/main/java/org/elasticsearch/script/ScriptService.java @@ -46,6 +46,7 @@ import java.io.IOException; import java.nio.charset.StandardCharsets; import java.util.ArrayList; +import java.util.Collection; import java.util.Collections; import java.util.Comparator; import java.util.HashMap; @@ -235,7 +236,7 @@ public ScriptService(Settings settings, Map engines, Map(new CacheHolder(settings, contexts.keySet(), compilationLimitsEnabled())); + cacheHolder = new AtomicReference<>(new CacheHolder(settings, contexts.values(), compilationLimitsEnabled())); } /** @@ -249,12 +250,12 @@ void registerClusterSettingsListeners(ClusterSettings clusterSettings) { clusterSettings.addSettingsUpdateConsumer(SCRIPT_MAX_SIZE_IN_BYTES, this::setMaxSizeInBytes); // Handle all updatable per-context settings at once for each context. - for (String context: contexts.keySet()) { + for (ScriptContext context: contexts.values()) { clusterSettings.addSettingsUpdateConsumer( (settings) -> cacheHolder.get().updateContextSettings(settings, context), - List.of(SCRIPT_CACHE_SIZE_SETTING.getConcreteSettingForNamespace(context), - SCRIPT_CACHE_EXPIRE_SETTING.getConcreteSettingForNamespace(context), - SCRIPT_MAX_COMPILATIONS_RATE_SETTING.getConcreteSettingForNamespace(context), + List.of(SCRIPT_CACHE_SIZE_SETTING.getConcreteSettingForNamespace(context.name), + SCRIPT_CACHE_EXPIRE_SETTING.getConcreteSettingForNamespace(context.name), + SCRIPT_MAX_COMPILATIONS_RATE_SETTING.getConcreteSettingForNamespace(context.name), SCRIPT_GENERAL_CACHE_EXPIRE_SETTING, // general settings used for fallbacks SCRIPT_GENERAL_CACHE_SIZE_SETTING ) @@ -574,17 +575,18 @@ static class CacheHolder { final ScriptCache general; final Map> contextCache; - final Set contexts; + final Set> contexts; final boolean compilationLimitsEnabled; - CacheHolder(Settings settings, Set contexts, boolean compilationLimitsEnabled) { + CacheHolder(Settings settings, Collection> contexts, boolean compilationLimitsEnabled) { this.compilationLimitsEnabled = compilationLimitsEnabled; this.contexts = Set.copyOf(contexts); if (SCRIPT_GENERAL_MAX_COMPILATIONS_RATE_SETTING.get(settings).equals(USE_CONTEXT_RATE_VALUE)) { this.general = null; Map> contextCache = new HashMap<>(this.contexts.size()); - for (String context : this.contexts) { - contextCache.put(context, new AtomicReference<>(contextFromSettings(settings, context, this.compilationLimitsEnabled))); + for (ScriptContext context : this.contexts) { + contextCache.put(context.name, + new AtomicReference<>(contextFromSettings(settings, context, this.compilationLimitsEnabled))); } this.contextCache = Collections.unmodifiableMap(contextCache); } else { @@ -601,12 +603,24 @@ static class CacheHolder { /** * Create a ScriptCache for the given context. */ - private static ScriptCache contextFromSettings(Settings settings, String context, boolean compilationLimitsEnabled) { - return new ScriptCache(SCRIPT_CACHE_SIZE_SETTING.getConcreteSettingForNamespace(context).get(settings), - SCRIPT_CACHE_EXPIRE_SETTING.getConcreteSettingForNamespace(context).get(settings), - compilationLimitsEnabled ? - SCRIPT_MAX_COMPILATIONS_RATE_SETTING.getConcreteSettingForNamespace(context).get(settings) : - SCRIPT_COMPILATION_RATE_ZERO); + private static ScriptCache contextFromSettings(Settings settings, ScriptContext context, boolean compilationLimitsEnabled) { + String name = context.name; + Tuple compileRate; + Setting> rateSetting = SCRIPT_MAX_COMPILATIONS_RATE_SETTING.getConcreteSettingForNamespace(name); + if (compilationLimitsEnabled == false) { + compileRate = SCRIPT_COMPILATION_RATE_ZERO; + } else if (rateSetting.existsOrFallbackExists(settings)) { + compileRate = rateSetting.get(settings); + } else { + compileRate = context.maxCompilationRateDefault; + } + + Setting cacheExpire = SCRIPT_CACHE_EXPIRE_SETTING.getConcreteSettingForNamespace(name); + Setting cacheSize = SCRIPT_CACHE_SIZE_SETTING.getConcreteSettingForNamespace(name); + + return new ScriptCache(cacheSize.existsOrFallbackExists(settings) ? cacheSize.get(settings) : context.cacheSizeDefault, + cacheExpire.existsOrFallbackExists(settings) ? cacheExpire.get(settings) : context.cacheExpireDefault, + compileRate); } /** @@ -660,16 +674,16 @@ ScriptStats stats() { /** * Update settings for the context cache, if we're in the context cache mode otherwise no-op. */ - void updateContextSettings(Settings settings, String context) { + void updateContextSettings(Settings settings, ScriptContext context) { if (general != null) { return; } - AtomicReference ref = contextCache.get(context); - assert ref != null : "expected script cache to exist for context [" + context + "]"; + AtomicReference ref = contextCache.get(context.name); + assert ref != null : "expected script cache to exist for context [" + context.name + "]"; ScriptCache cache = ref.get(); - assert cache != null : "expected script cache to be non-null for context [" + context + "]"; + assert cache != null : "expected script cache to be non-null for context [" + context.name + "]"; ref.set(contextFromSettings(settings, context, compilationLimitsEnabled)); - logger.debug("Replaced context [" + context + "] with new settings"); + logger.debug("Replaced context [" + context.name + "] with new settings"); } } } diff --git a/server/src/test/java/org/elasticsearch/script/ScriptServiceTests.java b/server/src/test/java/org/elasticsearch/script/ScriptServiceTests.java index 77ce3fe127c7f..326d7b4ba2c0c 100644 --- a/server/src/test/java/org/elasticsearch/script/ScriptServiceTests.java +++ b/server/src/test/java/org/elasticsearch/script/ScriptServiceTests.java @@ -36,16 +36,24 @@ import org.junit.Before; import java.io.IOException; +import java.util.Collection; import java.util.Collections; import java.util.HashMap; import java.util.HashSet; +import java.util.List; import java.util.Map; import java.util.Set; import java.util.function.Function; +import java.util.stream.Collectors; import static org.elasticsearch.script.ScriptService.MAX_COMPILATION_RATE_FUNCTION; +import static org.elasticsearch.script.ScriptService.SCRIPT_CACHE_EXPIRE_SETTING; +import static org.elasticsearch.script.ScriptService.SCRIPT_CACHE_SIZE_SETTING; +import static org.elasticsearch.script.ScriptService.SCRIPT_GENERAL_CACHE_EXPIRE_SETTING; +import static org.elasticsearch.script.ScriptService.SCRIPT_GENERAL_CACHE_SIZE_SETTING; import static org.elasticsearch.script.ScriptService.SCRIPT_GENERAL_MAX_COMPILATIONS_RATE_SETTING; import static org.elasticsearch.script.ScriptService.SCRIPT_MAX_COMPILATIONS_RATE_SETTING; +import static org.elasticsearch.script.ScriptService.USE_CONTEXT_RATE_KEY; import static org.hamcrest.CoreMatchers.containsString; import static org.hamcrest.Matchers.is; import static org.hamcrest.Matchers.notNullValue; @@ -226,7 +234,7 @@ public void testMultipleCompilationsCountedInCompilationStats() throws IOExcepti public void testCompilationStatsOnCacheHit() throws IOException { Settings.Builder builder = Settings.builder(); - builder.put(ScriptService.SCRIPT_GENERAL_CACHE_SIZE_SETTING.getKey(), 1); + builder.put(SCRIPT_GENERAL_CACHE_SIZE_SETTING.getKey(), 1); buildScriptService(builder.build()); Script script = new Script(ScriptType.INLINE, "test", "1+1", Collections.emptyMap()); ScriptContext context = randomFrom(contexts.values()); @@ -243,7 +251,7 @@ public void testIndexedScriptCountedInCompilationStats() throws IOException { public void testCacheEvictionCountedInCacheEvictionsStats() throws IOException { Settings.Builder builder = Settings.builder(); - builder.put(ScriptService.SCRIPT_GENERAL_CACHE_SIZE_SETTING.getKey(), 1); + builder.put(SCRIPT_GENERAL_CACHE_SIZE_SETTING.getKey(), 1); buildScriptService(builder.build()); scriptService.compile(new Script(ScriptType.INLINE, "test", "1+1", Collections.emptyMap()), randomFrom(contexts.values())); scriptService.compile(new Script(ScriptType.INLINE, "test", "2+2", Collections.emptyMap()), randomFrom(contexts.values())); @@ -361,7 +369,7 @@ public void testFallbackContextSettings() { TimeValue cacheExpireFooParsed = TimeValue.parseTimeValue(cacheExpireFoo, ""); Settings s = Settings.builder() - .put(ScriptService.SCRIPT_GENERAL_CACHE_SIZE_SETTING.getKey(), cacheSizeBackup) + .put(SCRIPT_GENERAL_CACHE_SIZE_SETTING.getKey(), cacheSizeBackup) .put(ScriptService.SCRIPT_CACHE_SIZE_SETTING.getConcreteSettingForNamespace("foo").getKey(), cacheSizeFoo) .put(ScriptService.SCRIPT_GENERAL_CACHE_EXPIRE_SETTING.getKey(), cacheExpireBackup) .put(ScriptService.SCRIPT_CACHE_EXPIRE_SETTING.getConcreteSettingForNamespace("foo").getKey(), cacheExpireFoo) @@ -396,7 +404,7 @@ public void testCacheHolderGeneralConstructor() { boolean compilationLimitsEnabled = true; ScriptService.CacheHolder holder = new ScriptService.CacheHolder( Settings.builder().put(SCRIPT_GENERAL_MAX_COMPILATIONS_RATE_SETTING.getKey(), compilationRate).build(), - new HashSet<>(Collections.singleton("foo")), + Set.of(newContext("foo")), compilationLimitsEnabled ); @@ -407,7 +415,7 @@ public void testCacheHolderGeneralConstructor() { compilationLimitsEnabled = false; holder = new ScriptService.CacheHolder( Settings.builder().put(SCRIPT_GENERAL_MAX_COMPILATIONS_RATE_SETTING.getKey(), compilationRate).build(), - new HashSet<>(Collections.singleton("foo")), + new HashSet<>(Collections.singleton(newContext("foo"))), compilationLimitsEnabled ); @@ -426,13 +434,13 @@ public void testCacheHolderContextConstructor() { .put(SCRIPT_MAX_COMPILATIONS_RATE_SETTING.getConcreteSettingForNamespace("foo").getKey(), fooCompilationRate) .put(SCRIPT_MAX_COMPILATIONS_RATE_SETTING.getConcreteSettingForNamespace("bar").getKey(), barCompilationRate) .build(); - Set contexts = Set.of("foo", "bar", "baz"); + Collection> contexts = Set.of(newContext("foo"), newContext("bar"), newContext("baz")); ScriptService.CacheHolder holder = new ScriptService.CacheHolder(s, contexts, compilationLimitsEnabled); assertNull(holder.general); assertNotNull(holder.contextCache); assertEquals(3, holder.contextCache.size()); - assertEquals(contexts, holder.contextCache.keySet()); + assertEquals(contexts.stream().map(c -> c.name).collect(Collectors.toSet()), 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); @@ -459,7 +467,7 @@ public void testCacheHolderChangeSettings() { Settings s = Settings.builder() .put(SCRIPT_GENERAL_MAX_COMPILATIONS_RATE_SETTING.getKey(), compilationRate) .build(); - Set contexts = Set.of("foo", "bar", "baz"); + Collection> contexts = Set.of(newContext("foo"), newContext("bar"), newContext("baz")); ScriptService.CacheHolder holder = new ScriptService.CacheHolder(s, contexts, true); assertNotNull(holder.general); @@ -476,7 +484,7 @@ public void testCacheHolderChangeSettings() { assertNull(holder.general); assertNotNull(holder.contextCache); assertEquals(3, holder.contextCache.size()); - assertEquals(contexts, holder.contextCache.keySet()); + assertEquals(contexts.stream().map(c -> c.name).collect(Collectors.toSet()), 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); @@ -485,7 +493,7 @@ public void testCacheHolderChangeSettings() { holder.updateContextSettings(Settings.builder() .put(SCRIPT_MAX_COMPILATIONS_RATE_SETTING.getConcreteSettingForNamespace("bar").getKey(), fooCompilationRate).build(), - "bar" + newContext("bar") ); assertEquals(ScriptService.MAX_COMPILATION_RATE_FUNCTION.apply(fooCompilationRate), holder.contextCache.get("bar").get().rate); @@ -508,6 +516,92 @@ public void testCacheHolderChangeSettings() { assertSame(holder, update); } + public void testFallbackToContextDefaults() { + Tuple contextDefaultRate = new Tuple<>(randomIntBetween(10, 1024), + TimeValue.timeValueMinutes(randomIntBetween(10, 200))); + String name = "foo"; + ScriptContext foo = new ScriptContext<>(name, + ScriptContextTests.DummyScript.Factory.class, + randomIntBetween(1, 1024), + TimeValue.timeValueMinutes(randomIntBetween(10, 200)), + contextDefaultRate); + + + int generalCacheSize = randomIntBetween(1, 1024); + TimeValue generalExpire = TimeValue.timeValueMinutes(randomIntBetween(10, 200)); + + String contextRateStr = randomIntBetween(10, 1024) + "/" + randomIntBetween(10, 200) + "m"; + Tuple contextRate = MAX_COMPILATION_RATE_FUNCTION.apply(contextRateStr); + int contextCacheSize = randomIntBetween(1, 1024); + TimeValue contextExpire = TimeValue.timeValueMinutes(randomIntBetween(10, 200)); + + // Use context specific + ScriptService.CacheHolder contextCache = new ScriptService.CacheHolder( + Settings.builder() + .put(SCRIPT_CACHE_SIZE_SETTING.getConcreteSettingForNamespace(name).getKey(), contextCacheSize) + .put(SCRIPT_CACHE_EXPIRE_SETTING.getConcreteSettingForNamespace(name).getKey(), contextExpire) + .put(SCRIPT_MAX_COMPILATIONS_RATE_SETTING.getConcreteSettingForNamespace(name).getKey(), contextRateStr) + .put(SCRIPT_GENERAL_CACHE_SIZE_SETTING.getKey(), generalCacheSize) + .put(SCRIPT_GENERAL_CACHE_EXPIRE_SETTING.getKey(), generalExpire) + .put(SCRIPT_GENERAL_MAX_COMPILATIONS_RATE_SETTING.getKey(), USE_CONTEXT_RATE_KEY) + .build(), + List.of(foo), + true); + + assertNotNull(contextCache.contextCache); + assertNotNull(contextCache.contextCache.get(name)); + assertNotNull(contextCache.contextCache.get(name).get()); + + assertEquals(contextRate, contextCache.contextCache.get(name).get().rate); + assertEquals(contextCacheSize, contextCache.contextCache.get(name).get().cacheSize); + assertEquals(contextExpire, contextCache.contextCache.get(name).get().cacheExpire); + + // Fallback to general + contextCache = new ScriptService.CacheHolder( + Settings.builder() + .put(SCRIPT_GENERAL_CACHE_SIZE_SETTING.getKey(), generalCacheSize) + .put(SCRIPT_GENERAL_CACHE_EXPIRE_SETTING.getKey(), generalExpire) + .put(SCRIPT_GENERAL_MAX_COMPILATIONS_RATE_SETTING.getKey(), USE_CONTEXT_RATE_KEY) + .build(), + List.of(foo), + true); + + assertNotNull(contextCache.contextCache); + assertNotNull(contextCache.contextCache.get(name)); + assertNotNull(contextCache.contextCache.get(name).get()); + + assertEquals(contextDefaultRate, contextCache.contextCache.get(name).get().rate); + assertEquals(generalCacheSize, contextCache.contextCache.get(name).get().cacheSize); + assertEquals(generalExpire, contextCache.contextCache.get(name).get().cacheExpire); + + // Fallback to context defaults + contextCache = new ScriptService.CacheHolder( + Settings.builder() + .put(SCRIPT_GENERAL_MAX_COMPILATIONS_RATE_SETTING.getKey(), USE_CONTEXT_RATE_KEY) + .build(), + List.of(foo), + true); + + assertNotNull(contextCache.contextCache); + assertNotNull(contextCache.contextCache.get(name)); + assertNotNull(contextCache.contextCache.get(name).get()); + + assertEquals(contextDefaultRate, contextCache.contextCache.get(name).get().rate); + assertEquals(foo.cacheSizeDefault, contextCache.contextCache.get(name).get().cacheSize); + assertEquals(foo.cacheExpireDefault, contextCache.contextCache.get(name).get().cacheExpire); + + // Use context specific for ingest + contextCache = new ScriptService.CacheHolder( + Settings.builder() + .put(SCRIPT_GENERAL_MAX_COMPILATIONS_RATE_SETTING.getKey(), USE_CONTEXT_RATE_KEY) + .build(), + List.of(foo, IngestScript.CONTEXT), + true); + assertEquals(new Tuple<>(375, TimeValue.timeValueMinutes(5)), contextCache.contextCache.get("ingest").get().rate); + assertEquals(200, contextCache.contextCache.get("ingest").get().cacheSize); + assertEquals(TimeValue.timeValueMillis(0), contextCache.contextCache.get("ingest").get().cacheExpire); + } + private void assertCompileRejected(String lang, String script, ScriptType scriptType, ScriptContext scriptContext) { try { scriptService.compile(new Script(scriptType, lang, script, Collections.emptyMap()), scriptContext); @@ -524,4 +618,8 @@ private void assertCompileAccepted(String lang, String script, ScriptType script notNullValue() ); } + + ScriptContext newContext(String name) { + return new ScriptContext<>(name, ScriptContextTests.DummyScript.Factory.class); + } }