From c90363edd855602d2fee0612c935e5a3a5fe628e Mon Sep 17 00:00:00 2001 From: Stuart Tettemer Date: Mon, 18 Oct 2021 20:57:46 -0500 Subject: [PATCH 1/7] Revert to general and script cache --- .../org/elasticsearch/script/ScriptCache.java | 4 + .../script/ScriptCacheStats.java | 149 +++++++++++ .../elasticsearch/script/ScriptMetrics.java | 7 +- .../elasticsearch/script/ScriptService.java | 134 +++++++++- .../org/elasticsearch/script/ScriptStats.java | 48 +++- .../script/ScriptCacheTests.java | 41 ++++ .../script/ScriptServiceTests.java | 231 +++++++++++++++--- .../script/ScriptStatsTests.java | 9 + 8 files changed, 567 insertions(+), 56 deletions(-) create mode 100644 server/src/main/java/org/elasticsearch/script/ScriptCacheStats.java diff --git a/server/src/main/java/org/elasticsearch/script/ScriptCache.java b/server/src/main/java/org/elasticsearch/script/ScriptCache.java index 622e9910ac372..6e1ab496105da 100644 --- a/server/src/main/java/org/elasticsearch/script/ScriptCache.java +++ b/server/src/main/java/org/elasticsearch/script/ScriptCache.java @@ -121,6 +121,10 @@ static void rethrow(Throwable t) throws T { throw (T) t; } + public ScriptStats stats() { + return scriptMetrics.stats(); + } + public ScriptContextStats stats(String context) { return scriptMetrics.stats(context); } diff --git a/server/src/main/java/org/elasticsearch/script/ScriptCacheStats.java b/server/src/main/java/org/elasticsearch/script/ScriptCacheStats.java new file mode 100644 index 0000000000000..d580f94a65a68 --- /dev/null +++ b/server/src/main/java/org/elasticsearch/script/ScriptCacheStats.java @@ -0,0 +1,149 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the Elastic License + * 2.0 and the Server Side Public License, v 1; you may not use this file except + * in compliance with, at your election, the Elastic License 2.0 or the Server + * Side Public License, v 1. + */ + +package org.elasticsearch.script; + +import org.elasticsearch.common.io.stream.StreamInput; +import org.elasticsearch.common.io.stream.StreamOutput; +import org.elasticsearch.common.io.stream.Writeable; +import org.elasticsearch.xcontent.ToXContentFragment; +import org.elasticsearch.xcontent.XContentBuilder; + +import java.io.IOException; +import java.util.Collections; +import java.util.HashMap; +import java.util.Map; +import java.util.Objects; +import java.util.stream.Collectors; + +// This class is deprecated in favor of ScriptStats and ScriptContextStats. It is removed in 8. +public class ScriptCacheStats implements Writeable, ToXContentFragment { + private final Map context; + private final ScriptStats general; + + public ScriptCacheStats(Map context) { + this.context = Collections.unmodifiableMap(context); + this.general = null; + } + + public ScriptCacheStats(ScriptStats general) { + this.general = Objects.requireNonNull(general); + this.context = null; + } + + public ScriptCacheStats(StreamInput in) throws IOException { + boolean isContext = in.readBoolean(); + if (isContext == false) { + general = new ScriptStats(in); + context = null; + return; + } + + general = null; + int size = in.readInt(); + Map context = new HashMap<>(size); + for (int i=0; i < size; i++) { + String name = in.readString(); + context.put(name, new ScriptStats(in)); + } + this.context = Collections.unmodifiableMap(context); + } + + @Override + public void writeTo(StreamOutput out) throws IOException { + if (general != null) { + out.writeBoolean(false); + general.writeTo(out); + return; + } + + out.writeBoolean(true); + out.writeInt(context.size()); + for (String name: context.keySet().stream().sorted().collect(Collectors.toList())) { + out.writeString(name); + context.get(name).writeTo(out); + } + } + + @Override + public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException { + builder.startObject(Fields.SCRIPT_CACHE_STATS); + builder.startObject(Fields.SUM); + if (general != null) { + builder.field(ScriptStats.Fields.COMPILATIONS, general.getCompilations()); + builder.field(ScriptStats.Fields.CACHE_EVICTIONS, general.getCacheEvictions()); + builder.field(ScriptStats.Fields.COMPILATION_LIMIT_TRIGGERED, general.getCompilationLimitTriggered()); + builder.endObject().endObject(); + return builder; + } + + ScriptStats sum = sum(); + builder.field(ScriptStats.Fields.COMPILATIONS, sum.getCompilations()); + builder.field(ScriptStats.Fields.CACHE_EVICTIONS, sum.getCacheEvictions()); + builder.field(ScriptStats.Fields.COMPILATION_LIMIT_TRIGGERED, sum.getCompilationLimitTriggered()); + builder.endObject(); + + builder.startArray(Fields.CONTEXTS); + for (String name: context.keySet().stream().sorted().collect(Collectors.toList())) { + ScriptStats stats = context.get(name); + builder.startObject(); + builder.field(Fields.CONTEXT, name); + builder.field(ScriptStats.Fields.COMPILATIONS, stats.getCompilations()); + builder.field(ScriptStats.Fields.CACHE_EVICTIONS, stats.getCacheEvictions()); + builder.field(ScriptStats.Fields.COMPILATION_LIMIT_TRIGGERED, stats.getCompilationLimitTriggered()); + builder.endObject(); + } + builder.endArray(); + builder.endObject(); + + return builder; + } + + /** + * Get the context specific stats, null if using general cache + */ + public Map getContextStats() { + return context; + } + + /** + * Get the general stats, null if using context cache + */ + public ScriptStats getGeneralStats() { + return general; + } + + /** + * The sum of all script stats, either the general stats or the sum of all stats of the context stats. + */ + public ScriptStats sum() { + if (general != null) { + return general; + } + long compilations = 0; + long cacheEvictions = 0; + long compilationLimitTriggered = 0; + for (ScriptStats stat: context.values()) { + compilations += stat.getCompilations(); + cacheEvictions += stat.getCacheEvictions(); + compilationLimitTriggered += stat.getCompilationLimitTriggered(); + } + return new ScriptStats( + compilations, + cacheEvictions, + compilationLimitTriggered + ); + } + + static final class Fields { + static final String SCRIPT_CACHE_STATS = "script_cache"; + static final String CONTEXT = "context"; + static final String SUM = "sum"; + static final String CONTEXTS = "contexts"; + } +} diff --git a/server/src/main/java/org/elasticsearch/script/ScriptMetrics.java b/server/src/main/java/org/elasticsearch/script/ScriptMetrics.java index f853305c3cd45..c4d26bc861c8c 100644 --- a/server/src/main/java/org/elasticsearch/script/ScriptMetrics.java +++ b/server/src/main/java/org/elasticsearch/script/ScriptMetrics.java @@ -27,6 +27,10 @@ public void onCompilationLimit() { compilationLimitTriggered.inc(); } + public ScriptStats stats() { + return new ScriptStats(compilationsMetric.count(), cacheEvictionsMetric.count(), compilationLimitTriggered.count()); + } + public ScriptContextStats stats(String context) { return new ScriptContextStats( context, @@ -36,4 +40,5 @@ public ScriptContextStats stats(String context) { new ScriptContextStats.TimeSeries(), new ScriptContextStats.TimeSeries() ); - }} + } +} diff --git a/server/src/main/java/org/elasticsearch/script/ScriptService.java b/server/src/main/java/org/elasticsearch/script/ScriptService.java index d0e9f65425498..d760978b1d93e 100644 --- a/server/src/main/java/org/elasticsearch/script/ScriptService.java +++ b/server/src/main/java/org/elasticsearch/script/ScriptService.java @@ -34,6 +34,7 @@ import java.io.IOException; import java.nio.charset.StandardCharsets; import java.util.ArrayList; +import java.util.Arrays; import java.util.Collections; import java.util.Comparator; import java.util.HashMap; @@ -52,8 +53,21 @@ public class ScriptService implements Closeable, ClusterStateApplier, ScriptComp static final String DISABLE_DYNAMIC_SCRIPTING_SETTING = "script.disable_dynamic"; + // Special setting value for SCRIPT_GENERAL_MAX_COMPILATIONS_RATE to indicate the script service should use context + // specific caches + static final ScriptCache.CompilationRate USE_CONTEXT_RATE_VALUE = new ScriptCache.CompilationRate(-1, TimeValue.MINUS_ONE); + static final String USE_CONTEXT_RATE_KEY = "use-context"; + + public static final Setting SCRIPT_GENERAL_CACHE_SIZE_SETTING = + Setting.intSetting("script.cache.max_size", 100, 0, Property.NodeScope); + public static final Setting SCRIPT_GENERAL_CACHE_EXPIRE_SETTING = + Setting.positiveTimeSetting("script.cache.expire", TimeValue.timeValueMillis(0), Property.NodeScope); public static final Setting SCRIPT_MAX_SIZE_IN_BYTES = Setting.intSetting("script.max_size_in_bytes", 65535, 0, Property.Dynamic, Property.NodeScope); + public static final Setting SCRIPT_GENERAL_MAX_COMPILATIONS_RATE_SETTING = + new Setting<>("script.max_compilations_rate", USE_CONTEXT_RATE_KEY, + (String value) -> value.equals(USE_CONTEXT_RATE_KEY) ? USE_CONTEXT_RATE_VALUE: new ScriptCache.CompilationRate(value), + Property.Dynamic, Property.NodeScope); // Per-context settings static final String CONTEXT_PREFIX = "script.context."; @@ -62,11 +76,14 @@ public class ScriptService implements Closeable, ClusterStateApplier, ScriptComp public static final Setting.AffixSetting SCRIPT_CACHE_SIZE_SETTING = Setting.affixKeySetting(CONTEXT_PREFIX, - "cache_max_size", key -> Setting.intSetting(key, 0, Property.NodeScope, Property.Dynamic)); + "cache_max_size", + key -> Setting.intSetting(key, SCRIPT_GENERAL_CACHE_SIZE_SETTING, 0, Property.NodeScope, Property.Dynamic)); public static final Setting.AffixSetting SCRIPT_CACHE_EXPIRE_SETTING = Setting.affixKeySetting(CONTEXT_PREFIX, - "cache_expire", key -> Setting.positiveTimeSetting(key, TimeValue.timeValueMillis(0), Property.NodeScope, Property.Dynamic)); + "cache_expire", + 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"; @@ -76,7 +93,7 @@ public class ScriptService implements Closeable, ClusterStateApplier, ScriptComp "max_compilations_rate", key -> new Setting(key, "75/5m", (String value) -> value.equals(UNLIMITED_COMPILATION_RATE_KEY) ? ScriptCache.UNLIMITED_COMPILATION_RATE: - new ScriptCache.CompilationRate(value), + new ScriptCache.CompilationRate(value), Property.NodeScope, Property.Dynamic)); private static final ScriptCache.CompilationRate SCRIPT_COMPILATION_RATE_ZERO = new ScriptCache.CompilationRate(0, TimeValue.ZERO); @@ -185,7 +202,7 @@ public ScriptService(Settings settings, Map engines, Map context: contexts.values()) { clusterSettings.addSettingsUpdateConsumer( (settings) -> cacheHolder.get().set(context.name, contextCache(settings, 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) + Arrays.asList(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 ) ); } + + // Handle all settings for context and general caches, this flips between general and context caches. + clusterSettings.addSettingsUpdateConsumer( + (settings) -> setCacheHolder(settings), + Arrays.asList(SCRIPT_GENERAL_MAX_COMPILATIONS_RATE_SETTING, + SCRIPT_GENERAL_CACHE_EXPIRE_SETTING, + SCRIPT_GENERAL_CACHE_SIZE_SETTING, + SCRIPT_MAX_COMPILATIONS_RATE_SETTING, + SCRIPT_DISABLE_MAX_COMPILATIONS_RATE_SETTING, + SCRIPT_CACHE_EXPIRE_SETTING, + SCRIPT_CACHE_SIZE_SETTING), + this::validateCacheSettings + ); } /** @@ -215,8 +248,9 @@ void registerClusterSettingsListeners(ClusterSettings clusterSettings) { * when using the general cache. */ void validateCacheSettings(Settings settings) { - List> affixes = List.of(SCRIPT_MAX_COMPILATIONS_RATE_SETTING, SCRIPT_CACHE_EXPIRE_SETTING, - SCRIPT_CACHE_SIZE_SETTING); + boolean useContext = SCRIPT_GENERAL_MAX_COMPILATIONS_RATE_SETTING.get(settings).equals(USE_CONTEXT_RATE_VALUE); + List> affixes = Arrays.asList(SCRIPT_MAX_COMPILATIONS_RATE_SETTING, SCRIPT_CACHE_EXPIRE_SETTING, + SCRIPT_CACHE_SIZE_SETTING); List customRates = new ArrayList<>(); List keys = new ArrayList<>(); for (Setting.AffixSetting affix: affixes) { @@ -231,6 +265,11 @@ void validateCacheSettings(Settings settings) { } } } + if (useContext == false && keys.isEmpty() == false) { + keys.sort(Comparator.naturalOrder()); + throw new IllegalArgumentException("Context cache settings [" + String.join(", ", keys) + "] requires [" + + SCRIPT_GENERAL_MAX_COMPILATIONS_RATE_SETTING.getKey() + "] to be [" + USE_CONTEXT_RATE_KEY + "]"); + } if (SCRIPT_DISABLE_MAX_COMPILATIONS_RATE_SETTING.get(settings)) { if (customRates.size() > 0) { customRates.sort(Comparator.naturalOrder()); @@ -238,6 +277,12 @@ void validateCacheSettings(Settings settings) { String.join(", ", customRates) + "] if compile rates disabled via [" + SCRIPT_DISABLE_MAX_COMPILATIONS_RATE_SETTING.getKey() + "]"); } + if (useContext == false) { + throw new IllegalArgumentException("Cannot set custom general compilation rates [" + + SCRIPT_GENERAL_MAX_COMPILATIONS_RATE_SETTING.getKey() + "] to [" + + SCRIPT_GENERAL_MAX_COMPILATIONS_RATE_SETTING.get(settings) + "] if compile rates disabled via [" + + SCRIPT_DISABLE_MAX_COMPILATIONS_RATE_SETTING.getKey() + "]"); + } } } @@ -489,11 +534,48 @@ public ScriptStats stats() { return cacheHolder.get().stats(); } + public ScriptCacheStats cacheStats() { + return cacheHolder.get().cacheStats(); + } + @Override public void applyClusterState(ClusterChangedEvent event) { clusterState = event.state(); } + void setCacheHolder(Settings settings) { + CacheHolder current = cacheHolder.get(); + boolean useContext = SCRIPT_GENERAL_MAX_COMPILATIONS_RATE_SETTING.get(settings).equals(USE_CONTEXT_RATE_VALUE); + + if (current == null) { + if (useContext) { + cacheHolder.set(contextCacheHolder(settings)); + } else { + cacheHolder.set(generalCacheHolder(settings)); + } + return; + } + + // Update + if (useContext) { + if (current.general != null) { + // Flipping to context specific + cacheHolder.set(contextCacheHolder(settings)); + } + } else if (current.general == null) { + // Flipping to general + cacheHolder.set(generalCacheHolder(settings)); + } else if (current.general.rate.equals(SCRIPT_GENERAL_MAX_COMPILATIONS_RATE_SETTING.get(settings)) == false) { + // General compilation rate changed, that setting is the only dynamically updated general setting + cacheHolder.set(generalCacheHolder(settings)); + } + } + + CacheHolder generalCacheHolder(Settings settings) { + return new CacheHolder(SCRIPT_GENERAL_CACHE_SIZE_SETTING.get(settings), SCRIPT_GENERAL_CACHE_EXPIRE_SETTING.get(settings), + SCRIPT_GENERAL_MAX_COMPILATIONS_RATE_SETTING.get(settings), SCRIPT_GENERAL_MAX_COMPILATIONS_RATE_SETTING.getKey()); + } + CacheHolder contextCacheHolder(Settings settings) { Map contextCache = new HashMap<>(contexts.size()); contexts.forEach((k, v) -> contextCache.put(k, contextCache(settings, v))); @@ -528,12 +610,19 @@ ScriptCache contextCache(Settings settings, ScriptContext context) { * 2) context mode, if the context script cache is configured. There is no general cache in this case. */ static class CacheHolder { + final ScriptCache general; final Map> contextCache; + CacheHolder(int cacheMaxSize, TimeValue cacheExpire, ScriptCache.CompilationRate maxCompilationRate, String contextRateSetting) { + contextCache = null; + general = new ScriptCache(cacheMaxSize, cacheExpire, maxCompilationRate, contextRateSetting); + } + CacheHolder(Map context) { Map> refs = new HashMap<>(context.size()); context.forEach((k, v) -> refs.put(k, new AtomicReference<>(v))); contextCache = Collections.unmodifiableMap(refs); + general = null; } /** @@ -541,6 +630,9 @@ static class CacheHolder { * the given context. Returns null in context mode if the requested context does not exist. */ ScriptCache get(String context) { + if (general != null) { + return general; + } AtomicReference ref = contextCache.get(context); if (ref == null) { return null; @@ -549,17 +641,35 @@ ScriptCache get(String context) { } ScriptStats stats() { - List stats = new ArrayList<>(contextCache.size()); + if (general != null) { + return general.stats(); + } + List contextStats = new ArrayList<>(contextCache.size()); for (Map.Entry> entry : contextCache.entrySet()) { - stats.add(entry.getValue().get().stats(entry.getKey())); + ScriptCache cache = entry.getValue().get(); + contextStats.add(cache.stats(entry.getKey())); + } + return new ScriptStats(contextStats); + } + + ScriptCacheStats cacheStats() { + if (general != null) { + return new ScriptCacheStats(general.stats()); + } + Map context = new HashMap<>(contextCache.size()); + for (String name: contextCache.keySet()) { + context.put(name, contextCache.get(name).get().stats()); } - return new ScriptStats(stats); + return new ScriptCacheStats(context); } /** * Update a single context cache if we're in the context cache mode otherwise no-op. */ void set(String name, ScriptCache cache) { + if (general != null) { + return; + } AtomicReference ref = contextCache.get(name); assert ref != null : "expected script cache to exist for context [" + name + "]"; ScriptCache oldCache = ref.get(); diff --git a/server/src/main/java/org/elasticsearch/script/ScriptStats.java b/server/src/main/java/org/elasticsearch/script/ScriptStats.java index 55ba2a847f7bb..b360a669052aa 100644 --- a/server/src/main/java/org/elasticsearch/script/ScriptStats.java +++ b/server/src/main/java/org/elasticsearch/script/ScriptStats.java @@ -8,6 +8,7 @@ package org.elasticsearch.script; +import org.elasticsearch.Version; import org.elasticsearch.common.io.stream.StreamInput; import org.elasticsearch.common.io.stream.StreamOutput; import org.elasticsearch.common.io.stream.Writeable; @@ -15,8 +16,11 @@ import org.elasticsearch.xcontent.XContentBuilder; import java.io.IOException; +import java.util.ArrayList; +import java.util.Collections; +import java.util.HashMap; import java.util.List; -import java.util.stream.Collectors; +import java.util.Map; public class ScriptStats implements Writeable, ToXContentFragment { private final List contextStats; @@ -24,9 +28,11 @@ public class ScriptStats implements Writeable, ToXContentFragment { private final long cacheEvictions; private final long compilationLimitTriggered; - public ScriptStats(List contextStats) { - this.contextStats = contextStats.stream().sorted().collect(Collectors.toUnmodifiableList()); + ArrayList ctxStats = new ArrayList<>(contextStats.size()); + ctxStats.addAll(contextStats); + ctxStats.sort(ScriptContextStats::compareTo); + this.contextStats = Collections.unmodifiableList(ctxStats); long compilations = 0; long cacheEvictions = 0; long compilationLimitTriggered = 0; @@ -40,19 +46,34 @@ public ScriptStats(List contextStats) { this.compilationLimitTriggered = compilationLimitTriggered; } + public ScriptStats(long compilations, long cacheEvictions, long compilationLimitTriggered) { + this.contextStats = Collections.emptyList(); + this.compilations = compilations; + this.cacheEvictions = cacheEvictions; + this.compilationLimitTriggered = compilationLimitTriggered; + } + + public ScriptStats(ScriptContextStats context) { + this(context.getCompilations(), context.getCacheEvictions(), context.getCompilationLimitTriggered()); + } + public ScriptStats(StreamInput in) throws IOException { compilations = in.readVLong(); cacheEvictions = in.readVLong(); - compilationLimitTriggered = in.readVLong(); - contextStats = in.readList(ScriptContextStats::new); + compilationLimitTriggered = in.getVersion().onOrAfter(Version.V_7_0_0) ? in.readVLong() : 0; + contextStats = in.getVersion().onOrAfter(Version.V_7_9_0) ? in.readList(ScriptContextStats::new) : Collections.emptyList(); } @Override public void writeTo(StreamOutput out) throws IOException { out.writeVLong(compilations); out.writeVLong(cacheEvictions); - out.writeVLong(compilationLimitTriggered); - out.writeList(contextStats); + if (out.getVersion().onOrAfter(Version.V_7_0_0)) { + out.writeVLong(compilationLimitTriggered); + } + if (out.getVersion().onOrAfter(Version.V_7_9_0)) { + out.writeList(contextStats); + } } public List getContextStats() { @@ -71,17 +92,30 @@ public long getCompilationLimitTriggered() { return compilationLimitTriggered; } + public ScriptCacheStats toScriptCacheStats() { + if (contextStats.isEmpty()) { + return new ScriptCacheStats(this); + } + Map contexts = new HashMap<>(contextStats.size()); + for (ScriptContextStats contextStats : contextStats) { + contexts.put(contextStats.getContext(), new ScriptStats(contextStats)); + } + return new ScriptCacheStats(contexts); + } + @Override public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException { builder.startObject(Fields.SCRIPT_STATS); builder.field(Fields.COMPILATIONS, compilations); builder.field(Fields.CACHE_EVICTIONS, cacheEvictions); builder.field(Fields.COMPILATION_LIMIT_TRIGGERED, compilationLimitTriggered); + /* TODO(stu): master only builder.startArray(Fields.CONTEXTS); for (ScriptContextStats contextStats: contextStats) { contextStats.toXContent(builder, params); } builder.endArray(); + */ builder.endObject(); return builder; } diff --git a/server/src/test/java/org/elasticsearch/script/ScriptCacheTests.java b/server/src/test/java/org/elasticsearch/script/ScriptCacheTests.java index 6ee7149a460ec..00c3c1267403c 100644 --- a/server/src/test/java/org/elasticsearch/script/ScriptCacheTests.java +++ b/server/src/test/java/org/elasticsearch/script/ScriptCacheTests.java @@ -55,6 +55,33 @@ public void testCompilationCircuitBreaking() throws Exception { } } + public void testGeneralCompilationCircuitBreaking() throws Exception { + final TimeValue expire = ScriptService.SCRIPT_GENERAL_CACHE_EXPIRE_SETTING.get(Settings.EMPTY); + final Integer size = ScriptService.SCRIPT_GENERAL_CACHE_SIZE_SETTING.get(Settings.EMPTY); + String settingName = ScriptService.SCRIPT_GENERAL_MAX_COMPILATIONS_RATE_SETTING.getKey(); + ScriptCache cache = new ScriptCache(size, expire, new ScriptCache.CompilationRate(1, TimeValue.timeValueMinutes(1)), settingName); + cache.checkCompilationLimit(); // should pass + expectThrows(CircuitBreakingException.class, cache::checkCompilationLimit); + cache = new ScriptCache(size, expire, new ScriptCache.CompilationRate(2, TimeValue.timeValueMinutes(1)), settingName); + cache.checkCompilationLimit(); // should pass + cache.checkCompilationLimit(); // should pass + expectThrows(CircuitBreakingException.class, cache::checkCompilationLimit); + int count = randomIntBetween(5, 50); + cache = new ScriptCache(size, expire, new ScriptCache.CompilationRate(count, TimeValue.timeValueMinutes(1)), settingName); + for (int i = 0; i < count; i++) { + cache.checkCompilationLimit(); // should pass + } + expectThrows(CircuitBreakingException.class, cache::checkCompilationLimit); + cache = new ScriptCache(size, expire, new ScriptCache.CompilationRate(0, TimeValue.timeValueMinutes(1)), settingName); + expectThrows(CircuitBreakingException.class, cache::checkCompilationLimit); + cache = new ScriptCache(size, expire, + new ScriptCache.CompilationRate(Integer.MAX_VALUE, TimeValue.timeValueMinutes(1)), settingName); + int largeLimit = randomIntBetween(1000, 10000); + for (int i = 0; i < largeLimit; i++) { + cache.checkCompilationLimit(); + } + } + public void testUnlimitedCompilationRate() { String context = randomFrom( ScriptModule.CORE_CONTEXTS.values().stream().filter( @@ -73,4 +100,18 @@ public void testUnlimitedCompilationRate() { assertEquals(initialState.availableTokens, currentState.availableTokens, 0.0); // delta of 0.0 because it should never change } } + + public void testGeneralUnlimitedCompilationRate() { + final Integer size = ScriptService.SCRIPT_GENERAL_CACHE_SIZE_SETTING.get(Settings.EMPTY); + final TimeValue expire = ScriptService.SCRIPT_GENERAL_CACHE_EXPIRE_SETTING.get(Settings.EMPTY); + String settingName = ScriptService.SCRIPT_GENERAL_MAX_COMPILATIONS_RATE_SETTING.getKey(); + ScriptCache cache = new ScriptCache(size, expire, ScriptCache.UNLIMITED_COMPILATION_RATE, settingName); + ScriptCache.TokenBucketState initialState = cache.tokenBucketState.get(); + for(int i=0; i < 3000; i++) { + cache.checkCompilationLimit(); + ScriptCache.TokenBucketState currentState = cache.tokenBucketState.get(); + assertEquals(initialState.lastInlineCompileTime, currentState.lastInlineCompileTime); + assertEquals(initialState.availableTokens, currentState.availableTokens, 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 1ea9780c762a6..8e182e1b02353 100644 --- a/server/src/test/java/org/elasticsearch/script/ScriptServiceTests.java +++ b/server/src/test/java/org/elasticsearch/script/ScriptServiceTests.java @@ -36,6 +36,9 @@ 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_MAX_COMPILATIONS_RATE_SETTING; +import static org.elasticsearch.script.ScriptService.SCRIPT_GENERAL_CACHE_SIZE_SETTING; import static org.elasticsearch.script.ScriptService.SCRIPT_MAX_COMPILATIONS_RATE_SETTING; import static org.hamcrest.CoreMatchers.containsString; import static org.hamcrest.Matchers.is; @@ -216,7 +219,10 @@ public void testMultipleCompilationsCountedInCompilationStats() throws IOExcepti } public void testCompilationStatsOnCacheHit() throws IOException { - buildScriptService(Settings.EMPTY); + Settings.Builder builder = Settings.builder() + .put(SCRIPT_GENERAL_CACHE_SIZE_SETTING.getKey(), 1) + .put(SCRIPT_GENERAL_MAX_COMPILATIONS_RATE_SETTING.getKey(), "2/1m"); + buildScriptService(builder.build()); Script script = new Script(ScriptType.INLINE, "test", "1+1", Collections.emptyMap()); ScriptContext context = randomFrom(contexts.values()); scriptService.compile(script, context); @@ -229,19 +235,20 @@ public void testIndexedScriptCountedInCompilationStats() throws IOException { ScriptContext ctx = randomFrom(contexts.values()); scriptService.compile(new Script(ScriptType.STORED, null, "script", Collections.emptyMap()), ctx); assertEquals(1L, scriptService.stats().getCompilations()); - assertEquals(1L, getByContext(scriptService.stats(), ctx.name).getCompilations()); + assertEquals(1L, scriptService.cacheStats().getContextStats().get(ctx.name).getCompilations()); } public void testCacheEvictionCountedInCacheEvictionsStats() throws IOException { - ScriptContext context = randomFrom(contexts.values()); - buildScriptService(Settings.builder() - .put(SCRIPT_CACHE_SIZE_SETTING.getConcreteSettingForNamespace(context.name).getKey(), 1) - .build() - ); - scriptService.compile(new Script(ScriptType.INLINE, "test", "1+1", Collections.emptyMap()), context); - scriptService.compile(new Script(ScriptType.INLINE, "test", "2+2", Collections.emptyMap()), context); + Settings.Builder builder = Settings.builder(); + builder.put(SCRIPT_GENERAL_CACHE_SIZE_SETTING.getKey(), 1); + builder.put(SCRIPT_GENERAL_MAX_COMPILATIONS_RATE_SETTING.getKey(), "10/1m"); + 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())); assertEquals(2L, scriptService.stats().getCompilations()); + assertEquals(2L, scriptService.cacheStats().getGeneralStats().getCompilations()); assertEquals(1L, scriptService.stats().getCacheEvictions()); + assertEquals(1L, scriptService.cacheStats().getGeneralStats().getCacheEvictions()); } public void testContextCacheStats() throws IOException { @@ -282,14 +289,15 @@ public void testContextCacheStats() throws IOException { assertEquals(CircuitBreakingException.class, gse.getRootCause().getClass()); // Context specific - ScriptStats stats = scriptService.stats(); - assertEquals(2L, getByContext(stats, contextA.name).getCompilations()); - assertEquals(1L, getByContext(stats, contextA.name).getCacheEvictions()); - assertEquals(1L, getByContext(stats, contextA.name).getCompilationLimitTriggered()); + ScriptCacheStats stats = scriptService.cacheStats(); + assertEquals(2L, stats.getContextStats().get(contextA.name).getCompilations()); + assertEquals(1L, stats.getContextStats().get(contextA.name).getCacheEvictions()); + assertEquals(1L, stats.getContextStats().get(contextA.name).getCompilationLimitTriggered()); - assertEquals(3L, getByContext(stats, contextB.name).getCompilations()); - assertEquals(1L, getByContext(stats, contextB.name).getCacheEvictions()); - assertEquals(2L, getByContext(stats, contextB.name).getCompilationLimitTriggered()); + assertEquals(3L, stats.getContextStats().get(contextB.name).getCompilations()); + assertEquals(1L, stats.getContextStats().get(contextB.name).getCacheEvictions()); + assertEquals(2L, stats.getContextStats().get(contextB.name).getCompilationLimitTriggered()); + assertNull(scriptService.cacheStats().getGeneralStats()); // Summed up assertEquals(5L, scriptService.stats().getCompilations()); @@ -366,6 +374,99 @@ public void testMaxSizeLimit() throws Exception { iae.getMessage()); } + public void testConflictContextSettings() throws IOException { + IllegalArgumentException illegal = expectThrows(IllegalArgumentException.class, () -> { + buildScriptService(Settings.builder() + .put(SCRIPT_GENERAL_MAX_COMPILATIONS_RATE_SETTING.getKey(), "10/1m") + .put(ScriptService.SCRIPT_CACHE_SIZE_SETTING.getConcreteSettingForNamespace("field").getKey(), 123).build()); + }); + assertEquals("Context cache settings [script.context.field.cache_max_size] requires " + + "[script.max_compilations_rate] to be [use-context]", + illegal.getMessage() + ); + + illegal = expectThrows(IllegalArgumentException.class, () -> { + buildScriptService(Settings.builder() + .put(SCRIPT_GENERAL_MAX_COMPILATIONS_RATE_SETTING.getKey(), "10/1m") + .put(ScriptService.SCRIPT_CACHE_EXPIRE_SETTING.getConcreteSettingForNamespace("ingest").getKey(), "5m").build()); + }); + + assertEquals("Context cache settings [script.context.ingest.cache_expire] requires " + + "[script.max_compilations_rate] to be [use-context]", + illegal.getMessage() + ); + + illegal = expectThrows(IllegalArgumentException.class, () -> { + buildScriptService(Settings.builder() + .put(SCRIPT_GENERAL_MAX_COMPILATIONS_RATE_SETTING.getKey(), "10/1m") + .put(ScriptService.SCRIPT_MAX_COMPILATIONS_RATE_SETTING.getConcreteSettingForNamespace("score").getKey(), "50/5m").build()); + }); + + assertEquals("Context cache settings [script.context.score.max_compilations_rate] requires " + + "[script.max_compilations_rate] to be [use-context]", + illegal.getMessage() + ); + + buildScriptService( + Settings.builder() + .put(ScriptService.SCRIPT_CACHE_EXPIRE_SETTING.getConcreteSettingForNamespace("ingest").getKey(), "5m") + .put(ScriptService.SCRIPT_CACHE_SIZE_SETTING.getConcreteSettingForNamespace("field").getKey(), 123) + .put(ScriptService.SCRIPT_MAX_COMPILATIONS_RATE_SETTING.getConcreteSettingForNamespace("score").getKey(), "50/5m") + .build()); + } + + public void testFallbackContextSettings() { + int cacheSizeBackup = randomIntBetween(0, 1024); + int cacheSizeFoo = randomValueOtherThan(cacheSizeBackup, () -> randomIntBetween(0, 1024)); + + String cacheExpireBackup = randomTimeValue(1, 1000, "h"); + TimeValue cacheExpireBackupParsed = TimeValue.parseTimeValue(cacheExpireBackup, ""); + String cacheExpireFoo = randomValueOtherThan(cacheExpireBackup, () -> randomTimeValue(1, 1000, "h")); + TimeValue cacheExpireFooParsed = TimeValue.parseTimeValue(cacheExpireFoo, ""); + + Settings s = Settings.builder() + .put(SCRIPT_GENERAL_CACHE_SIZE_SETTING.getKey(), cacheSizeBackup) + .put(ScriptService.SCRIPT_CACHE_SIZE_SETTING.getConcreteSettingForNamespace("foo").getKey(), cacheSizeFoo) + .put(SCRIPT_GENERAL_CACHE_EXPIRE_SETTING.getKey(), cacheExpireBackup) + .put(ScriptService.SCRIPT_CACHE_EXPIRE_SETTING.getConcreteSettingForNamespace("foo").getKey(), cacheExpireFoo) + .build(); + + assertEquals(cacheSizeFoo, ScriptService.SCRIPT_CACHE_SIZE_SETTING.getConcreteSettingForNamespace("foo").get(s).intValue()); + assertEquals(cacheSizeBackup, ScriptService.SCRIPT_CACHE_SIZE_SETTING.getConcreteSettingForNamespace("bar").get(s).intValue()); + + assertEquals(cacheExpireFooParsed, ScriptService.SCRIPT_CACHE_EXPIRE_SETTING.getConcreteSettingForNamespace("foo").get(s)); + assertEquals(cacheExpireBackupParsed, ScriptService.SCRIPT_CACHE_EXPIRE_SETTING.getConcreteSettingForNamespace("bar").get(s)); + } + + public void testUseContextSettingValue() { + Settings s = Settings.builder() + .put(ScriptService.SCRIPT_GENERAL_MAX_COMPILATIONS_RATE_SETTING.getKey(), ScriptService.USE_CONTEXT_RATE_KEY) + .put(ScriptService.SCRIPT_MAX_COMPILATIONS_RATE_SETTING.getConcreteSettingForNamespace("foo").getKey(), + ScriptService.USE_CONTEXT_RATE_KEY) + .build(); + + assertEquals(ScriptService.SCRIPT_GENERAL_MAX_COMPILATIONS_RATE_SETTING.get(s), ScriptService.USE_CONTEXT_RATE_VALUE); + + IllegalArgumentException illegal = expectThrows(IllegalArgumentException.class, () -> { + ScriptService.SCRIPT_MAX_COMPILATIONS_RATE_SETTING.getAsMap(s); + }); + + assertEquals("parameter must contain a positive integer and a timevalue, i.e. 10/1m, but was [use-context]", illegal.getMessage()); + } + + public void testCacheHolderGeneralConstructor() throws IOException { + String compilationRate = "77/5m"; + buildScriptService( + Settings.builder().put(SCRIPT_GENERAL_MAX_COMPILATIONS_RATE_SETTING.getKey(), compilationRate).build() + ); + + ScriptService.CacheHolder holder = scriptService.cacheHolder.get(); + + assertNotNull(holder.general); + assertNull(holder.contextCache); + assertEquals(holder.general.rate, new ScriptCache.CompilationRate(compilationRate)); + } + public void testCacheHolderContextConstructor() throws IOException { String a = randomFrom(contexts.keySet()); String b = randomValueOtherThan(a, () -> randomFrom(contexts.keySet())); @@ -377,6 +478,7 @@ public void testCacheHolderContextConstructor() throws IOException { .put(SCRIPT_MAX_COMPILATIONS_RATE_SETTING.getConcreteSettingForNamespace(b).getKey(), bCompilationRate) .build()); + assertNull(scriptService.cacheHolder.get().general); assertNotNull(scriptService.cacheHolder.get().contextCache); assertEquals(contexts.keySet(), scriptService.cacheHolder.get().contextCache.keySet()); @@ -386,6 +488,24 @@ public void testCacheHolderContextConstructor() throws IOException { scriptService.cacheHolder.get().contextCache.get(b).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 testDisableCompilationRateSetting() throws IOException { IllegalArgumentException illegal = expectThrows(IllegalArgumentException.class, () -> { buildScriptService(Settings.builder() @@ -399,6 +519,16 @@ public void testDisableCompilationRateSetting() throws IOException { "[script.disable_max_compilations_rate]", illegal.getMessage()); + illegal = expectThrows(IllegalArgumentException.class, () -> { + buildScriptService(Settings.builder() + .put("script.disable_max_compilations_rate", true) + .put("script.max_compilations_rate", "76/10m") + .build()); + }); + assertEquals("Cannot set custom general compilation rates [script.max_compilations_rate] " + + "to [76/10m] if compile rates disabled via [script.disable_max_compilations_rate]", + illegal.getMessage()); + buildScriptService(Settings.builder() .put("script.disable_max_compilations_rate", true) .build()); @@ -411,20 +541,31 @@ public void testCacheHolderChangeSettings() throws IOException { String b = randomValueOtherThan(a, () -> randomFrom(contextNames)); String bRate = "78/6m"; String c = randomValueOtherThanMany(s -> a.equals(s) || b.equals(s), () -> randomFrom(contextNames)); + String compilationRate = "77/5m"; + ScriptCache.CompilationRate generalRate = new ScriptCache.CompilationRate(compilationRate); - buildScriptService(Settings.EMPTY); - - Settings settings = Settings.builder() - .put(SCRIPT_MAX_COMPILATIONS_RATE_SETTING.getConcreteSettingForNamespace(a).getKey(), aRate) - .put(SCRIPT_MAX_COMPILATIONS_RATE_SETTING.getConcreteSettingForNamespace(b).getKey(), bRate) - .put(SCRIPT_MAX_COMPILATIONS_RATE_SETTING.getConcreteSettingForNamespace(c).getKey(), - ScriptService.UNLIMITED_COMPILATION_RATE_KEY) + Settings s = Settings.builder() + .put(SCRIPT_GENERAL_MAX_COMPILATIONS_RATE_SETTING.getKey(), compilationRate) .build(); + buildScriptService(s); + + assertNotNull(scriptService.cacheHolder.get().general); + // Set should not throw when using general cache + scriptService.cacheHolder.get().set(c, scriptService.contextCache(s, contexts.get(c))); + assertNull(scriptService.cacheHolder.get().contextCache); + assertEquals(generalRate, scriptService.cacheHolder.get().general.rate); + + scriptService.setCacheHolder(Settings.builder() + .put(SCRIPT_MAX_COMPILATIONS_RATE_SETTING.getConcreteSettingForNamespace(a).getKey(), aRate) + .put(SCRIPT_MAX_COMPILATIONS_RATE_SETTING.getConcreteSettingForNamespace(b).getKey(), bRate) + .put(SCRIPT_MAX_COMPILATIONS_RATE_SETTING.getConcreteSettingForNamespace(c).getKey(), + ScriptService.UNLIMITED_COMPILATION_RATE_KEY) + .build() + ); + + assertNull(scriptService.cacheHolder.get().general); assertNotNull(scriptService.cacheHolder.get().contextCache); - scriptService.cacheHolder.get().set(a, scriptService.contextCache(settings, contexts.get(a))); - scriptService.cacheHolder.get().set(b, scriptService.contextCache(settings, contexts.get(b))); - scriptService.cacheHolder.get().set(c, scriptService.contextCache(settings, contexts.get(c))); // get of missing context should be null assertNull(scriptService.cacheHolder.get().get( randomValueOtherThanMany(contexts.keySet()::contains, () -> randomAlphaOfLength(8))) @@ -443,6 +584,25 @@ public void testCacheHolderChangeSettings() throws IOException { contexts.get(b))); assertEquals(new ScriptCache.CompilationRate(aRate), scriptService.cacheHolder.get().contextCache.get(b).get().rate); + + scriptService.setCacheHolder(s); + assertNotNull(scriptService.cacheHolder.get().general); + assertNull(scriptService.cacheHolder.get().contextCache); + assertEquals(generalRate, scriptService.cacheHolder.get().general.rate); + + scriptService.setCacheHolder( + Settings.builder().put(SCRIPT_GENERAL_MAX_COMPILATIONS_RATE_SETTING.getKey(), bRate).build() + ); + + assertNotNull(scriptService.cacheHolder.get().general); + assertNull(scriptService.cacheHolder.get().contextCache); + assertEquals(new ScriptCache.CompilationRate(bRate), scriptService.cacheHolder.get().general.rate); + + ScriptService.CacheHolder holder = scriptService.cacheHolder.get(); + scriptService.setCacheHolder( + Settings.builder().put(SCRIPT_GENERAL_MAX_COMPILATIONS_RATE_SETTING.getKey(), bRate).build() + ); + assertEquals(holder, scriptService.cacheHolder.get()); } public void testFallbackToContextDefaults() throws IOException { @@ -451,20 +611,19 @@ public void testFallbackToContextDefaults() throws IOException { int contextCacheSize = randomIntBetween(1, 1024); TimeValue contextExpire = TimeValue.timeValueMinutes(randomIntBetween(10, 200)); - buildScriptService(Settings.EMPTY); + buildScriptService( + Settings.builder().put(SCRIPT_GENERAL_MAX_COMPILATIONS_RATE_SETTING.getKey(), "75/5m").build() + ); String name = "ingest"; // Use context specific - scriptService.cacheHolder.get().set( - name, - scriptService.contextCache(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) - .build(), - contexts.get(name) - )); + scriptService.setCacheHolder(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) + .build() + ); ScriptService.CacheHolder holder = scriptService.cacheHolder.get(); assertNotNull(holder.contextCache); diff --git a/server/src/test/java/org/elasticsearch/script/ScriptStatsTests.java b/server/src/test/java/org/elasticsearch/script/ScriptStatsTests.java index f7fe489a5fd38..f9e3e2355eecf 100644 --- a/server/src/test/java/org/elasticsearch/script/ScriptStatsTests.java +++ b/server/src/test/java/org/elasticsearch/script/ScriptStatsTests.java @@ -40,6 +40,7 @@ public void testXContent() throws IOException { stats.toXContent(builder, ToXContent.EMPTY_PARAMS); builder.endObject(); + /* TODO(stu): master only String expected = "{\n" + " \"script\" : {\n" + " \"compilations\" : 1100,\n" + @@ -71,6 +72,14 @@ public void testXContent() throws IOException { " ]\n" + " }\n" + "}"; + */ + String expected = "{\n" + + " \"script\" : {\n" + + " \"compilations\" : 1100,\n" + + " \"cache_evictions\" : 2211,\n" + + " \"compilation_limit_triggered\" : 3322\n" + + " }\n" + + "}"; assertThat(Strings.toString(builder), equalTo(expected)); } From 25ac8cef3b757dbbb0a8d622a48a753cccf75df2 Mon Sep 17 00:00:00 2001 From: Stuart Tettemer Date: Mon, 18 Oct 2021 21:54:40 -0500 Subject: [PATCH 2/7] Opt system contexts out of compilation rate limiting --- .../script/AbstractFieldScript.java | 44 +++++++------------ .../script/IngestConditionalScript.java | 5 +-- .../elasticsearch/script/IngestScript.java | 5 +-- .../org/elasticsearch/script/ScriptCache.java | 13 +++--- .../elasticsearch/script/ScriptContext.java | 25 ++++++++--- .../elasticsearch/script/ScriptService.java | 15 ++++--- .../elasticsearch/script/TemplateScript.java | 5 +-- .../script/ScriptCacheTests.java | 4 +- .../script/ScriptServiceTests.java | 36 ++++++++++----- .../elasticsearch/xpack/watcher/Watcher.java | 3 +- .../condition/WatcherConditionScript.java | 3 +- .../script/WatcherTransformScript.java | 3 +- 12 files changed, 82 insertions(+), 79 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/script/AbstractFieldScript.java b/server/src/main/java/org/elasticsearch/script/AbstractFieldScript.java index 2792ce48b94f2..ff2a92cfe8114 100644 --- a/server/src/main/java/org/elasticsearch/script/AbstractFieldScript.java +++ b/server/src/main/java/org/elasticsearch/script/AbstractFieldScript.java @@ -19,8 +19,6 @@ import java.util.Map; import java.util.function.Function; -import static org.elasticsearch.core.TimeValue.timeValueMillis; - /** * Abstract base for scripts to execute to build scripted fields. Inspired by * {@link AggregationScript} but hopefully with less historical baggage. @@ -32,33 +30,21 @@ public abstract class AbstractFieldScript extends DocBasedScript { public static final int MAX_VALUES = 100; static ScriptContext newContext(String name, Class factoryClass) { - return new ScriptContext<>( - name, - factoryClass, - /* - * We rely on the script cache in two ways: - * 1. It caches the "heavy" part of mappings generated at runtime. - * 2. Mapping updates tend to try to compile the script twice. Not - * for any good reason. They just do. - * Thus we use the default 100. - */ - 100, - timeValueMillis(0), - /* - * Disable compilation rate limits for runtime fields so we - * don't prevent mapping updates because we've performed too - * many recently. That'd just be lame. We also compile these - * scripts during search requests so this could totally be a - * source of runaway script compilations. We think folks will - * mostly reuse scripts though. - */ - ScriptCache.UNLIMITED_COMPILATION_RATE.asTuple(), - /* - * Disable runtime fields scripts from being allowed - * to be stored as part of the script meta data. - */ - false - ); + /* + * We rely on the script cache in two ways: + * 1. It caches the "heavy" part of mappings generated at runtime. + * 2. Mapping updates tend to try to compile the script twice. Not + * for any good reason. They just do. + * Thus we use the default cache size. + * + * We also disable compilation rate limits for runtime fields so we + * don't prevent mapping updates because we've performed too + * many recently. That'd just be lame. We also compile these + * scripts during search requests so this could totally be a + * source of runaway script compilations. We think folks will + * mostly reuse scripts though. + */ + return new ScriptContext<>(name, factoryClass, false); } private static final Map> PARAMS_FUNCTIONS = Map.of( diff --git a/server/src/main/java/org/elasticsearch/script/IngestConditionalScript.java b/server/src/main/java/org/elasticsearch/script/IngestConditionalScript.java index 430f6c22a116f..424f30e35973f 100644 --- a/server/src/main/java/org/elasticsearch/script/IngestConditionalScript.java +++ b/server/src/main/java/org/elasticsearch/script/IngestConditionalScript.java @@ -8,8 +8,6 @@ package org.elasticsearch.script; -import org.elasticsearch.core.TimeValue; - import java.util.Map; /** @@ -20,8 +18,7 @@ public abstract class IngestConditionalScript { public static final String[] PARAMETERS = { "ctx" }; /** The context used to compile {@link IngestConditionalScript} factories. */ - public static final ScriptContext CONTEXT = new ScriptContext<>("processor_conditional", Factory.class, - 200, TimeValue.timeValueMillis(0), ScriptCache.UNLIMITED_COMPILATION_RATE.asTuple(), true); + public static final ScriptContext CONTEXT = new ScriptContext<>("processor_conditional", Factory.class, true); /** The generic runtime parameters for the script. */ private final Map params; diff --git a/server/src/main/java/org/elasticsearch/script/IngestScript.java b/server/src/main/java/org/elasticsearch/script/IngestScript.java index a0fa0d9bbdde8..4e9a7aa35a3a7 100644 --- a/server/src/main/java/org/elasticsearch/script/IngestScript.java +++ b/server/src/main/java/org/elasticsearch/script/IngestScript.java @@ -9,8 +9,6 @@ package org.elasticsearch.script; -import org.elasticsearch.core.TimeValue; - import java.util.Map; /** @@ -21,8 +19,7 @@ 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, - 200, TimeValue.timeValueMillis(0), ScriptCache.UNLIMITED_COMPILATION_RATE.asTuple(), true); + public static final ScriptContext CONTEXT = new ScriptContext<>("ingest", Factory.class, true); /** The generic runtime parameters for the script. */ private final Map params; diff --git a/server/src/main/java/org/elasticsearch/script/ScriptCache.java b/server/src/main/java/org/elasticsearch/script/ScriptCache.java index 6e1ab496105da..b2fcdc9f24572 100644 --- a/server/src/main/java/org/elasticsearch/script/ScriptCache.java +++ b/server/src/main/java/org/elasticsearch/script/ScriptCache.java @@ -44,12 +44,7 @@ public class ScriptCache { private final double compilesAllowedPerNano; private final String contextRateSetting; - ScriptCache( - int cacheMaxSize, - TimeValue cacheExpire, - CompilationRate maxCompilationRate, - String contextRateSetting - ) { + ScriptCache(int cacheMaxSize, TimeValue cacheExpire, CompilationRate maxCompilationRate, String contextRateSetting) { this.cacheSize = cacheMaxSize; this.cacheExpire = cacheExpire; this.contextRateSetting = contextRateSetting; @@ -94,8 +89,10 @@ FactoryType compile( logger.trace("context [{}]: compiling script, type: [{}], lang: [{}], options: [{}]", context.name, type, lang, options); } - // Check whether too many compilations have happened - checkCompilationLimit(); + if (context.compilationRateLimited) { + // Check whether too many compilations have happened + checkCompilationLimit(); + } Object compiledScript = scriptEngine.compile(id, idOrCode, context, options); // Since the cache key is the script content itself we don't need to // invalidate/check the cache if an indexed script changes. diff --git a/server/src/main/java/org/elasticsearch/script/ScriptContext.java b/server/src/main/java/org/elasticsearch/script/ScriptContext.java index eb158f444096c..5a91864278a10 100644 --- a/server/src/main/java/org/elasticsearch/script/ScriptContext.java +++ b/server/src/main/java/org/elasticsearch/script/ScriptContext.java @@ -47,6 +47,8 @@ * be {@code boolean needs_score()}. */ public final class ScriptContext { + /** The default compilation rate limit for contexts with compilation rate limiting enabled */ + public static final Tuple DEFAULT_COMPILATION_RATE_LIMIT = new Tuple<>(150, TimeValue.timeValueMinutes(5)); /** A unique identifier for this context. */ public final String name; @@ -66,15 +68,15 @@ public final class ScriptContext { /** 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; + /** Is compilation rate limiting enabled for this context? */ + public final boolean compilationRateLimited; /** Determines if the script can be stored as part of the cluster state. */ public final boolean allowStoredScript; /** 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, boolean allowStoredScript) { + ScriptContext(String name, Class factoryClazz, int cacheSizeDefault, TimeValue cacheExpireDefault, + boolean compilationRateLimited, boolean allowStoredScript) { this.name = name; this.factoryClazz = factoryClazz; Method newInstanceMethod = findMethod("FactoryType", factoryClazz, "newInstance"); @@ -98,7 +100,7 @@ public ScriptContext(String name, Class factoryClazz, int cacheSize this.cacheSizeDefault = cacheSizeDefault; this.cacheExpireDefault = cacheExpireDefault; - this.maxCompilationRateDefault = maxCompilationRateDefault; + this.compilationRateLimited = compilationRateLimited; this.allowStoredScript = allowStoredScript; } @@ -106,7 +108,18 @@ public ScriptContext(String name, Class factoryClazz, int cacheSize * maxCompilationRateDefault and allow scripts of this context to be stored scripts */ 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)), true); + this(name, factoryClazz, 100, TimeValue.timeValueMillis(0), true, true); + } + + + /** + * Construct a context for a system script usage, using the: + * - default cache size (only relevant when context caching enabled): 200 + * - default cache expiration: no expiration + * - unlimited compilation rate + */ + public ScriptContext(String name, Class factoryClazz, boolean allowStoredScript) { + this(name, factoryClazz, 200, TimeValue.timeValueMillis(0), false, allowStoredScript); } /** 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 d760978b1d93e..b79c89150656a 100644 --- a/server/src/main/java/org/elasticsearch/script/ScriptService.java +++ b/server/src/main/java/org/elasticsearch/script/ScriptService.java @@ -231,7 +231,7 @@ void registerClusterSettingsListeners(ClusterSettings clusterSettings) { // Handle all settings for context and general caches, this flips between general and context caches. clusterSettings.addSettingsUpdateConsumer( - (settings) -> setCacheHolder(settings), + this::setCacheHolder, Arrays.asList(SCRIPT_GENERAL_MAX_COMPILATIONS_RATE_SETTING, SCRIPT_GENERAL_CACHE_EXPIRE_SETTING, SCRIPT_GENERAL_CACHE_SIZE_SETTING, @@ -565,8 +565,9 @@ void setCacheHolder(Settings settings) { } else if (current.general == null) { // Flipping to general cacheHolder.set(generalCacheHolder(settings)); - } else if (current.general.rate.equals(SCRIPT_GENERAL_MAX_COMPILATIONS_RATE_SETTING.get(settings)) == false) { - // General compilation rate changed, that setting is the only dynamically updated general setting + } else if (current.general.rate.equals(SCRIPT_GENERAL_MAX_COMPILATIONS_RATE_SETTING.get(settings)) == false || + current.general.cacheExpire.equals(SCRIPT_GENERAL_CACHE_EXPIRE_SETTING.get(settings)) == false || + current.general.cacheSize != SCRIPT_GENERAL_CACHE_SIZE_SETTING.get(settings)) { cacheHolder.set(generalCacheHolder(settings)); } } @@ -592,13 +593,15 @@ ScriptCache contextCache(Settings settings, ScriptContext context) { Setting rateSetting = SCRIPT_MAX_COMPILATIONS_RATE_SETTING.getConcreteSettingForNamespace(context.name); - ScriptCache.CompilationRate rate = null; - if (SCRIPT_DISABLE_MAX_COMPILATIONS_RATE_SETTING.get(settings) || compilationLimitsEnabled() == false) { + ScriptCache.CompilationRate rate; + if (SCRIPT_DISABLE_MAX_COMPILATIONS_RATE_SETTING.get(settings) + || compilationLimitsEnabled() == false + || context.compilationRateLimited == false) { rate = SCRIPT_COMPILATION_RATE_ZERO; } else if (rateSetting.existsOrFallbackExists(settings)) { rate = rateSetting.get(settings); } else { - rate = new ScriptCache.CompilationRate(context.maxCompilationRateDefault); + rate = new ScriptCache.CompilationRate(ScriptContext.DEFAULT_COMPILATION_RATE_LIMIT); } return new ScriptCache(cacheSize, cacheExpire, rate, rateSetting.getKey()); diff --git a/server/src/main/java/org/elasticsearch/script/TemplateScript.java b/server/src/main/java/org/elasticsearch/script/TemplateScript.java index d1777316434cf..ea7699ff3c883 100644 --- a/server/src/main/java/org/elasticsearch/script/TemplateScript.java +++ b/server/src/main/java/org/elasticsearch/script/TemplateScript.java @@ -8,8 +8,6 @@ package org.elasticsearch.script; -import org.elasticsearch.core.TimeValue; - import java.util.Map; /** @@ -41,6 +39,5 @@ public interface Factory { // Remove compilation rate limit for ingest. Ingest pipelines may use many mustache templates, triggering compilation // rate limiting. MustacheScriptEngine explicitly checks for TemplateScript. Rather than complicating the implementation there by // creating a new Script class (as would be customary), this context is used to avoid the default rate limit. - public static final ScriptContext INGEST_CONTEXT = new ScriptContext<>("ingest_template", Factory.class, - 200, TimeValue.timeValueMillis(0), ScriptCache.UNLIMITED_COMPILATION_RATE.asTuple(), true); + public static final ScriptContext INGEST_CONTEXT = new ScriptContext<>("ingest_template", Factory.class, true); } diff --git a/server/src/test/java/org/elasticsearch/script/ScriptCacheTests.java b/server/src/test/java/org/elasticsearch/script/ScriptCacheTests.java index 00c3c1267403c..efcbf07cd3f76 100644 --- a/server/src/test/java/org/elasticsearch/script/ScriptCacheTests.java +++ b/server/src/test/java/org/elasticsearch/script/ScriptCacheTests.java @@ -21,7 +21,7 @@ public class ScriptCacheTests extends ESTestCase { public void testCompilationCircuitBreaking() throws Exception { String context = randomFrom( ScriptModule.CORE_CONTEXTS.values().stream().filter( - c -> c.maxCompilationRateDefault.equals(ScriptCache.UNLIMITED_COMPILATION_RATE) == false + c -> c.compilationRateLimited ).collect(Collectors.toList()) ).name; final TimeValue expire = ScriptService.SCRIPT_CACHE_EXPIRE_SETTING.getConcreteSettingForNamespace(context).get(Settings.EMPTY); @@ -85,7 +85,7 @@ public void testGeneralCompilationCircuitBreaking() throws Exception { public void testUnlimitedCompilationRate() { String context = randomFrom( ScriptModule.CORE_CONTEXTS.values().stream().filter( - c -> c.maxCompilationRateDefault.equals(ScriptCache.UNLIMITED_COMPILATION_RATE) == false + c -> c.compilationRateLimited ).collect(Collectors.toList()) ).name; final Integer size = ScriptService.SCRIPT_CACHE_SIZE_SETTING.getConcreteSettingForNamespace(context).get(Settings.EMPTY); diff --git a/server/src/test/java/org/elasticsearch/script/ScriptServiceTests.java b/server/src/test/java/org/elasticsearch/script/ScriptServiceTests.java index 8e182e1b02353..293d0d4f5a63f 100644 --- a/server/src/test/java/org/elasticsearch/script/ScriptServiceTests.java +++ b/server/src/test/java/org/elasticsearch/script/ScriptServiceTests.java @@ -27,6 +27,7 @@ import java.io.IOException; import java.util.Collections; import java.util.HashMap; +import java.util.HashSet; import java.util.List; import java.util.Map; import java.util.Set; @@ -40,6 +41,7 @@ import static org.elasticsearch.script.ScriptService.SCRIPT_GENERAL_MAX_COMPILATIONS_RATE_SETTING; import static org.elasticsearch.script.ScriptService.SCRIPT_GENERAL_CACHE_SIZE_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; @@ -53,6 +55,7 @@ public class ScriptServiceTests extends ESTestCase { private ScriptService scriptService; private Settings baseSettings; private ClusterSettings clusterSettings; + private Map> rateLimitedContexts; @Before public void setup() throws IOException { @@ -71,6 +74,7 @@ public void setup() throws IOException { engines.put(scriptEngine.getType(), scriptEngine); engines.put("test", new MockScriptEngine("test", scripts, Collections.emptyMap())); logger.info("--> setup script service"); + rateLimitedContexts = compilationRateLimitedContexts(); } private void buildScriptService(Settings additionalSettings) throws IOException { @@ -252,9 +256,9 @@ public void testCacheEvictionCountedInCacheEvictionsStats() throws IOException { } public void testContextCacheStats() throws IOException { - ScriptContext contextA = randomFrom(contexts.values()); + ScriptContext contextA = randomFrom(rateLimitedContexts.values()); String aRate = "2/10m"; - ScriptContext contextB = randomValueOtherThan(contextA, () -> randomFrom(contexts.values())); + ScriptContext contextB = randomValueOtherThan(contextA, () -> randomFrom(rateLimitedContexts.values())); String bRate = "3/10m"; BiFunction msg = (rate, ctx) -> ( "[script] Too many dynamic script compilations within, max: [" + rate + @@ -262,6 +266,7 @@ public void testContextCacheStats() throws IOException { ".max_compilations_rate] setting" ); buildScriptService(Settings.builder() + .put(SCRIPT_GENERAL_MAX_COMPILATIONS_RATE_SETTING.getKey(), USE_CONTEXT_RATE_KEY) .put(SCRIPT_CACHE_SIZE_SETTING.getConcreteSettingForNamespace(contextA.name).getKey(), 1) .put(SCRIPT_MAX_COMPILATIONS_RATE_SETTING.getConcreteSettingForNamespace(contextA.name).getKey(), aRate) .put(SCRIPT_CACHE_SIZE_SETTING.getConcreteSettingForNamespace(contextB.name).getKey(), 2) @@ -468,8 +473,8 @@ public void testCacheHolderGeneralConstructor() throws IOException { } public void testCacheHolderContextConstructor() throws IOException { - String a = randomFrom(contexts.keySet()); - String b = randomValueOtherThan(a, () -> randomFrom(contexts.keySet())); + String a = randomFrom(rateLimitedContexts.keySet()); + String b = randomValueOtherThan(a, () -> randomFrom(rateLimitedContexts.keySet())); String aCompilationRate = "77/5m"; String bCompilationRate = "78/6m"; @@ -535,7 +540,8 @@ public void testDisableCompilationRateSetting() throws IOException { } public void testCacheHolderChangeSettings() throws IOException { - Set contextNames = contexts.keySet(); + Set contextNames = contexts.entrySet().stream().filter(e -> e.getValue().compilationRateLimited) + .map(Map.Entry::getKey).collect(Collectors.toSet()); String a = randomFrom(contextNames); String aRate = "77/5m"; String b = randomValueOtherThan(a, () -> randomFrom(contextNames)); @@ -615,7 +621,7 @@ public void testFallbackToContextDefaults() throws IOException { Settings.builder().put(SCRIPT_GENERAL_MAX_COMPILATIONS_RATE_SETTING.getKey(), "75/5m").build() ); - String name = "ingest"; + String name = "score"; // Use context specific scriptService.setCacheHolder(Settings.builder() @@ -634,7 +640,7 @@ public void testFallbackToContextDefaults() throws IOException { assertEquals(contextCacheSize, holder.contextCache.get(name).get().cacheSize); assertEquals(contextExpire, holder.contextCache.get(name).get().cacheExpire); - ScriptContext ingest = contexts.get(name); + ScriptContext score = contexts.get(name); // Fallback to context defaults buildScriptService(Settings.EMPTY); @@ -643,9 +649,19 @@ public void testFallbackToContextDefaults() throws IOException { assertNotNull(holder.contextCache.get(name)); assertNotNull(holder.contextCache.get(name).get()); - assertEquals(ingest.maxCompilationRateDefault, holder.contextCache.get(name).get().rate.asTuple()); - assertEquals(ingest.cacheSizeDefault, holder.contextCache.get(name).get().cacheSize); - assertEquals(ingest.cacheExpireDefault, holder.contextCache.get(name).get().cacheExpire); + assertEquals(ScriptContext.DEFAULT_COMPILATION_RATE_LIMIT, holder.contextCache.get(name).get().rate.asTuple()); + assertEquals(score.cacheSizeDefault, holder.contextCache.get(name).get().cacheSize); + assertEquals(score.cacheExpireDefault, holder.contextCache.get(name).get().cacheExpire); + } + + protected HashMap> compilationRateLimitedContexts() { + HashMap> rateLimited = new HashMap<>(); + for (Map.Entry> entry: contexts.entrySet()) { + if (entry.getValue().compilationRateLimited) { + rateLimited.put(entry.getKey(), entry.getValue()); + } + } + return rateLimited; } private void assertCompileRejected(String lang, String script, ScriptType scriptType, ScriptContext scriptContext) { diff --git a/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/Watcher.java b/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/Watcher.java index 5d31bb245649d..1d3f102b72238 100644 --- a/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/Watcher.java +++ b/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/Watcher.java @@ -232,8 +232,7 @@ public class Watcher extends Plugin implements SystemIndexPlugin, ScriptPlugin, new ByteSizeValue(1, ByteSizeUnit.MB), new ByteSizeValue(10, ByteSizeUnit.MB), NodeScope); public static final ScriptContext SCRIPT_TEMPLATE_CONTEXT - = new ScriptContext<>("xpack_template", TemplateScript.Factory.class, - 200, TimeValue.timeValueMillis(0), ScriptCache.UNLIMITED_COMPILATION_RATE.asTuple(), true); + = new ScriptContext<>("xpack_template", TemplateScript.Factory.class, true); private static final Logger logger = LogManager.getLogger(Watcher.class); private WatcherIndexingListener listener; diff --git a/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/condition/WatcherConditionScript.java b/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/condition/WatcherConditionScript.java index bc38efd0c6572..c0075398e33bc 100644 --- a/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/condition/WatcherConditionScript.java +++ b/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/condition/WatcherConditionScript.java @@ -44,6 +44,5 @@ public interface Factory { WatcherConditionScript newInstance(Map params, WatchExecutionContext watcherContext); } - public static ScriptContext CONTEXT = new ScriptContext<>("watcher_condition", Factory.class, - 200, TimeValue.timeValueMillis(0), ScriptCache.UNLIMITED_COMPILATION_RATE.asTuple(), true); + public static ScriptContext CONTEXT = new ScriptContext<>("watcher_condition", Factory.class, true); } diff --git a/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/transform/script/WatcherTransformScript.java b/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/transform/script/WatcherTransformScript.java index 465f87cc26bd8..9ca48fc30ae95 100644 --- a/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/transform/script/WatcherTransformScript.java +++ b/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/transform/script/WatcherTransformScript.java @@ -45,6 +45,5 @@ public interface Factory { WatcherTransformScript newInstance(Map params, WatchExecutionContext watcherContext, Payload payload); } - public static ScriptContext CONTEXT = new ScriptContext<>("watcher_transform", Factory.class, - 200, TimeValue.timeValueMillis(0), ScriptCache.UNLIMITED_COMPILATION_RATE.asTuple(), true); + public static ScriptContext CONTEXT = new ScriptContext<>("watcher_transform", Factory.class, true); } From 2d03b6d19b8c36c8413f1eb3973eb48685949f6b Mon Sep 17 00:00:00 2001 From: Stuart Tettemer Date: Mon, 18 Oct 2021 22:02:05 -0500 Subject: [PATCH 3/7] Unused import --- .../test/java/org/elasticsearch/script/ScriptServiceTests.java | 1 - 1 file changed, 1 deletion(-) diff --git a/server/src/test/java/org/elasticsearch/script/ScriptServiceTests.java b/server/src/test/java/org/elasticsearch/script/ScriptServiceTests.java index 293d0d4f5a63f..0ea0760d46dae 100644 --- a/server/src/test/java/org/elasticsearch/script/ScriptServiceTests.java +++ b/server/src/test/java/org/elasticsearch/script/ScriptServiceTests.java @@ -27,7 +27,6 @@ import java.io.IOException; import java.util.Collections; import java.util.HashMap; -import java.util.HashSet; import java.util.List; import java.util.Map; import java.util.Set; From 5bb85b52a1a7637bfed43e424a86decb56123b25 Mon Sep 17 00:00:00 2001 From: Stuart Tettemer Date: Mon, 18 Oct 2021 22:05:15 -0500 Subject: [PATCH 4/7] Unused imports in xpack --- .../src/main/java/org/elasticsearch/xpack/watcher/Watcher.java | 1 - .../xpack/watcher/condition/WatcherConditionScript.java | 2 -- .../xpack/watcher/transform/script/WatcherTransformScript.java | 2 -- 3 files changed, 5 deletions(-) diff --git a/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/Watcher.java b/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/Watcher.java index 1d3f102b72238..17b9a7cd1d05c 100644 --- a/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/Watcher.java +++ b/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/Watcher.java @@ -51,7 +51,6 @@ import org.elasticsearch.repositories.RepositoriesService; import org.elasticsearch.rest.RestController; import org.elasticsearch.rest.RestHandler; -import org.elasticsearch.script.ScriptCache; import org.elasticsearch.script.ScriptContext; import org.elasticsearch.script.ScriptService; import org.elasticsearch.script.TemplateScript; diff --git a/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/condition/WatcherConditionScript.java b/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/condition/WatcherConditionScript.java index c0075398e33bc..bd6446b82cb9c 100644 --- a/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/condition/WatcherConditionScript.java +++ b/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/condition/WatcherConditionScript.java @@ -6,8 +6,6 @@ */ package org.elasticsearch.xpack.watcher.condition; -import org.elasticsearch.core.TimeValue; -import org.elasticsearch.script.ScriptCache; import org.elasticsearch.script.ScriptContext; import org.elasticsearch.xpack.core.watcher.execution.WatchExecutionContext; import org.elasticsearch.xpack.watcher.support.Variables; diff --git a/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/transform/script/WatcherTransformScript.java b/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/transform/script/WatcherTransformScript.java index 9ca48fc30ae95..e661ea56711b4 100644 --- a/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/transform/script/WatcherTransformScript.java +++ b/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/transform/script/WatcherTransformScript.java @@ -6,8 +6,6 @@ */ package org.elasticsearch.xpack.watcher.transform.script; -import org.elasticsearch.core.TimeValue; -import org.elasticsearch.script.ScriptCache; import org.elasticsearch.script.ScriptContext; import org.elasticsearch.xpack.core.watcher.execution.WatchExecutionContext; import org.elasticsearch.xpack.core.watcher.watch.Payload; From 147a1ffc4367b675c4e5bf78f43746c9f4195439 Mon Sep 17 00:00:00 2001 From: Stuart Tettemer Date: Mon, 18 Oct 2021 22:13:00 -0500 Subject: [PATCH 5/7] Comment and whitespace cleanup --- .../main/java/org/elasticsearch/script/ScriptCacheStats.java | 2 +- .../src/main/java/org/elasticsearch/script/ScriptContext.java | 1 - 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/script/ScriptCacheStats.java b/server/src/main/java/org/elasticsearch/script/ScriptCacheStats.java index d580f94a65a68..28183c1d46308 100644 --- a/server/src/main/java/org/elasticsearch/script/ScriptCacheStats.java +++ b/server/src/main/java/org/elasticsearch/script/ScriptCacheStats.java @@ -21,7 +21,7 @@ import java.util.Objects; import java.util.stream.Collectors; -// This class is deprecated in favor of ScriptStats and ScriptContextStats. It is removed in 8. +// This class is deprecated in favor of ScriptStats and ScriptContextStats public class ScriptCacheStats implements Writeable, ToXContentFragment { private final Map context; private final ScriptStats general; diff --git a/server/src/main/java/org/elasticsearch/script/ScriptContext.java b/server/src/main/java/org/elasticsearch/script/ScriptContext.java index 5a91864278a10..aafee11d82b07 100644 --- a/server/src/main/java/org/elasticsearch/script/ScriptContext.java +++ b/server/src/main/java/org/elasticsearch/script/ScriptContext.java @@ -111,7 +111,6 @@ public ScriptContext(String name, Class factoryClazz) { this(name, factoryClazz, 100, TimeValue.timeValueMillis(0), true, true); } - /** * Construct a context for a system script usage, using the: * - default cache size (only relevant when context caching enabled): 200 From 7ac581ea7940089170ddf8d105138edac3440a13 Mon Sep 17 00:00:00 2001 From: Stuart Tettemer Date: Tue, 19 Oct 2021 09:04:32 -0500 Subject: [PATCH 6/7] Restore node stats --- .../admin/cluster/node/stats/NodeStats.java | 15 +++++++++ .../org/elasticsearch/node/NodeService.java | 1 + .../cluster/node/stats/NodeStatsTests.java | 32 ++++++++++++++++++- .../elasticsearch/cluster/DiskUsageTests.java | 18 +++++++---- .../MockInternalClusterInfoService.java | 3 +- .../AutoscalingMemoryInfoServiceTests.java | 1 + ...chineLearningInfoTransportActionTests.java | 2 +- ...sportGetTrainedModelsStatsActionTests.java | 2 +- .../node/NodeStatsMonitoringDocTests.java | 3 +- 9 files changed, 66 insertions(+), 11 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/action/admin/cluster/node/stats/NodeStats.java b/server/src/main/java/org/elasticsearch/action/admin/cluster/node/stats/NodeStats.java index 7c17367f9ecbe..855ed0c8676c2 100644 --- a/server/src/main/java/org/elasticsearch/action/admin/cluster/node/stats/NodeStats.java +++ b/server/src/main/java/org/elasticsearch/action/admin/cluster/node/stats/NodeStats.java @@ -27,6 +27,7 @@ import org.elasticsearch.monitor.os.OsStats; import org.elasticsearch.monitor.process.ProcessStats; import org.elasticsearch.node.AdaptiveSelectionStats; +import org.elasticsearch.script.ScriptCacheStats; import org.elasticsearch.script.ScriptStats; import org.elasticsearch.threadpool.ThreadPoolStats; import org.elasticsearch.transport.TransportStats; @@ -71,6 +72,9 @@ public class NodeStats extends BaseNodeResponse implements ToXContentFragment { @Nullable private ScriptStats scriptStats; + @Nullable + private ScriptCacheStats scriptCacheStats; + @Nullable private DiscoveryStats discoveryStats; @@ -98,6 +102,7 @@ public NodeStats(StreamInput in) throws IOException { http = in.readOptionalWriteable(HttpStats::new); breaker = in.readOptionalWriteable(AllCircuitBreakerStats::new); scriptStats = in.readOptionalWriteable(ScriptStats::new); + scriptCacheStats = scriptStats != null ? scriptStats.toScriptCacheStats() : null; discoveryStats = in.readOptionalWriteable(DiscoveryStats::new); ingestStats = in.readOptionalWriteable(IngestStats::new); adaptiveSelectionStats = in.readOptionalWriteable(AdaptiveSelectionStats::new); @@ -112,6 +117,7 @@ public NodeStats(DiscoveryNode node, long timestamp, @Nullable NodeIndicesStats @Nullable DiscoveryStats discoveryStats, @Nullable IngestStats ingestStats, @Nullable AdaptiveSelectionStats adaptiveSelectionStats, + @Nullable ScriptCacheStats scriptCacheStats, @Nullable IndexingPressureStats indexingPressureStats) { super(node); this.timestamp = timestamp; @@ -128,6 +134,7 @@ public NodeStats(DiscoveryNode node, long timestamp, @Nullable NodeIndicesStats this.discoveryStats = discoveryStats; this.ingestStats = ingestStats; this.adaptiveSelectionStats = adaptiveSelectionStats; + this.scriptCacheStats = scriptCacheStats; this.indexingPressureStats = indexingPressureStats; } @@ -223,6 +230,11 @@ public AdaptiveSelectionStats getAdaptiveSelectionStats() { return adaptiveSelectionStats; } + @Nullable + public ScriptCacheStats getScriptCacheStats() { + return scriptCacheStats; + } + @Nullable public IndexingPressureStats getIndexingPressureStats() { return indexingPressureStats; @@ -314,6 +326,9 @@ public XContentBuilder toXContent(XContentBuilder builder, Params params) throws if (getAdaptiveSelectionStats() != null) { getAdaptiveSelectionStats().toXContent(builder, params); } + if (getScriptCacheStats() != null) { + getScriptCacheStats().toXContent(builder, params); + } if (getIndexingPressureStats() != null) { getIndexingPressureStats().toXContent(builder, params); } diff --git a/server/src/main/java/org/elasticsearch/node/NodeService.java b/server/src/main/java/org/elasticsearch/node/NodeService.java index 590a64ccd7b60..305d0aa995835 100644 --- a/server/src/main/java/org/elasticsearch/node/NodeService.java +++ b/server/src/main/java/org/elasticsearch/node/NodeService.java @@ -118,6 +118,7 @@ public NodeStats stats(CommonStatsFlags indices, boolean os, boolean process, bo discoveryStats ? coordinator.stats() : null, ingest ? ingestService.stats() : null, adaptiveSelection ? responseCollectorService.getAdaptiveStats(searchTransportService.getPendingSearchRequests()) : null, + scriptCache ? scriptService.cacheStats() : null, indexingPressure ? this.indexingPressure.stats() : null); } diff --git a/server/src/test/java/org/elasticsearch/action/admin/cluster/node/stats/NodeStatsTests.java b/server/src/test/java/org/elasticsearch/action/admin/cluster/node/stats/NodeStatsTests.java index 4272dfa77f2cc..39e197d7a0f96 100644 --- a/server/src/test/java/org/elasticsearch/action/admin/cluster/node/stats/NodeStatsTests.java +++ b/server/src/test/java/org/elasticsearch/action/admin/cluster/node/stats/NodeStatsTests.java @@ -29,6 +29,7 @@ import org.elasticsearch.monitor.process.ProcessStats; import org.elasticsearch.node.AdaptiveSelectionStats; import org.elasticsearch.node.ResponseCollectorService; +import org.elasticsearch.script.ScriptCacheStats; import org.elasticsearch.script.ScriptContextStats; import org.elasticsearch.script.ScriptStats; import org.elasticsearch.test.ESTestCase; @@ -418,6 +419,34 @@ public void testSerialization() throws IOException { assertEquals(aStats.responseTime, bStats.responseTime, 0.01); }); } + ScriptCacheStats scriptCacheStats = nodeStats.getScriptCacheStats(); + ScriptCacheStats deserializedScriptCacheStats = deserializedNodeStats.getScriptCacheStats(); + if (scriptCacheStats == null) { + assertNull(deserializedScriptCacheStats); + } else if (deserializedScriptCacheStats.getContextStats() != null) { + Map deserialized = deserializedScriptCacheStats.getContextStats(); + long evictions = 0; + long limited = 0; + long compilations = 0; + Map stats = scriptCacheStats.getContextStats(); + for (String context: stats.keySet()) { + ScriptStats deserStats = deserialized.get(context); + ScriptStats generatedStats = stats.get(context); + + evictions += generatedStats.getCacheEvictions(); + assertEquals(generatedStats.getCacheEvictions(), deserStats.getCacheEvictions()); + + limited += generatedStats.getCompilationLimitTriggered(); + assertEquals(generatedStats.getCompilationLimitTriggered(), deserStats.getCompilationLimitTriggered()); + + compilations += generatedStats.getCompilations(); + assertEquals(generatedStats.getCompilations(), deserStats.getCompilations()); + } + ScriptStats sum = deserializedScriptCacheStats.sum(); + assertEquals(evictions, sum.getCacheEvictions()); + assertEquals(limited, sum.getCompilationLimitTriggered()); + assertEquals(compilations, sum.getCompilations()); + } } } } @@ -688,10 +717,11 @@ public static NodeStats createNodeStats() { } adaptiveSelectionStats = new AdaptiveSelectionStats(nodeConnections, nodeStats); } + ScriptCacheStats scriptCacheStats = scriptStats != null ? scriptStats.toScriptCacheStats() : null; //TODO NodeIndicesStats are not tested here, way too complicated to create, also they need to be migrated to Writeable yet return new NodeStats(node, randomNonNegativeLong(), null, osStats, processStats, jvmStats, threadPoolStats, fsInfo, transportStats, httpStats, allCircuitBreakerStats, scriptStats, discoveryStats, - ingestStats, adaptiveSelectionStats, null); + ingestStats, adaptiveSelectionStats, scriptCacheStats, null); } private static ScriptContextStats.TimeSeries randomTimeSeries() { diff --git a/server/src/test/java/org/elasticsearch/cluster/DiskUsageTests.java b/server/src/test/java/org/elasticsearch/cluster/DiskUsageTests.java index 61466be75cf99..dc89a392c1e25 100644 --- a/server/src/test/java/org/elasticsearch/cluster/DiskUsageTests.java +++ b/server/src/test/java/org/elasticsearch/cluster/DiskUsageTests.java @@ -151,11 +151,14 @@ public void testFillDiskUsage() { }; List nodeStats = Arrays.asList( new NodeStats(new DiscoveryNode("node_1", buildNewFakeTransportAddress(), emptyMap(), emptySet(), Version.CURRENT), 0, - null,null,null,null,null,new FsInfo(0, null, node1FSInfo), null,null,null,null,null, null, null, null), + null,null,null,null,null,new FsInfo(0, null, node1FSInfo), null,null,null,null,null, null, null, + null, null), new NodeStats(new DiscoveryNode("node_2", buildNewFakeTransportAddress(), emptyMap(), emptySet(), Version.CURRENT), 0, - null,null,null,null,null, new FsInfo(0, null, node2FSInfo), null,null,null,null,null, null, null, null), + null,null,null,null,null, new FsInfo(0, null, node2FSInfo), null,null,null,null,null, null, null, + null, null), new NodeStats(new DiscoveryNode("node_3", buildNewFakeTransportAddress(), emptyMap(), emptySet(), Version.CURRENT), 0, - null,null,null,null,null, new FsInfo(0, null, node3FSInfo), null,null,null,null,null, null, null, null) + null,null,null,null,null, new FsInfo(0, null, node3FSInfo), null,null,null,null,null, null, null, + null, null) ); InternalClusterInfoService.fillDiskUsagePerNode(nodeStats, newLeastAvaiableUsages, newMostAvaiableUsages); DiskUsage leastNode_1 = newLeastAvaiableUsages.get("node_1"); @@ -192,11 +195,14 @@ public void testFillDiskUsageSomeInvalidValues() { }; List nodeStats = Arrays.asList( new NodeStats(new DiscoveryNode("node_1", buildNewFakeTransportAddress(), emptyMap(), emptySet(), Version.CURRENT), 0, - null,null,null,null,null,new FsInfo(0, null, node1FSInfo), null,null,null,null,null, null, null, null), + null,null,null,null,null,new FsInfo(0, null, node1FSInfo), null,null,null,null,null, null, null, + null, null), new NodeStats(new DiscoveryNode("node_2", buildNewFakeTransportAddress(), emptyMap(), emptySet(), Version.CURRENT), 0, - null,null,null,null,null, new FsInfo(0, null, node2FSInfo), null,null,null,null,null, null, null, null), + null,null,null,null,null, new FsInfo(0, null, node2FSInfo), null,null,null,null,null, null, null, + null, null), new NodeStats(new DiscoveryNode("node_3", buildNewFakeTransportAddress(), emptyMap(), emptySet(), Version.CURRENT), 0, - null,null,null,null,null, new FsInfo(0, null, node3FSInfo), null,null,null,null,null, null, null, null) + null,null,null,null,null, new FsInfo(0, null, node3FSInfo), null,null,null,null,null, null, null, + null, null) ); InternalClusterInfoService.fillDiskUsagePerNode(nodeStats, newLeastAvailableUsages, newMostAvailableUsages); DiskUsage leastNode_1 = newLeastAvailableUsages.get("node_1"); diff --git a/test/framework/src/main/java/org/elasticsearch/cluster/MockInternalClusterInfoService.java b/test/framework/src/main/java/org/elasticsearch/cluster/MockInternalClusterInfoService.java index 100b6611226ed..fc71839b93209 100644 --- a/test/framework/src/main/java/org/elasticsearch/cluster/MockInternalClusterInfoService.java +++ b/test/framework/src/main/java/org/elasticsearch/cluster/MockInternalClusterInfoService.java @@ -74,7 +74,8 @@ List adjustNodesStats(List nodesStats) { .map(fsInfoPath -> diskUsageFunction.apply(discoveryNode, fsInfoPath)) .toArray(FsInfo.Path[]::new)), nodeStats.getTransport(), nodeStats.getHttp(), nodeStats.getBreaker(), nodeStats.getScriptStats(), nodeStats.getDiscoveryStats(), - nodeStats.getIngestStats(), nodeStats.getAdaptiveSelectionStats(), nodeStats.getIndexingPressureStats()); + nodeStats.getIngestStats(), nodeStats.getAdaptiveSelectionStats(), nodeStats.getScriptCacheStats(), + nodeStats.getIndexingPressureStats()); }).collect(Collectors.toList()); } diff --git a/x-pack/plugin/autoscaling/src/test/java/org/elasticsearch/xpack/autoscaling/capacity/memory/AutoscalingMemoryInfoServiceTests.java b/x-pack/plugin/autoscaling/src/test/java/org/elasticsearch/xpack/autoscaling/capacity/memory/AutoscalingMemoryInfoServiceTests.java index fb0a073c02e86..1c005095854bd 100644 --- a/x-pack/plugin/autoscaling/src/test/java/org/elasticsearch/xpack/autoscaling/capacity/memory/AutoscalingMemoryInfoServiceTests.java +++ b/x-pack/plugin/autoscaling/src/test/java/org/elasticsearch/xpack/autoscaling/capacity/memory/AutoscalingMemoryInfoServiceTests.java @@ -367,6 +367,7 @@ private static NodeStats statsForNode(DiscoveryNode node, long memory) { null, null, null, + null, null ); } diff --git a/x-pack/plugin/ml/src/test/java/org/elasticsearch/xpack/ml/MachineLearningInfoTransportActionTests.java b/x-pack/plugin/ml/src/test/java/org/elasticsearch/xpack/ml/MachineLearningInfoTransportActionTests.java index f00da807175a8..feb436317a982 100644 --- a/x-pack/plugin/ml/src/test/java/org/elasticsearch/xpack/ml/MachineLearningInfoTransportActionTests.java +++ b/x-pack/plugin/ml/src/test/java/org/elasticsearch/xpack/ml/MachineLearningInfoTransportActionTests.java @@ -687,7 +687,7 @@ private static NodeStats buildNodeStats(List pipelineNames, List Date: Wed, 20 Oct 2021 09:39:40 -0500 Subject: [PATCH 7/7] Script: Restore the scripting general cache Deprecate the script context cache in favor of the general cache. Users should use the following settings: `script.max_compilations_rate` to set the max compilation rate for user scripts such as filter scripts. Certain script contexts that submit scripts outside of the control of the user are exempted from this rate limit. Examples include runtime fields, ingest and watcher. `script.cache.max_size` to set the max size of the cache. `script.cache.expire` to set the expiration time for entries in the cache. Whats deprecated? `script.max_compilations_rate: use-context`. This special setting value was used to turn on the script context-specific caches. `script.context.$CONTEXT.cache_max_size`, use `script.cache.max_size` instead. `script.context.$CONTEXT.cache_expire`, use `script.cache.expire` instead. `script.context.$CONTEXT.max_compilations_rate`, use `script.max_compilations_rate` instead. The default cache size was increased from `100` to `3000`, which was approximately the max cache size when using context-specific caches. The default compilation rate limit was increased from `75/5m` to `150/5m` to account for increasing uses of scripts. System script contexts can now opt-out of compilation rate limiting using a flag rather than a sentinel rate limit value. 7.16: Script: Deprecate script context cache #79508 Refs: #62899 7.16: Script: Opt-out system contexts from script compilation rate limit #79459 Refs: #62899 --- .../modules/indices/circuit_breaker.asciidoc | 6 +- docs/reference/scripting/using.asciidoc | 12 +- .../script/AbstractFieldScript.java | 44 ++-- .../script/IngestConditionalScript.java | 5 +- .../elasticsearch/script/IngestScript.java | 5 +- .../elasticsearch/script/ScriptContext.java | 16 +- .../elasticsearch/script/ScriptService.java | 33 ++- .../elasticsearch/script/TemplateScript.java | 5 +- .../script/ScriptServiceTests.java | 195 ++++++++++++++---- .../test/InternalTestCluster.java | 9 +- .../elasticsearch/xpack/watcher/Watcher.java | 3 +- .../condition/WatcherConditionScript.java | 4 +- .../script/WatcherTransformScript.java | 4 +- 13 files changed, 246 insertions(+), 95 deletions(-) diff --git a/docs/reference/modules/indices/circuit_breaker.asciidoc b/docs/reference/modules/indices/circuit_breaker.asciidoc index b128ce5dfb371..161f3a8876f18 100644 --- a/docs/reference/modules/indices/circuit_breaker.asciidoc +++ b/docs/reference/modules/indices/circuit_breaker.asciidoc @@ -126,11 +126,11 @@ within a period of time. See the "prefer-parameters" section of the <> documentation for more information. -`script.context.$CONTEXT.max_compilations_rate`:: +`script.max_compilations_rate`:: (<>) Limit for the number of unique dynamic scripts within a certain interval - that are allowed to be compiled for a given context. Defaults to `75/5m`, - meaning 75 every 5 minutes. + that are allowed to be compiled. Defaults to `150/5m`, + meaning 150 every 5 minutes. [[regex-circuit-breaker]] [discrete] diff --git a/docs/reference/scripting/using.asciidoc b/docs/reference/scripting/using.asciidoc index f49be226c2d37..e1eaeaaedad93 100644 --- a/docs/reference/scripting/using.asciidoc +++ b/docs/reference/scripting/using.asciidoc @@ -120,12 +120,8 @@ the `multiplier` parameter without {es} recompiling the script. } ---- -For most contexts, you can compile up to 75 scripts per 5 minutes by default. -For ingest contexts, the default script compilation rate is unlimited. You -can change these settings dynamically by setting -`script.context.$CONTEXT.max_compilations_rate`. For example, the following -setting limits script compilation to 100 scripts every 10 minutes for the -{painless}/painless-field-context.html[field context]: +You can compile up to 150 scripts per 5 minutes by default. +For ingest contexts, the default script compilation rate is unlimited. [source,js] ---- @@ -406,8 +402,8 @@ small. All scripts are cached by default so that they only need to be recompiled when updates occur. By default, scripts do not have a time-based expiration. -You can change this behavior by using the `script.context.$CONTEXT.cache_expire` setting. -Use the `script.context.$CONTEXT.cache_max_size` setting to configure the size of the cache. +You can change this behavior by using the `script.cache.expire` setting. +Use the `script.cache.max_size` setting to configure the size of the cache. NOTE: The size of scripts is limited to 65,535 bytes. Set the value of `script.max_size_in_bytes` to increase that soft limit. If your scripts are really large, then consider using a diff --git a/server/src/main/java/org/elasticsearch/script/AbstractFieldScript.java b/server/src/main/java/org/elasticsearch/script/AbstractFieldScript.java index ff2a92cfe8114..7cd479a388289 100644 --- a/server/src/main/java/org/elasticsearch/script/AbstractFieldScript.java +++ b/server/src/main/java/org/elasticsearch/script/AbstractFieldScript.java @@ -19,6 +19,8 @@ import java.util.Map; import java.util.function.Function; +import static org.elasticsearch.core.TimeValue.timeValueMillis; + /** * Abstract base for scripts to execute to build scripted fields. Inspired by * {@link AggregationScript} but hopefully with less historical baggage. @@ -30,21 +32,33 @@ public abstract class AbstractFieldScript extends DocBasedScript { public static final int MAX_VALUES = 100; static ScriptContext newContext(String name, Class factoryClass) { - /* - * We rely on the script cache in two ways: - * 1. It caches the "heavy" part of mappings generated at runtime. - * 2. Mapping updates tend to try to compile the script twice. Not - * for any good reason. They just do. - * Thus we use the default cache size. - * - * We also disable compilation rate limits for runtime fields so we - * don't prevent mapping updates because we've performed too - * many recently. That'd just be lame. We also compile these - * scripts during search requests so this could totally be a - * source of runaway script compilations. We think folks will - * mostly reuse scripts though. - */ - return new ScriptContext<>(name, factoryClass, false); + return new ScriptContext<>( + name, + factoryClass, + /* + * We rely on the script cache in two ways: + * 1. It caches the "heavy" part of mappings generated at runtime. + * 2. Mapping updates tend to try to compile the script twice. Not + * for any good reason. They just do. + * Thus we use the default 100. + */ + 100, + timeValueMillis(0), + /* + * Disable compilation rate limits for runtime fields so we + * don't prevent mapping updates because we've performed too + * many recently. That'd just be lame. We also compile these + * scripts during search requests so this could totally be a + * source of runaway script compilations. We think folks will + * mostly reuse scripts though. + */ + false, + /* + * Disable runtime fields scripts from being allowed + * to be stored as part of the script meta data. + */ + false + ); } private static final Map> PARAMS_FUNCTIONS = Map.of( diff --git a/server/src/main/java/org/elasticsearch/script/IngestConditionalScript.java b/server/src/main/java/org/elasticsearch/script/IngestConditionalScript.java index 424f30e35973f..caa6cbbe0164b 100644 --- a/server/src/main/java/org/elasticsearch/script/IngestConditionalScript.java +++ b/server/src/main/java/org/elasticsearch/script/IngestConditionalScript.java @@ -8,6 +8,8 @@ package org.elasticsearch.script; +import org.elasticsearch.core.TimeValue; + import java.util.Map; /** @@ -18,7 +20,8 @@ public abstract class IngestConditionalScript { public static final String[] PARAMETERS = { "ctx" }; /** The context used to compile {@link IngestConditionalScript} factories. */ - public static final ScriptContext CONTEXT = new ScriptContext<>("processor_conditional", Factory.class, true); + public static final ScriptContext CONTEXT = new ScriptContext<>("processor_conditional", Factory.class, + 200, TimeValue.timeValueMillis(0), false, true); /** The generic runtime parameters for the script. */ private final Map params; diff --git a/server/src/main/java/org/elasticsearch/script/IngestScript.java b/server/src/main/java/org/elasticsearch/script/IngestScript.java index 4e9a7aa35a3a7..bb444b132d1d0 100644 --- a/server/src/main/java/org/elasticsearch/script/IngestScript.java +++ b/server/src/main/java/org/elasticsearch/script/IngestScript.java @@ -9,6 +9,8 @@ package org.elasticsearch.script; +import org.elasticsearch.core.TimeValue; + import java.util.Map; /** @@ -19,7 +21,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, true); + public static final ScriptContext CONTEXT = new ScriptContext<>("ingest", Factory.class, + 200, TimeValue.timeValueMillis(0), false, true); /** 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 aafee11d82b07..1e84d36b08b13 100644 --- a/server/src/main/java/org/elasticsearch/script/ScriptContext.java +++ b/server/src/main/java/org/elasticsearch/script/ScriptContext.java @@ -75,8 +75,8 @@ public final class ScriptContext { public final boolean allowStoredScript; /** Construct a context with the related instance and compiled classes with caller provided cache defaults */ - ScriptContext(String name, Class factoryClazz, int cacheSizeDefault, TimeValue cacheExpireDefault, - boolean compilationRateLimited, boolean allowStoredScript) { + public ScriptContext(String name, Class factoryClazz, int cacheSizeDefault, TimeValue cacheExpireDefault, + boolean compilationRateLimited, boolean allowStoredScript) { this.name = name; this.factoryClazz = factoryClazz; Method newInstanceMethod = findMethod("FactoryType", factoryClazz, "newInstance"); @@ -105,22 +105,12 @@ public final class ScriptContext { } /** Construct a context with the related instance and compiled classes with defaults for cacheSizeDefault, cacheExpireDefault and - * maxCompilationRateDefault and allow scripts of this context to be stored scripts */ + * compilationRateLimited and allow scripts of this context to be stored scripts */ 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), true, true); } - /** - * Construct a context for a system script usage, using the: - * - default cache size (only relevant when context caching enabled): 200 - * - default cache expiration: no expiration - * - unlimited compilation rate - */ - public ScriptContext(String name, Class factoryClazz, boolean allowStoredScript) { - this(name, factoryClazz, 200, TimeValue.timeValueMillis(0), false, allowStoredScript); - } - /** Returns a method with the given name, or throws an exception if multiple are found. */ private Method findMethod(String type, Class clazz, String methodName) { Method foundMethod = null; diff --git a/server/src/main/java/org/elasticsearch/script/ScriptService.java b/server/src/main/java/org/elasticsearch/script/ScriptService.java index b79c89150656a..104d37467c1cd 100644 --- a/server/src/main/java/org/elasticsearch/script/ScriptService.java +++ b/server/src/main/java/org/elasticsearch/script/ScriptService.java @@ -23,6 +23,8 @@ import org.elasticsearch.cluster.metadata.Metadata; import org.elasticsearch.cluster.service.ClusterService; import org.elasticsearch.common.Strings; +import org.elasticsearch.common.logging.DeprecationCategory; +import org.elasticsearch.common.logging.DeprecationLogger; import org.elasticsearch.common.settings.ClusterSettings; import org.elasticsearch.common.settings.Setting; import org.elasticsearch.common.settings.Setting.Property; @@ -50,6 +52,7 @@ public class ScriptService implements Closeable, ClusterStateApplier, ScriptCompiler { private static final Logger logger = LogManager.getLogger(ScriptService.class); + private static final DeprecationLogger deprecationLogger = DeprecationLogger.getLogger(ScriptService.class); static final String DISABLE_DYNAMIC_SCRIPTING_SETTING = "script.disable_dynamic"; @@ -59,16 +62,22 @@ public class ScriptService implements Closeable, ClusterStateApplier, ScriptComp static final String USE_CONTEXT_RATE_KEY = "use-context"; public static final Setting SCRIPT_GENERAL_CACHE_SIZE_SETTING = - Setting.intSetting("script.cache.max_size", 100, 0, Property.NodeScope); + Setting.intSetting("script.cache.max_size", 3000, 0, Property.Dynamic, Property.NodeScope); public static final Setting SCRIPT_GENERAL_CACHE_EXPIRE_SETTING = - Setting.positiveTimeSetting("script.cache.expire", TimeValue.timeValueMillis(0), Property.NodeScope); + Setting.positiveTimeSetting("script.cache.expire", TimeValue.timeValueMillis(0), Property.Dynamic, Property.NodeScope); public static final Setting SCRIPT_MAX_SIZE_IN_BYTES = Setting.intSetting("script.max_size_in_bytes", 65535, 0, Property.Dynamic, Property.NodeScope); public static final Setting SCRIPT_GENERAL_MAX_COMPILATIONS_RATE_SETTING = - new Setting<>("script.max_compilations_rate", USE_CONTEXT_RATE_KEY, + new Setting<>("script.max_compilations_rate", "150/5m", (String value) -> value.equals(USE_CONTEXT_RATE_KEY) ? USE_CONTEXT_RATE_VALUE: new ScriptCache.CompilationRate(value), Property.Dynamic, Property.NodeScope); + public static final String USE_CONTEXT_RATE_KEY_DEPRECATION_MESSAGE = "[" + USE_CONTEXT_RATE_KEY + "] is deprecated for the setting [" + + SCRIPT_GENERAL_MAX_COMPILATIONS_RATE_SETTING.getKey() + "] as system scripts are now exempt from the rate limit. " + + "Set to a value such as [150/5m] (a rate of 150 compilations per five minutes) to rate limit user scripts in case the " + + "script cache [" + SCRIPT_GENERAL_CACHE_SIZE_SETTING.getKey() + "] is undersized causing script compilation thrashing."; + + // Per-context settings static final String CONTEXT_PREFIX = "script.context."; @@ -77,13 +86,14 @@ public class ScriptService implements Closeable, ClusterStateApplier, ScriptComp public static final Setting.AffixSetting SCRIPT_CACHE_SIZE_SETTING = Setting.affixKeySetting(CONTEXT_PREFIX, "cache_max_size", - key -> Setting.intSetting(key, SCRIPT_GENERAL_CACHE_SIZE_SETTING, 0, Property.NodeScope, Property.Dynamic)); + key -> Setting.intSetting(key, SCRIPT_GENERAL_CACHE_SIZE_SETTING, 0, + Property.NodeScope, Property.Dynamic, Property.Deprecated)); public static final Setting.AffixSetting SCRIPT_CACHE_EXPIRE_SETTING = Setting.affixKeySetting(CONTEXT_PREFIX, "cache_expire", key -> Setting.positiveTimeSetting(key, SCRIPT_GENERAL_CACHE_EXPIRE_SETTING, TimeValue.timeValueMillis(0), - Property.NodeScope, Property.Dynamic)); + Property.NodeScope, Property.Dynamic, Property.Deprecated)); // Unlimited compilation rate for context-specific script caches static final String UNLIMITED_COMPILATION_RATE_KEY = "unlimited"; @@ -94,7 +104,7 @@ public class ScriptService implements Closeable, ClusterStateApplier, ScriptComp key -> new Setting(key, "75/5m", (String value) -> value.equals(UNLIMITED_COMPILATION_RATE_KEY) ? ScriptCache.UNLIMITED_COMPILATION_RATE: new ScriptCache.CompilationRate(value), - Property.NodeScope, Property.Dynamic)); + Property.NodeScope, Property.Dynamic, Property.Deprecated)); private static final ScriptCache.CompilationRate SCRIPT_COMPILATION_RATE_ZERO = new ScriptCache.CompilationRate(0, TimeValue.ZERO); @@ -205,6 +215,10 @@ public ScriptService(Settings settings, Map engines, Map> affixes = Arrays.asList(SCRIPT_MAX_COMPILATIONS_RATE_SETTING, SCRIPT_CACHE_EXPIRE_SETTING, SCRIPT_CACHE_SIZE_SETTING); List customRates = new ArrayList<>(); @@ -277,7 +295,7 @@ void validateCacheSettings(Settings settings) { String.join(", ", customRates) + "] if compile rates disabled via [" + SCRIPT_DISABLE_MAX_COMPILATIONS_RATE_SETTING.getKey() + "]"); } - if (useContext == false) { + if (useContext == false && SCRIPT_GENERAL_MAX_COMPILATIONS_RATE_SETTING.exists(settings)) { throw new IllegalArgumentException("Cannot set custom general compilation rates [" + SCRIPT_GENERAL_MAX_COMPILATIONS_RATE_SETTING.getKey() + "] to [" + SCRIPT_GENERAL_MAX_COMPILATIONS_RATE_SETTING.get(settings) + "] if compile rates disabled via [" + @@ -568,6 +586,7 @@ void setCacheHolder(Settings settings) { } else if (current.general.rate.equals(SCRIPT_GENERAL_MAX_COMPILATIONS_RATE_SETTING.get(settings)) == false || current.general.cacheExpire.equals(SCRIPT_GENERAL_CACHE_EXPIRE_SETTING.get(settings)) == false || current.general.cacheSize != SCRIPT_GENERAL_CACHE_SIZE_SETTING.get(settings)) { + // General compilation rate, cache expiration or cache size changed cacheHolder.set(generalCacheHolder(settings)); } } diff --git a/server/src/main/java/org/elasticsearch/script/TemplateScript.java b/server/src/main/java/org/elasticsearch/script/TemplateScript.java index ea7699ff3c883..7356be0bbdc32 100644 --- a/server/src/main/java/org/elasticsearch/script/TemplateScript.java +++ b/server/src/main/java/org/elasticsearch/script/TemplateScript.java @@ -8,6 +8,8 @@ package org.elasticsearch.script; +import org.elasticsearch.core.TimeValue; + import java.util.Map; /** @@ -39,5 +41,6 @@ public interface Factory { // Remove compilation rate limit for ingest. Ingest pipelines may use many mustache templates, triggering compilation // rate limiting. MustacheScriptEngine explicitly checks for TemplateScript. Rather than complicating the implementation there by // creating a new Script class (as would be customary), this context is used to avoid the default rate limit. - public static final ScriptContext INGEST_CONTEXT = new ScriptContext<>("ingest_template", Factory.class, true); + public static final ScriptContext INGEST_CONTEXT = new ScriptContext<>("ingest_template", Factory.class, + 200, TimeValue.timeValueMillis(0), false, true); } diff --git a/server/src/test/java/org/elasticsearch/script/ScriptServiceTests.java b/server/src/test/java/org/elasticsearch/script/ScriptServiceTests.java index 0ea0760d46dae..86341ee7d236c 100644 --- a/server/src/test/java/org/elasticsearch/script/ScriptServiceTests.java +++ b/server/src/test/java/org/elasticsearch/script/ScriptServiceTests.java @@ -7,6 +7,7 @@ */ package org.elasticsearch.script; +// import org.apache.logging.log4j.Level; // TODO(stu) import org.elasticsearch.ResourceNotFoundException; import org.elasticsearch.action.admin.cluster.storedscripts.GetStoredScriptRequest; import org.elasticsearch.cluster.ClusterName; @@ -16,6 +17,7 @@ import org.elasticsearch.common.bytes.BytesArray; import org.elasticsearch.common.bytes.BytesReference; import org.elasticsearch.common.settings.ClusterSettings; +import org.elasticsearch.common.settings.Setting; import org.elasticsearch.common.settings.Settings; import org.elasticsearch.core.TimeValue; import org.elasticsearch.xcontent.XContentFactory; @@ -37,9 +39,10 @@ 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_MAX_COMPILATIONS_RATE_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_DEPRECATION_MESSAGE; import static org.elasticsearch.script.ScriptService.USE_CONTEXT_RATE_KEY; import static org.hamcrest.CoreMatchers.containsString; import static org.hamcrest.Matchers.is; @@ -210,7 +213,6 @@ public void testCompileCountedInCompilationStats() throws IOException { scriptService.compile(new Script(ScriptType.INLINE, "test", "1+1", Collections.emptyMap()), randomFrom(contexts.values())); assertEquals(1L, scriptService.stats().getCompilations()); } - public void testMultipleCompilationsCountedInCompilationStats() throws IOException { buildScriptService(Settings.EMPTY); int numberOfCompilations = randomIntBetween(1, 20); @@ -221,7 +223,36 @@ public void testMultipleCompilationsCountedInCompilationStats() throws IOExcepti assertEquals(numberOfCompilations, scriptService.stats().getCompilations()); } - public void testCompilationStatsOnCacheHit() throws IOException { + public void testCompilationGeneralStatsOnCacheHit() throws IOException { + buildScriptService(Settings.EMPTY); + Script script = new Script(ScriptType.INLINE, "test", "1+1", Collections.emptyMap()); + ScriptContext context = randomFrom(contexts.values()); + scriptService.compile(script, context); + scriptService.compile(script, context); + assertEquals(1L, scriptService.stats().getCompilations()); + } + + public void testIndexedScriptCountedInGeneralCompilationStats() throws IOException { + buildScriptService(Settings.EMPTY); + ScriptContext ctx = randomFrom(contexts.values()); + scriptService.compile(new Script(ScriptType.STORED, null, "script", Collections.emptyMap()), ctx); + assertEquals(1L, scriptService.stats().getCompilations()); + } + + public void testContextCompilationStatsOnCacheHit() throws IOException { + buildScriptService(Settings.builder() + .put(SCRIPT_GENERAL_MAX_COMPILATIONS_RATE_SETTING.getKey(), USE_CONTEXT_RATE_KEY) + .build()); + Script script = new Script(ScriptType.INLINE, "test", "1+1", Collections.emptyMap()); + ScriptContext context = randomFrom(contexts.values()); + scriptService.compile(script, context); + scriptService.compile(script, context); + assertEquals(1L, scriptService.stats().getCompilations()); + assertWarnings(true, USE_CONTEXT_RATE_KEY_DEPRECATION_MESSAGE); + // assertWarnings(true, new DeprecationWarning(Level.WARN, USE_CONTEXT_RATE_KEY_DEPRECATION_MESSAGE)); // TODO(stu) + } + + public void testGeneralCompilationStatsOnCacheHit() throws IOException { Settings.Builder builder = Settings.builder() .put(SCRIPT_GENERAL_CACHE_SIZE_SETTING.getKey(), 1) .put(SCRIPT_GENERAL_MAX_COMPILATIONS_RATE_SETTING.getKey(), "2/1m"); @@ -233,15 +264,44 @@ public void testCompilationStatsOnCacheHit() throws IOException { assertEquals(1L, scriptService.stats().getCompilations()); } - public void testIndexedScriptCountedInCompilationStats() throws IOException { + public void testGeneralIndexedScriptCountedInCompilationStats() throws IOException { buildScriptService(Settings.EMPTY); ScriptContext ctx = randomFrom(contexts.values()); scriptService.compile(new Script(ScriptType.STORED, null, "script", Collections.emptyMap()), ctx); assertEquals(1L, scriptService.stats().getCompilations()); + } + + public void testContextIndexedScriptCountedInCompilationStats() throws IOException { + buildScriptService(Settings.builder() + .put(SCRIPT_GENERAL_MAX_COMPILATIONS_RATE_SETTING.getKey(), USE_CONTEXT_RATE_KEY) + .build()); + ScriptContext ctx = randomFrom(contexts.values()); + scriptService.compile(new Script(ScriptType.STORED, null, "script", Collections.emptyMap()), ctx); + assertEquals(1L, scriptService.stats().getCompilations()); assertEquals(1L, scriptService.cacheStats().getContextStats().get(ctx.name).getCompilations()); + assertWarnings(true, USE_CONTEXT_RATE_KEY_DEPRECATION_MESSAGE); + // assertWarnings(true, new DeprecationWarning(Level.WARN, USE_CONTEXT_RATE_KEY_DEPRECATION_MESSAGE)); // TODO(stu) } public void testCacheEvictionCountedInCacheEvictionsStats() throws IOException { + ScriptContext context = randomFrom(contexts.values()); + Setting contextCacheSizeSetting = SCRIPT_CACHE_SIZE_SETTING.getConcreteSettingForNamespace(context.name); + buildScriptService(Settings.builder() + .put(SCRIPT_GENERAL_MAX_COMPILATIONS_RATE_SETTING.getKey(), USE_CONTEXT_RATE_KEY) + .put(contextCacheSizeSetting.getKey(), 1) + .build() + ); + scriptService.compile(new Script(ScriptType.INLINE, "test", "1+1", Collections.emptyMap()), context); + scriptService.compile(new Script(ScriptType.INLINE, "test", "2+2", Collections.emptyMap()), context); + assertEquals(2L, scriptService.stats().getCompilations()); + assertEquals(1L, scriptService.stats().getCacheEvictions()); + assertSettingDeprecationsAndWarnings(new Setting[]{contextCacheSizeSetting}, + USE_CONTEXT_RATE_KEY_DEPRECATION_MESSAGE); + // assertSettingDeprecationsAndWarnings(new Setting[]{contextCacheSizeSetting}, // TODO(stu) + // new DeprecationWarning(Level.WARN, USE_CONTEXT_RATE_KEY_DEPRECATION_MESSAGE)); + } + + public void testGeneralCacheEvictionCountedInCacheEvictionsStats() throws IOException { Settings.Builder builder = Settings.builder(); builder.put(SCRIPT_GENERAL_CACHE_SIZE_SETTING.getKey(), 1); builder.put(SCRIPT_GENERAL_MAX_COMPILATIONS_RATE_SETTING.getKey(), "10/1m"); @@ -264,12 +324,18 @@ public void testContextCacheStats() throws IOException { "]; please use indexed, or scripts with parameters instead; this limit can be changed by the [script.context." + ctx + ".max_compilations_rate] setting" ); + Setting cacheSizeA = SCRIPT_CACHE_SIZE_SETTING.getConcreteSettingForNamespace(contextA.name); + Setting compilationRateA = SCRIPT_MAX_COMPILATIONS_RATE_SETTING.getConcreteSettingForNamespace(contextA.name); + + Setting cacheSizeB = SCRIPT_CACHE_SIZE_SETTING.getConcreteSettingForNamespace(contextB.name); + Setting compilationRateB = SCRIPT_MAX_COMPILATIONS_RATE_SETTING.getConcreteSettingForNamespace(contextB.name); + buildScriptService(Settings.builder() .put(SCRIPT_GENERAL_MAX_COMPILATIONS_RATE_SETTING.getKey(), USE_CONTEXT_RATE_KEY) - .put(SCRIPT_CACHE_SIZE_SETTING.getConcreteSettingForNamespace(contextA.name).getKey(), 1) - .put(SCRIPT_MAX_COMPILATIONS_RATE_SETTING.getConcreteSettingForNamespace(contextA.name).getKey(), aRate) - .put(SCRIPT_CACHE_SIZE_SETTING.getConcreteSettingForNamespace(contextB.name).getKey(), 2) - .put(SCRIPT_MAX_COMPILATIONS_RATE_SETTING.getConcreteSettingForNamespace(contextB.name).getKey(), bRate) + .put(cacheSizeA.getKey(), 1) + .put(compilationRateA.getKey(), aRate) + .put(cacheSizeB.getKey(), 2) + .put(compilationRateB.getKey(), bRate) .build()); // Context A @@ -293,20 +359,24 @@ public void testContextCacheStats() throws IOException { assertEquals(CircuitBreakingException.class, gse.getRootCause().getClass()); // Context specific - ScriptCacheStats stats = scriptService.cacheStats(); - assertEquals(2L, stats.getContextStats().get(contextA.name).getCompilations()); - assertEquals(1L, stats.getContextStats().get(contextA.name).getCacheEvictions()); - assertEquals(1L, stats.getContextStats().get(contextA.name).getCompilationLimitTriggered()); + ScriptStats stats = scriptService.stats(); + assertEquals(2L, getByContext(stats, contextA.name).getCompilations()); + assertEquals(1L, getByContext(stats, contextA.name).getCacheEvictions()); + assertEquals(1L, getByContext(stats, contextA.name).getCompilationLimitTriggered()); - assertEquals(3L, stats.getContextStats().get(contextB.name).getCompilations()); - assertEquals(1L, stats.getContextStats().get(contextB.name).getCacheEvictions()); - assertEquals(2L, stats.getContextStats().get(contextB.name).getCompilationLimitTriggered()); - assertNull(scriptService.cacheStats().getGeneralStats()); + assertEquals(3L, getByContext(stats, contextB.name).getCompilations()); + assertEquals(1L, getByContext(stats, contextB.name).getCacheEvictions()); + assertEquals(2L, getByContext(stats, contextB.name).getCompilationLimitTriggered()); // Summed up assertEquals(5L, scriptService.stats().getCompilations()); assertEquals(2L, scriptService.stats().getCacheEvictions()); assertEquals(3L, scriptService.stats().getCompilationLimitTriggered()); + + assertSettingDeprecationsAndWarnings(new Setting[]{cacheSizeA, compilationRateA, cacheSizeB, compilationRateB}, + USE_CONTEXT_RATE_KEY_DEPRECATION_MESSAGE); + // assertSettingDeprecationsAndWarnings(new Setting[]{cacheSizeA, compilationRateA, cacheSizeB, compilationRateB}, // TODO(stu) + // new DeprecationWarning(Level.WARN, USE_CONTEXT_RATE_KEY_DEPRECATION_MESSAGE)); } private ScriptContextStats getByContext(ScriptStats stats, String context) { @@ -411,12 +481,21 @@ public void testConflictContextSettings() throws IOException { illegal.getMessage() ); + Setting ingestExpire = ScriptService.SCRIPT_CACHE_EXPIRE_SETTING.getConcreteSettingForNamespace("ingest"); + Setting fieldSize = ScriptService.SCRIPT_CACHE_SIZE_SETTING.getConcreteSettingForNamespace("field"); + Setting scoreCompilation = ScriptService.SCRIPT_MAX_COMPILATIONS_RATE_SETTING.getConcreteSettingForNamespace("score"); + buildScriptService( Settings.builder() - .put(ScriptService.SCRIPT_CACHE_EXPIRE_SETTING.getConcreteSettingForNamespace("ingest").getKey(), "5m") - .put(ScriptService.SCRIPT_CACHE_SIZE_SETTING.getConcreteSettingForNamespace("field").getKey(), 123) - .put(ScriptService.SCRIPT_MAX_COMPILATIONS_RATE_SETTING.getConcreteSettingForNamespace("score").getKey(), "50/5m") + .put(SCRIPT_GENERAL_MAX_COMPILATIONS_RATE_SETTING.getKey(), USE_CONTEXT_RATE_KEY) + .put(ingestExpire.getKey(), "5m") + .put(fieldSize.getKey(), 123) + .put(scoreCompilation.getKey(), "50/5m") .build()); + assertSettingDeprecationsAndWarnings(new Setting[]{ingestExpire, fieldSize, scoreCompilation}, + USE_CONTEXT_RATE_KEY_DEPRECATION_MESSAGE); + // assertSettingDeprecationsAndWarnings(new Setting[]{ingestExpire, fieldSize, scoreCompilation}, // TODO(stu) + // new DeprecationWarning(Level.WARN, USE_CONTEXT_RATE_KEY_DEPRECATION_MESSAGE)); } public void testFallbackContextSettings() { @@ -428,11 +507,13 @@ public void testFallbackContextSettings() { String cacheExpireFoo = randomValueOtherThan(cacheExpireBackup, () -> randomTimeValue(1, 1000, "h")); TimeValue cacheExpireFooParsed = TimeValue.parseTimeValue(cacheExpireFoo, ""); + Setting cacheSizeSetting = ScriptService.SCRIPT_CACHE_SIZE_SETTING.getConcreteSettingForNamespace("foo"); + Setting cacheExpireSetting = ScriptService.SCRIPT_CACHE_EXPIRE_SETTING.getConcreteSettingForNamespace("foo"); Settings s = Settings.builder() .put(SCRIPT_GENERAL_CACHE_SIZE_SETTING.getKey(), cacheSizeBackup) - .put(ScriptService.SCRIPT_CACHE_SIZE_SETTING.getConcreteSettingForNamespace("foo").getKey(), cacheSizeFoo) + .put(cacheSizeSetting.getKey(), cacheSizeFoo) .put(SCRIPT_GENERAL_CACHE_EXPIRE_SETTING.getKey(), cacheExpireBackup) - .put(ScriptService.SCRIPT_CACHE_EXPIRE_SETTING.getConcreteSettingForNamespace("foo").getKey(), cacheExpireFoo) + .put(cacheExpireSetting.getKey(), cacheExpireFoo) .build(); assertEquals(cacheSizeFoo, ScriptService.SCRIPT_CACHE_SIZE_SETTING.getConcreteSettingForNamespace("foo").get(s).intValue()); @@ -440,12 +521,14 @@ public void testFallbackContextSettings() { assertEquals(cacheExpireFooParsed, ScriptService.SCRIPT_CACHE_EXPIRE_SETTING.getConcreteSettingForNamespace("foo").get(s)); assertEquals(cacheExpireBackupParsed, ScriptService.SCRIPT_CACHE_EXPIRE_SETTING.getConcreteSettingForNamespace("bar").get(s)); + assertSettingDeprecationsAndWarnings(new Setting[]{cacheExpireSetting, cacheExpireSetting}); } public void testUseContextSettingValue() { + Setting contextMaxCompilationRate = ScriptService.SCRIPT_MAX_COMPILATIONS_RATE_SETTING.getConcreteSettingForNamespace("foo"); Settings s = Settings.builder() .put(ScriptService.SCRIPT_GENERAL_MAX_COMPILATIONS_RATE_SETTING.getKey(), ScriptService.USE_CONTEXT_RATE_KEY) - .put(ScriptService.SCRIPT_MAX_COMPILATIONS_RATE_SETTING.getConcreteSettingForNamespace("foo").getKey(), + .put(contextMaxCompilationRate.getKey(), ScriptService.USE_CONTEXT_RATE_KEY) .build(); @@ -456,6 +539,7 @@ public void testUseContextSettingValue() { }); assertEquals("parameter must contain a positive integer and a timevalue, i.e. 10/1m, but was [use-context]", illegal.getMessage()); + assertSettingDeprecationsAndWarnings(new Setting[]{contextMaxCompilationRate}); } public void testCacheHolderGeneralConstructor() throws IOException { @@ -477,9 +561,12 @@ public void testCacheHolderContextConstructor() throws IOException { String aCompilationRate = "77/5m"; String bCompilationRate = "78/6m"; + Setting aSetting = SCRIPT_MAX_COMPILATIONS_RATE_SETTING.getConcreteSettingForNamespace(a); + Setting bSetting = SCRIPT_MAX_COMPILATIONS_RATE_SETTING.getConcreteSettingForNamespace(b); buildScriptService(Settings.builder() - .put(SCRIPT_MAX_COMPILATIONS_RATE_SETTING.getConcreteSettingForNamespace(a).getKey(), aCompilationRate) - .put(SCRIPT_MAX_COMPILATIONS_RATE_SETTING.getConcreteSettingForNamespace(b).getKey(), bCompilationRate) + .put(SCRIPT_GENERAL_MAX_COMPILATIONS_RATE_SETTING.getKey(), USE_CONTEXT_RATE_KEY) + .put(aSetting.getKey(), aCompilationRate) + .put(bSetting.getKey(), bCompilationRate) .build()); assertNull(scriptService.cacheHolder.get().general); @@ -490,6 +577,10 @@ public void testCacheHolderContextConstructor() throws IOException { scriptService.cacheHolder.get().contextCache.get(a).get().rate); assertEquals(new ScriptCache.CompilationRate(bCompilationRate), scriptService.cacheHolder.get().contextCache.get(b).get().rate); + assertSettingDeprecationsAndWarnings(new Setting[]{aSetting, bSetting}, + USE_CONTEXT_RATE_KEY_DEPRECATION_MESSAGE); + // assertSettingDeprecationsAndWarnings(new Setting[]{aSetting, bSetting}, // TODO(stu) + // new DeprecationWarning(Level.WARN, USE_CONTEXT_RATE_KEY_DEPRECATION_MESSAGE)); } public void testCompilationRateUnlimitedContextOnly() throws IOException { @@ -500,19 +591,25 @@ public void testCompilationRateUnlimitedContextOnly() throws IOException { }); assertEquals("parameter must contain a positive integer and a timevalue, i.e. 10/1m, but was [unlimited]", illegal.getMessage()); + + Setting field = SCRIPT_MAX_COMPILATIONS_RATE_SETTING.getConcreteSettingForNamespace("field"); + Setting ingest = SCRIPT_MAX_COMPILATIONS_RATE_SETTING.getConcreteSettingForNamespace("ingest"); // 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(field.getKey(), ScriptService.UNLIMITED_COMPILATION_RATE_KEY) + .put(ingest.getKey(), ScriptService.UNLIMITED_COMPILATION_RATE_KEY) .put(SCRIPT_GENERAL_MAX_COMPILATIONS_RATE_SETTING.getKey(), ScriptService.USE_CONTEXT_RATE_KEY) .build()); + assertSettingDeprecationsAndWarnings(new Setting[]{field, ingest}, + USE_CONTEXT_RATE_KEY_DEPRECATION_MESSAGE); + // assertSettingDeprecationsAndWarnings(new Setting[]{field, ingest}, // TODO(stu) + // new DeprecationWarning(Level.WARN, USE_CONTEXT_RATE_KEY_DEPRECATION_MESSAGE)); } public void testDisableCompilationRateSetting() throws IOException { IllegalArgumentException illegal = expectThrows(IllegalArgumentException.class, () -> { buildScriptService(Settings.builder() + .put("script.max_compilations_rate", "use-context") .put("script.context.ingest.max_compilations_rate", "76/10m") .put("script.context.field.max_compilations_rate", "77/10m") .put("script.disable_max_compilations_rate", true) @@ -536,11 +633,18 @@ public void testDisableCompilationRateSetting() throws IOException { buildScriptService(Settings.builder() .put("script.disable_max_compilations_rate", true) .build()); + assertSettingDeprecationsAndWarnings(new Setting[]{ + SCRIPT_MAX_COMPILATIONS_RATE_SETTING.getConcreteSettingForNamespace("field"), + SCRIPT_MAX_COMPILATIONS_RATE_SETTING.getConcreteSettingForNamespace("ingest")}, + USE_CONTEXT_RATE_KEY_DEPRECATION_MESSAGE); + // assertSettingDeprecationsAndWarnings(new Setting[]{ // TODO(stu) + // SCRIPT_MAX_COMPILATIONS_RATE_SETTING.getConcreteSettingForNamespace("field"), + // SCRIPT_MAX_COMPILATIONS_RATE_SETTING.getConcreteSettingForNamespace("ingest")}, + // new DeprecationWarning(Level.WARN, USE_CONTEXT_RATE_KEY_DEPRECATION_MESSAGE)); } public void testCacheHolderChangeSettings() throws IOException { - Set contextNames = contexts.entrySet().stream().filter(e -> e.getValue().compilationRateLimited) - .map(Map.Entry::getKey).collect(Collectors.toSet()); + Set contextNames = rateLimitedContexts.keySet(); String a = randomFrom(contextNames); String aRate = "77/5m"; String b = randomValueOtherThan(a, () -> randomFrom(contextNames)); @@ -561,11 +665,15 @@ public void testCacheHolderChangeSettings() throws IOException { assertNull(scriptService.cacheHolder.get().contextCache); assertEquals(generalRate, scriptService.cacheHolder.get().general.rate); + Setting compilationRateA = SCRIPT_MAX_COMPILATIONS_RATE_SETTING.getConcreteSettingForNamespace(a); + Setting compilationRateB = SCRIPT_MAX_COMPILATIONS_RATE_SETTING.getConcreteSettingForNamespace(b); + Setting compilationRateC = SCRIPT_MAX_COMPILATIONS_RATE_SETTING.getConcreteSettingForNamespace(c); + scriptService.setCacheHolder(Settings.builder() - .put(SCRIPT_MAX_COMPILATIONS_RATE_SETTING.getConcreteSettingForNamespace(a).getKey(), aRate) - .put(SCRIPT_MAX_COMPILATIONS_RATE_SETTING.getConcreteSettingForNamespace(b).getKey(), bRate) - .put(SCRIPT_MAX_COMPILATIONS_RATE_SETTING.getConcreteSettingForNamespace(c).getKey(), - ScriptService.UNLIMITED_COMPILATION_RATE_KEY) + .put(SCRIPT_GENERAL_MAX_COMPILATIONS_RATE_SETTING.getKey(), USE_CONTEXT_RATE_KEY) + .put(compilationRateA.getKey(), aRate) + .put(compilationRateB.getKey(), bRate) + .put(compilationRateC.getKey(), ScriptService.UNLIMITED_COMPILATION_RATE_KEY) .build() ); @@ -608,6 +716,8 @@ public void testCacheHolderChangeSettings() throws IOException { Settings.builder().put(SCRIPT_GENERAL_MAX_COMPILATIONS_RATE_SETTING.getKey(), bRate).build() ); assertEquals(holder, scriptService.cacheHolder.get()); + + assertSettingDeprecationsAndWarnings(new Setting[]{compilationRateA, compilationRateB, compilationRateC}); } public void testFallbackToContextDefaults() throws IOException { @@ -622,11 +732,15 @@ public void testFallbackToContextDefaults() throws IOException { String name = "score"; + Setting cacheSizeContextSetting = SCRIPT_CACHE_SIZE_SETTING.getConcreteSettingForNamespace(name); + Setting cacheExpireContextSetting = SCRIPT_CACHE_EXPIRE_SETTING.getConcreteSettingForNamespace(name); + Setting compilationRateContextSetting = SCRIPT_MAX_COMPILATIONS_RATE_SETTING.getConcreteSettingForNamespace(name); // Use context specific scriptService.setCacheHolder(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_MAX_COMPILATIONS_RATE_SETTING.getKey(), USE_CONTEXT_RATE_KEY) + .put(cacheSizeContextSetting.getKey(), contextCacheSize) + .put(cacheExpireContextSetting.getKey(), contextExpire) + .put(compilationRateContextSetting.getKey(), contextRateStr) .build() ); @@ -641,7 +755,7 @@ public void testFallbackToContextDefaults() throws IOException { ScriptContext score = contexts.get(name); // Fallback to context defaults - buildScriptService(Settings.EMPTY); + buildScriptService(Settings.builder().put(SCRIPT_GENERAL_MAX_COMPILATIONS_RATE_SETTING.getKey(), USE_CONTEXT_RATE_KEY).build()); holder = scriptService.cacheHolder.get(); assertNotNull(holder.contextCache); @@ -651,6 +765,11 @@ public void testFallbackToContextDefaults() throws IOException { assertEquals(ScriptContext.DEFAULT_COMPILATION_RATE_LIMIT, holder.contextCache.get(name).get().rate.asTuple()); assertEquals(score.cacheSizeDefault, holder.contextCache.get(name).get().cacheSize); assertEquals(score.cacheExpireDefault, holder.contextCache.get(name).get().cacheExpire); + + assertSettingDeprecationsAndWarnings(new Setting[]{cacheSizeContextSetting, cacheExpireContextSetting, + compilationRateContextSetting}, USE_CONTEXT_RATE_KEY_DEPRECATION_MESSAGE); + // assertSettingDeprecationsAndWarnings(new Setting[]{cacheSizeContextSetting, cacheExpireContextSetting, // TODO(stu) + // compilationRateContextSetting}, new DeprecationWarning(Level.WARN, USE_CONTEXT_RATE_KEY_DEPRECATION_MESSAGE)); } protected HashMap> compilationRateLimitedContexts() { diff --git a/test/framework/src/main/java/org/elasticsearch/test/InternalTestCluster.java b/test/framework/src/main/java/org/elasticsearch/test/InternalTestCluster.java index 6cfebd3275812..4305f69924ce8 100644 --- a/test/framework/src/main/java/org/elasticsearch/test/InternalTestCluster.java +++ b/test/framework/src/main/java/org/elasticsearch/test/InternalTestCluster.java @@ -93,7 +93,6 @@ import org.elasticsearch.node.NodeService; import org.elasticsearch.node.NodeValidationException; import org.elasticsearch.plugins.Plugin; -import org.elasticsearch.script.ScriptModule; import org.elasticsearch.script.ScriptService; import org.elasticsearch.search.SearchService; import org.elasticsearch.tasks.TaskManager; @@ -521,16 +520,16 @@ private static Settings getRandomNodeSettings(long seed) { builder.put(TransportSettings.PING_SCHEDULE.getKey(), RandomNumbers.randomIntBetween(random, 100, 2000) + "ms"); } + if (random.nextBoolean()) { - String ctx = randomFrom(random, ScriptModule.CORE_CONTEXTS.keySet()); - builder.put(ScriptService.SCRIPT_CACHE_SIZE_SETTING.getConcreteSettingForNamespace(ctx).getKey(), + builder.put(ScriptService.SCRIPT_GENERAL_CACHE_SIZE_SETTING.getKey(), RandomNumbers.randomIntBetween(random, 0, 2000)); } if (random.nextBoolean()) { - String ctx = randomFrom(random, ScriptModule.CORE_CONTEXTS.keySet()); - builder.put(ScriptService.SCRIPT_CACHE_EXPIRE_SETTING.getConcreteSettingForNamespace(ctx).getKey(), + builder.put(ScriptService.SCRIPT_GENERAL_CACHE_EXPIRE_SETTING.getKey(), timeValueMillis(RandomNumbers.randomIntBetween(random, 750, 10000000)).getStringRep()); } + if (random.nextBoolean()) { int initialMillisBound = RandomNumbers.randomIntBetween(random,10, 100); builder.put(TransportReplicationAction.REPLICATION_INITIAL_RETRY_BACKOFF_BOUND.getKey(), timeValueMillis(initialMillisBound)); diff --git a/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/Watcher.java b/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/Watcher.java index 17b9a7cd1d05c..f682fa9e0bb1c 100644 --- a/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/Watcher.java +++ b/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/Watcher.java @@ -231,7 +231,8 @@ public class Watcher extends Plugin implements SystemIndexPlugin, ScriptPlugin, new ByteSizeValue(1, ByteSizeUnit.MB), new ByteSizeValue(10, ByteSizeUnit.MB), NodeScope); public static final ScriptContext SCRIPT_TEMPLATE_CONTEXT - = new ScriptContext<>("xpack_template", TemplateScript.Factory.class, true); + = new ScriptContext<>("xpack_template", TemplateScript.Factory.class, + 200, TimeValue.timeValueMillis(0), false, true); private static final Logger logger = LogManager.getLogger(Watcher.class); private WatcherIndexingListener listener; diff --git a/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/condition/WatcherConditionScript.java b/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/condition/WatcherConditionScript.java index bd6446b82cb9c..04d4b97a8508c 100644 --- a/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/condition/WatcherConditionScript.java +++ b/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/condition/WatcherConditionScript.java @@ -6,6 +6,7 @@ */ package org.elasticsearch.xpack.watcher.condition; +import org.elasticsearch.core.TimeValue; import org.elasticsearch.script.ScriptContext; import org.elasticsearch.xpack.core.watcher.execution.WatchExecutionContext; import org.elasticsearch.xpack.watcher.support.Variables; @@ -42,5 +43,6 @@ public interface Factory { WatcherConditionScript newInstance(Map params, WatchExecutionContext watcherContext); } - public static ScriptContext CONTEXT = new ScriptContext<>("watcher_condition", Factory.class, true); + public static ScriptContext CONTEXT = new ScriptContext<>("watcher_condition", Factory.class, + 200, TimeValue.timeValueMillis(0), false, true); } diff --git a/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/transform/script/WatcherTransformScript.java b/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/transform/script/WatcherTransformScript.java index e661ea56711b4..3a48e25635f1d 100644 --- a/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/transform/script/WatcherTransformScript.java +++ b/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/transform/script/WatcherTransformScript.java @@ -6,6 +6,7 @@ */ package org.elasticsearch.xpack.watcher.transform.script; +import org.elasticsearch.core.TimeValue; import org.elasticsearch.script.ScriptContext; import org.elasticsearch.xpack.core.watcher.execution.WatchExecutionContext; import org.elasticsearch.xpack.core.watcher.watch.Payload; @@ -43,5 +44,6 @@ public interface Factory { WatcherTransformScript newInstance(Map params, WatchExecutionContext watcherContext, Payload payload); } - public static ScriptContext CONTEXT = new ScriptContext<>("watcher_transform", Factory.class, true); + public static ScriptContext CONTEXT = new ScriptContext<>("watcher_transform", Factory.class, + 200, TimeValue.timeValueMillis(0), false, true); }