From cf94aa14859ade34672ff5e975aa9bd612a29c2c Mon Sep 17 00:00:00 2001 From: Stuart Tettemer Date: Wed, 8 Jul 2020 20:20:21 -0500 Subject: [PATCH 1/7] Scripting: Unlimited compilation rate for ingest * `ingest` and `processor_conditional` default to unlimited compilation rate Refs: #50152 --- .../java/org/elasticsearch/script/IngestConditionalScript.java | 3 +-- .../src/main/java/org/elasticsearch/script/IngestScript.java | 3 +-- 2 files changed, 2 insertions(+), 4 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/script/IngestConditionalScript.java b/server/src/main/java/org/elasticsearch/script/IngestConditionalScript.java index ed224204f8933..f610089ba763a 100644 --- a/server/src/main/java/org/elasticsearch/script/IngestConditionalScript.java +++ b/server/src/main/java/org/elasticsearch/script/IngestConditionalScript.java @@ -19,7 +19,6 @@ package org.elasticsearch.script; -import org.elasticsearch.common.collect.Tuple; import org.elasticsearch.common.unit.TimeValue; import java.util.Map; @@ -33,7 +32,7 @@ public abstract class IngestConditionalScript { /** The context used to compile {@link IngestConditionalScript} factories. */ public static final ScriptContext CONTEXT = new ScriptContext<>("processor_conditional", Factory.class, - 200, TimeValue.timeValueMillis(0), new Tuple<>(375, TimeValue.timeValueMinutes(5))); + 200, TimeValue.timeValueMillis(0), ScriptCache.UNLIMITED_COMPILATION_RATE); /** 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 ffd3475fc52c3..e50d60b3ec1b1 100644 --- a/server/src/main/java/org/elasticsearch/script/IngestScript.java +++ b/server/src/main/java/org/elasticsearch/script/IngestScript.java @@ -20,7 +20,6 @@ package org.elasticsearch.script; -import org.elasticsearch.common.collect.Tuple; import org.elasticsearch.common.unit.TimeValue; import java.util.Map; @@ -34,7 +33,7 @@ public abstract class IngestScript { /** The context used to compile {@link IngestScript} factories. */ public static final ScriptContext CONTEXT = new ScriptContext<>("ingest", Factory.class, - 200, TimeValue.timeValueMillis(0), new Tuple<>(375, TimeValue.timeValueMinutes(5))); + 200, TimeValue.timeValueMillis(0), ScriptCache.UNLIMITED_COMPILATION_RATE); /** The generic runtime parameters for the script. */ private final Map params; From f9cb8f446bc8b905365132aa57a240e2784f7c82 Mon Sep 17 00:00:00 2001 From: Stuart Tettemer Date: Wed, 8 Jul 2020 21:09:19 -0500 Subject: [PATCH 2/7] Forward port CompilationRate value type --- .../script/IngestConditionalScript.java | 2 +- .../elasticsearch/script/IngestScript.java | 2 +- .../org/elasticsearch/script/ScriptCache.java | 94 ++++++++++++++++--- .../elasticsearch/script/ScriptService.java | 55 +++-------- 4 files changed, 97 insertions(+), 56 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/script/IngestConditionalScript.java b/server/src/main/java/org/elasticsearch/script/IngestConditionalScript.java index f610089ba763a..1174b75fb507a 100644 --- a/server/src/main/java/org/elasticsearch/script/IngestConditionalScript.java +++ b/server/src/main/java/org/elasticsearch/script/IngestConditionalScript.java @@ -32,7 +32,7 @@ public abstract class IngestConditionalScript { /** 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); + 200, TimeValue.timeValueMillis(0), ScriptCache.UNLIMITED_COMPILATION_RATE.asTuple()); /** 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 e50d60b3ec1b1..6af53e19614fa 100644 --- a/server/src/main/java/org/elasticsearch/script/IngestScript.java +++ b/server/src/main/java/org/elasticsearch/script/IngestScript.java @@ -33,7 +33,7 @@ public abstract class IngestScript { /** 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); + 200, TimeValue.timeValueMillis(0), ScriptCache.UNLIMITED_COMPILATION_RATE.asTuple()); /** 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 5d59a91b207ce..7ec7cefd58e91 100644 --- a/server/src/main/java/org/elasticsearch/script/ScriptCache.java +++ b/server/src/main/java/org/elasticsearch/script/ScriptCache.java @@ -42,7 +42,7 @@ public class ScriptCache { private static final Logger logger = LogManager.getLogger(ScriptService.class); - static final Tuple UNLIMITED_COMPILATION_RATE = new Tuple<>(0, TimeValue.ZERO); + static final CompilationRate UNLIMITED_COMPILATION_RATE = new CompilationRate(0, TimeValue.ZERO); private final Cache cache; private final ScriptMetrics scriptMetrics; @@ -51,15 +51,15 @@ public class ScriptCache { // Cache settings or derived from settings final int cacheSize; final TimeValue cacheExpire; - final Tuple rate; + final CompilationRate rate; private final double compilesAllowedPerNano; private final String contextRateSetting; ScriptCache( - int cacheMaxSize, - TimeValue cacheExpire, - Tuple maxCompilationRate, - String contextRateSetting + int cacheMaxSize, + TimeValue cacheExpire, + CompilationRate maxCompilationRate, + String contextRateSetting ) { this.cacheSize = cacheMaxSize; this.cacheExpire = cacheExpire; @@ -78,9 +78,9 @@ public class ScriptCache { this.cache = cacheBuilder.removalListener(new ScriptCacheRemovalListener()).build(); this.rate = maxCompilationRate; - this.compilesAllowedPerNano = ((double) rate.v1()) / rate.v2().nanos(); + this.compilesAllowedPerNano = ((double) rate.count) / rate.time.nanos(); this.scriptMetrics = new ScriptMetrics(); - this.tokenBucketState = new AtomicReference(new TokenBucketState(this.rate.v1())); + this.tokenBucketState = new AtomicReference(new TokenBucketState(this.rate.count)); } FactoryType compile( @@ -156,8 +156,8 @@ void checkCompilationLimit() { double scriptsPerTimeWindow = current.availableTokens + (timePassed) * compilesAllowedPerNano; // It's been over the time limit anyway, readjust the bucket to be level - if (scriptsPerTimeWindow > rate.v1()) { - scriptsPerTimeWindow = rate.v1(); + if (scriptsPerTimeWindow > rate.count) { + scriptsPerTimeWindow = rate.count; } // If there is enough tokens in the bucket, allow the request and decrease the tokens by 1 @@ -173,7 +173,7 @@ void checkCompilationLimit() { scriptMetrics.onCompilationLimit(); // Otherwise reject the request throw new CircuitBreakingException("[script] Too many dynamic script compilations within, max: [" + - rate.v1() + "/" + rate.v2() +"]; please use indexed, or scripts with parameters instead; " + + rate + "]; please use indexed, or scripts with parameters instead; " + "this limit can be changed by the [" + contextRateSetting + "] setting", CircuitBreaker.Durability.TRANSIENT); } @@ -243,4 +243,76 @@ private TokenBucketState(long lastInlineCompileTime, double availableTokens, boo this.tokenSuccessfullyTaken = tokenSuccessfullyTaken; } } + + public static class CompilationRate { + public final int count; + public final TimeValue time; + private final String source; + + public CompilationRate(Integer count, TimeValue time) { + this.count = count; + this.time = time; + this.source = null; + } + + public CompilationRate(Tuple rate) { + this(rate.v1(), rate.v2()); + } + + /** + * Parses a string as a non-negative int count and a {@code TimeValue} as arguments split by a slash + */ + public CompilationRate(String value) { + if (value.contains("/") == false || value.startsWith("/") || value.endsWith("/")) { + throw new IllegalArgumentException("parameter must contain a positive integer and a timevalue, i.e. 10/1m, but was [" + + value + "]"); + } + int idx = value.indexOf("/"); + String count = value.substring(0, idx); + String time = value.substring(idx + 1); + try { + int rate = Integer.parseInt(count); + if (rate < 0) { + throw new IllegalArgumentException("rate [" + rate + "] must be positive"); + } + TimeValue timeValue = TimeValue.parseTimeValue(time, "script.max_compilations_rate"); + if (timeValue.nanos() <= 0) { + throw new IllegalArgumentException("time value [" + time + "] must be positive"); + } + // protect against a too hard to check limit, like less than a minute + if (timeValue.seconds() < 60) { + throw new IllegalArgumentException("time value [" + time + "] must be at least on a one minute resolution"); + } + this.count = rate; + this.time = timeValue; + this.source = value; + } catch (NumberFormatException e) { + // the number format exception message is so confusing, that it makes more sense to wrap it with a useful one + throw new IllegalArgumentException("could not parse [" + count + "] as integer in value [" + value + "]", e); + } + } + + public Tuple asTuple() { + return new Tuple<>(this.count, this.time); + } + + @Override + public String toString() { + return source != null ? source : count + "/" + time.toHumanReadableString(0); + } + + @Override + public boolean equals(Object o) { + if (this == o) return true; + if (o == null || getClass() != o.getClass()) return false; + CompilationRate that = (CompilationRate) o; + return count == that.count && + Objects.equals(time, that.time); + } + + @Override + public int hashCode() { + return Objects.hash(count, time); + } + } } diff --git a/server/src/main/java/org/elasticsearch/script/ScriptService.java b/server/src/main/java/org/elasticsearch/script/ScriptService.java index 23913afbb1799..7ca8bc7a772e5 100644 --- a/server/src/main/java/org/elasticsearch/script/ScriptService.java +++ b/server/src/main/java/org/elasticsearch/script/ScriptService.java @@ -66,50 +66,18 @@ public class ScriptService implements Closeable, ClusterStateApplier { // Special setting value for SCRIPT_GENERAL_MAX_COMPILATIONS_RATE to indicate the script service should use context // specific caches - static final Tuple USE_CONTEXT_RATE_VALUE = new Tuple<>(-1, TimeValue.MINUS_ONE); + static final ScriptCache.CompilationRate USE_CONTEXT_RATE_VALUE = new ScriptCache.CompilationRate(-1, TimeValue.MINUS_ONE); static final String USE_CONTEXT_RATE_KEY = "use-context"; - // a parsing function that requires a non negative int and a timevalue as arguments split by a slash - // this allows you to easily define rates - static final Function> MAX_COMPILATION_RATE_FUNCTION = - (String value) -> { - if (value.contains("/") == false || value.startsWith("/") || value.endsWith("/")) { - throw new IllegalArgumentException("parameter must contain a positive integer and a timevalue, i.e. 10/1m, but was [" + - value + "]"); - } - int idx = value.indexOf("/"); - String count = value.substring(0, idx); - String time = value.substring(idx + 1); - try { - - int rate = Integer.parseInt(count); - if (rate < 0) { - throw new IllegalArgumentException("rate [" + rate + "] must be positive"); - } - TimeValue timeValue = TimeValue.parseTimeValue(time, "script.max_compilations_rate"); - if (timeValue.nanos() <= 0) { - throw new IllegalArgumentException("time value [" + time + "] must be positive"); - } - // protect against a too hard to check limit, like less than a minute - if (timeValue.seconds() < 60) { - throw new IllegalArgumentException("time value [" + time + "] must be at least on a one minute resolution"); - } - return Tuple.tuple(rate, timeValue); - } catch (NumberFormatException e) { - // the number format exception message is so confusing, that it makes more sense to wrap it with a useful one - throw new IllegalArgumentException("could not parse [" + count + "] as integer in value [" + value + "]", e); - } - }; - public static final Setting SCRIPT_GENERAL_CACHE_SIZE_SETTING = Setting.intSetting("script.cache.max_size", 100, 0, Property.NodeScope, Property.Deprecated); public static final Setting SCRIPT_GENERAL_CACHE_EXPIRE_SETTING = Setting.positiveTimeSetting("script.cache.expire", TimeValue.timeValueMillis(0), Property.NodeScope, Property.Deprecated); 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 = + 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: MAX_COMPILATION_RATE_FUNCTION.apply(value), + (String value) -> value.equals(USE_CONTEXT_RATE_KEY) ? USE_CONTEXT_RATE_VALUE: new ScriptCache.CompilationRate(value), Property.Dynamic, Property.NodeScope, Property.Deprecated); // Per-context settings @@ -128,15 +96,15 @@ public class ScriptService implements Closeable, ClusterStateApplier { // Unlimited compilation rate for context-specific script caches static final String UNLIMITED_COMPILATION_RATE_KEY = "unlimited"; - public static final Setting.AffixSetting> SCRIPT_MAX_COMPILATIONS_RATE_SETTING = + public static final Setting.AffixSetting SCRIPT_MAX_COMPILATIONS_RATE_SETTING = Setting.affixKeySetting(CONTEXT_PREFIX, "max_compilations_rate", - key -> new Setting<>(key, "75/5m", + key -> new Setting(key, "75/5m", (String value) -> value.equals(UNLIMITED_COMPILATION_RATE_KEY) ? ScriptCache.UNLIMITED_COMPILATION_RATE: - MAX_COMPILATION_RATE_FUNCTION.apply(value), + new ScriptCache.CompilationRate(value), Property.NodeScope, Property.Dynamic)); - private static final Tuple SCRIPT_COMPILATION_RATE_ZERO = new Tuple<>(0, TimeValue.ZERO); + private static final ScriptCache.CompilationRate SCRIPT_COMPILATION_RATE_ZERO = new ScriptCache.CompilationRate(0, TimeValue.ZERO); public static final Setting SCRIPT_DISABLE_MAX_COMPILATIONS_RATE_SETTING = Setting.boolSetting("script.disable_max_compilations_rate", false, Property.NodeScope); @@ -632,14 +600,15 @@ ScriptCache contextCache(Settings settings, ScriptContext context) { TimeValue cacheExpire = cacheExpireSetting.existsOrFallbackExists(settings) ? cacheExpireSetting.get(settings) : context.cacheExpireDefault; - Setting> rateSetting = SCRIPT_MAX_COMPILATIONS_RATE_SETTING.getConcreteSettingForNamespace(context.name); - Tuple rate = null; + 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) { rate = SCRIPT_COMPILATION_RATE_ZERO; } else if (rateSetting.existsOrFallbackExists(settings)) { rate = rateSetting.get(settings); } else { - rate = context.maxCompilationRateDefault; + rate = new ScriptCache.CompilationRate(context.maxCompilationRateDefault); } return new ScriptCache(cacheSize, cacheExpire, rate, rateSetting.getKey()); @@ -654,7 +623,7 @@ static class CacheHolder { final ScriptCache general; final Map> contextCache; - CacheHolder(int cacheMaxSize, TimeValue cacheExpire, Tuple maxCompilationRate, String contextRateSetting) { + CacheHolder(int cacheMaxSize, TimeValue cacheExpire, ScriptCache.CompilationRate maxCompilationRate, String contextRateSetting) { contextCache = null; general = new ScriptCache(cacheMaxSize, cacheExpire, maxCompilationRate, contextRateSetting); } From e216df679faed9f300b02608ca1fabc2f349d5ff Mon Sep 17 00:00:00 2001 From: Stuart Tettemer Date: Wed, 8 Jul 2020 21:25:43 -0500 Subject: [PATCH 3/7] Use CompilationRate everywhere --- .../elasticsearch/script/ScriptService.java | 1 - .../script/ScriptCacheTests.java | 13 +++++---- .../script/ScriptServiceTests.java | 27 +++++++++---------- 3 files changed, 18 insertions(+), 23 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/script/ScriptService.java b/server/src/main/java/org/elasticsearch/script/ScriptService.java index 7ca8bc7a772e5..f5745fd8aaceb 100644 --- a/server/src/main/java/org/elasticsearch/script/ScriptService.java +++ b/server/src/main/java/org/elasticsearch/script/ScriptService.java @@ -34,7 +34,6 @@ import org.elasticsearch.cluster.metadata.Metadata; import org.elasticsearch.cluster.service.ClusterService; import org.elasticsearch.common.Strings; -import org.elasticsearch.common.collect.Tuple; import org.elasticsearch.common.settings.ClusterSettings; import org.elasticsearch.common.settings.Setting; import org.elasticsearch.common.settings.Setting.Property; diff --git a/server/src/test/java/org/elasticsearch/script/ScriptCacheTests.java b/server/src/test/java/org/elasticsearch/script/ScriptCacheTests.java index 6c53624ab0cd3..8d27ec2ff469d 100644 --- a/server/src/test/java/org/elasticsearch/script/ScriptCacheTests.java +++ b/server/src/test/java/org/elasticsearch/script/ScriptCacheTests.java @@ -19,7 +19,6 @@ package org.elasticsearch.script; import org.elasticsearch.common.breaker.CircuitBreakingException; -import org.elasticsearch.common.collect.Tuple; import org.elasticsearch.common.settings.Settings; import org.elasticsearch.common.unit.TimeValue; import org.elasticsearch.test.ESTestCase; @@ -30,24 +29,24 @@ public class ScriptCacheTests extends ESTestCase { public void testCompilationCircuitBreaking() 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); - Tuple rate = ScriptService.SCRIPT_GENERAL_MAX_COMPILATIONS_RATE_SETTING.get(Settings.EMPTY); String settingName = ScriptService.SCRIPT_GENERAL_MAX_COMPILATIONS_RATE_SETTING.getKey(); - ScriptCache cache = new ScriptCache(size, expire, Tuple.tuple(1, TimeValue.timeValueMinutes(1)), settingName); + 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, (Tuple.tuple(2, TimeValue.timeValueMinutes(1))), settingName); + 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, (Tuple.tuple(count, TimeValue.timeValueMinutes(1))), settingName); + 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, (Tuple.tuple(0, TimeValue.timeValueMinutes(1))), settingName); + cache = new ScriptCache(size, expire, new ScriptCache.CompilationRate(0, TimeValue.timeValueMinutes(1)), settingName); expectThrows(CircuitBreakingException.class, cache::checkCompilationLimit); - cache = new ScriptCache(size, expire, (Tuple.tuple(Integer.MAX_VALUE, TimeValue.timeValueMinutes(1))), settingName); + 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(); diff --git a/server/src/test/java/org/elasticsearch/script/ScriptServiceTests.java b/server/src/test/java/org/elasticsearch/script/ScriptServiceTests.java index 651dd57884476..d12549f5d63da 100644 --- a/server/src/test/java/org/elasticsearch/script/ScriptServiceTests.java +++ b/server/src/test/java/org/elasticsearch/script/ScriptServiceTests.java @@ -26,7 +26,6 @@ import org.elasticsearch.common.breaker.CircuitBreakingException; import org.elasticsearch.common.bytes.BytesArray; import org.elasticsearch.common.bytes.BytesReference; -import org.elasticsearch.common.collect.Tuple; import org.elasticsearch.common.settings.ClusterSettings; import org.elasticsearch.common.settings.Setting; import org.elasticsearch.common.settings.Settings; @@ -46,7 +45,6 @@ import java.util.function.Function; import static org.elasticsearch.script.ScriptService.CacheHolder; -import static org.elasticsearch.script.ScriptService.MAX_COMPILATION_RATE_FUNCTION; import static org.elasticsearch.script.ScriptService.SCRIPT_CACHE_EXPIRE_SETTING; import static org.elasticsearch.script.ScriptService.SCRIPT_CACHE_SIZE_SETTING; import static org.elasticsearch.script.ScriptService.SCRIPT_GENERAL_CACHE_SIZE_SETTING; @@ -107,8 +105,8 @@ protected StoredScriptSource getScriptFromClusterState(String id) { } public void testMaxCompilationRateSetting() throws Exception { - assertThat(MAX_COMPILATION_RATE_FUNCTION.apply("10/1m"), is(Tuple.tuple(10, TimeValue.timeValueMinutes(1)))); - assertThat(MAX_COMPILATION_RATE_FUNCTION.apply("10/60s"), is(Tuple.tuple(10, TimeValue.timeValueMinutes(1)))); + assertThat(new ScriptCache.CompilationRate("10/1m"), is(new ScriptCache.CompilationRate(10, TimeValue.timeValueMinutes(1)))); + assertThat(new ScriptCache.CompilationRate("10/60s"), is(new ScriptCache.CompilationRate(10, TimeValue.timeValueMinutes(1)))); assertException("10/m", IllegalArgumentException.class, "failed to parse [m]"); assertException("6/1.6m", IllegalArgumentException.class, "failed to parse [1.6m], fractional time values are not supported"); assertException("foo/bar", IllegalArgumentException.class, "could not parse [foo] as integer in value [foo/bar]"); @@ -128,7 +126,7 @@ public void testMaxCompilationRateSetting() throws Exception { } private void assertException(String rate, Class clazz, String message) { - Exception e = expectThrows(clazz, () -> MAX_COMPILATION_RATE_FUNCTION.apply(rate)); + Exception e = expectThrows(clazz, () -> new ScriptCache.CompilationRate(rate)); assertThat(e.getMessage(), is(message)); } @@ -443,7 +441,7 @@ public void testUseContextSettingValue() { public void testCacheHolderGeneralConstructor() throws IOException { String compilationRate = "77/5m"; - Tuple rate = ScriptService.MAX_COMPILATION_RATE_FUNCTION.apply(compilationRate); + ScriptCache.CompilationRate rate = new ScriptCache.CompilationRate(compilationRate); buildScriptService( Settings.builder().put(SCRIPT_GENERAL_MAX_COMPILATIONS_RATE_SETTING.getKey(), compilationRate).build() ); @@ -472,9 +470,9 @@ public void testCacheHolderContextConstructor() throws IOException { assertNotNull(scriptService.cacheHolder.get().contextCache); assertEquals(contexts.keySet(), scriptService.cacheHolder.get().contextCache.keySet()); - assertEquals(ScriptService.MAX_COMPILATION_RATE_FUNCTION.apply(aCompilationRate), + assertEquals(new ScriptCache.CompilationRate(aCompilationRate), scriptService.cacheHolder.get().contextCache.get(a).get().rate); - assertEquals(ScriptService.MAX_COMPILATION_RATE_FUNCTION.apply(bCompilationRate), + assertEquals(new ScriptCache.CompilationRate(bCompilationRate), scriptService.cacheHolder.get().contextCache.get(b).get().rate); } @@ -535,7 +533,7 @@ public void testCacheHolderChangeSettings() throws IOException { String bRate = "78/6m"; String c = randomValueOtherThanMany(s -> a.equals(s) || b.equals(s), () -> randomFrom(contextNames)); String compilationRate = "77/5m"; - Tuple generalRate = MAX_COMPILATION_RATE_FUNCTION.apply(compilationRate); + ScriptCache.CompilationRate generalRate = new ScriptCache.CompilationRate(compilationRate); Settings s = Settings.builder() .put(SCRIPT_GENERAL_MAX_COMPILATIONS_RATE_SETTING.getKey(), compilationRate) @@ -565,10 +563,9 @@ public void testCacheHolderChangeSettings() throws IOException { ); assertEquals(contexts.keySet(), scriptService.cacheHolder.get().contextCache.keySet()); - String d = randomValueOtherThanMany(Set.of(a, b, c)::contains, () -> randomFrom(contextNames)); - assertEquals(ScriptService.MAX_COMPILATION_RATE_FUNCTION.apply(aRate), + assertEquals(new ScriptCache.CompilationRate(aRate), scriptService.cacheHolder.get().contextCache.get(a).get().rate); - assertEquals(ScriptService.MAX_COMPILATION_RATE_FUNCTION.apply(bRate), + assertEquals(new ScriptCache.CompilationRate(bRate), scriptService.cacheHolder.get().contextCache.get(b).get().rate); assertEquals(ScriptCache.UNLIMITED_COMPILATION_RATE, scriptService.cacheHolder.get().contextCache.get(c).get().rate); @@ -576,7 +573,7 @@ public void testCacheHolderChangeSettings() throws IOException { scriptService.cacheHolder.get().set(b, scriptService.contextCache(Settings.builder() .put(SCRIPT_MAX_COMPILATIONS_RATE_SETTING.getConcreteSettingForNamespace(b).getKey(), aRate).build(), contexts.get(b))); - assertEquals(ScriptService.MAX_COMPILATION_RATE_FUNCTION.apply(aRate), + assertEquals(new ScriptCache.CompilationRate(aRate), scriptService.cacheHolder.get().contextCache.get(b).get().rate); scriptService.setCacheHolder(s); @@ -590,7 +587,7 @@ public void testCacheHolderChangeSettings() throws IOException { assertNotNull(scriptService.cacheHolder.get().general); assertNull(scriptService.cacheHolder.get().contextCache); - assertEquals(MAX_COMPILATION_RATE_FUNCTION.apply(bRate), scriptService.cacheHolder.get().general.rate); + assertEquals(new ScriptCache.CompilationRate(bRate), scriptService.cacheHolder.get().general.rate); CacheHolder holder = scriptService.cacheHolder.get(); scriptService.setCacheHolder( @@ -603,7 +600,7 @@ public void testCacheHolderChangeSettings() throws IOException { public void testFallbackToContextDefaults() throws IOException { String contextRateStr = randomIntBetween(10, 1024) + "/" + randomIntBetween(10, 200) + "m"; - Tuple contextRate = MAX_COMPILATION_RATE_FUNCTION.apply(contextRateStr); + ScriptCache.CompilationRate contextRate = new ScriptCache.CompilationRate(contextRateStr); int contextCacheSize = randomIntBetween(1, 1024); TimeValue contextExpire = TimeValue.timeValueMinutes(randomIntBetween(10, 200)); From 318289c1f70d85a95948df7ed7d3c432f01df965 Mon Sep 17 00:00:00 2001 From: Stuart Tettemer Date: Wed, 8 Jul 2020 22:05:27 -0500 Subject: [PATCH 4/7] Update CompilationRate toString and asTuple --- .../java/org/elasticsearch/script/ScriptServiceTests.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/server/src/test/java/org/elasticsearch/script/ScriptServiceTests.java b/server/src/test/java/org/elasticsearch/script/ScriptServiceTests.java index d12549f5d63da..72a53f92b1f96 100644 --- a/server/src/test/java/org/elasticsearch/script/ScriptServiceTests.java +++ b/server/src/test/java/org/elasticsearch/script/ScriptServiceTests.java @@ -515,7 +515,7 @@ public void testDisableCompilationRateSetting() throws IOException { .build()); }); assertEquals("Cannot set custom general compilation rates [script.max_compilations_rate] " + - "to [Tuple [v1=76, v2=10m]] if compile " + + "to [76/10m] if compile " + "rates disabled via [script.disable_max_compilations_rate]", illegal.getMessage()); @@ -636,7 +636,7 @@ 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); + 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); From 3b0d81d00eb6cd4e801ada3126184c8e4363466a Mon Sep 17 00:00:00 2001 From: Stuart Tettemer Date: Thu, 9 Jul 2020 14:15:04 -0500 Subject: [PATCH 5/7] Reindent ScriptCache constructor --- .../main/java/org/elasticsearch/script/ScriptCache.java | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/script/ScriptCache.java b/server/src/main/java/org/elasticsearch/script/ScriptCache.java index 7ec7cefd58e91..cb0a403ae4c1e 100644 --- a/server/src/main/java/org/elasticsearch/script/ScriptCache.java +++ b/server/src/main/java/org/elasticsearch/script/ScriptCache.java @@ -56,10 +56,10 @@ public class ScriptCache { private final String contextRateSetting; ScriptCache( - int cacheMaxSize, - TimeValue cacheExpire, - CompilationRate maxCompilationRate, - String contextRateSetting + int cacheMaxSize, + TimeValue cacheExpire, + CompilationRate maxCompilationRate, + String contextRateSetting ) { this.cacheSize = cacheMaxSize; this.cacheExpire = cacheExpire; From 3e71d1f52b13bf14d1541459cbbcdfe2e044df17 Mon Sep 17 00:00:00 2001 From: Stuart Tettemer Date: Thu, 9 Jul 2020 14:24:40 -0500 Subject: [PATCH 6/7] Remove use-context --- .../org/elasticsearch/script/ScriptService.java | 5 ----- .../elasticsearch/script/ScriptServiceTests.java | 14 -------------- 2 files changed, 19 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/script/ScriptService.java b/server/src/main/java/org/elasticsearch/script/ScriptService.java index e09c5f662b438..fa673f35e8082 100644 --- a/server/src/main/java/org/elasticsearch/script/ScriptService.java +++ b/server/src/main/java/org/elasticsearch/script/ScriptService.java @@ -63,11 +63,6 @@ public class ScriptService implements Closeable, ClusterStateApplier { 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_MAX_SIZE_IN_BYTES = Setting.intSetting("script.max_size_in_bytes", 65535, 0, Property.Dynamic, Property.NodeScope); diff --git a/server/src/test/java/org/elasticsearch/script/ScriptServiceTests.java b/server/src/test/java/org/elasticsearch/script/ScriptServiceTests.java index 9fa4ac917f533..993a53db1bfe7 100644 --- a/server/src/test/java/org/elasticsearch/script/ScriptServiceTests.java +++ b/server/src/test/java/org/elasticsearch/script/ScriptServiceTests.java @@ -43,7 +43,6 @@ import java.util.function.BiFunction; import java.util.function.Function; -import static org.elasticsearch.script.ScriptService.CacheHolder; 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_MAX_COMPILATIONS_RATE_SETTING; @@ -370,19 +369,6 @@ public void testMaxSizeLimit() throws Exception { iae.getMessage()); } - public void testUseContextSettingValue() { - Settings s = Settings.builder() - .put(ScriptService.SCRIPT_MAX_COMPILATIONS_RATE_SETTING.getConcreteSettingForNamespace("foo").getKey(), - ScriptService.USE_CONTEXT_RATE_KEY) - .build(); - - 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 testCacheHolderContextConstructor() throws IOException { String a = randomFrom(contexts.keySet()); String b = randomValueOtherThan(a, () -> randomFrom(contexts.keySet())); From edb3b12b2be68a9152e66d092ab379172ba73fb7 Mon Sep 17 00:00:00 2001 From: Stuart Tettemer Date: Thu, 9 Jul 2020 14:33:13 -0500 Subject: [PATCH 7/7] Long line in ScriptCacheTests --- .../test/java/org/elasticsearch/script/ScriptCacheTests.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/server/src/test/java/org/elasticsearch/script/ScriptCacheTests.java b/server/src/test/java/org/elasticsearch/script/ScriptCacheTests.java index baf909fa82572..17dfa08b93fd2 100644 --- a/server/src/test/java/org/elasticsearch/script/ScriptCacheTests.java +++ b/server/src/test/java/org/elasticsearch/script/ScriptCacheTests.java @@ -42,7 +42,8 @@ public void testCompilationCircuitBreaking() throws Exception { ScriptCache.CompilationRate rate = ScriptService.SCRIPT_MAX_COMPILATIONS_RATE_SETTING.getConcreteSettingForNamespace(context).get(Settings.EMPTY); String rateSettingName = rateSetting.getKey(); - ScriptCache cache = new ScriptCache(size, expire, new ScriptCache.CompilationRate(1, TimeValue.timeValueMinutes(1)), rateSettingName); + ScriptCache cache = new ScriptCache(size, expire, + new ScriptCache.CompilationRate(1, TimeValue.timeValueMinutes(1)), rateSettingName); cache.checkCompilationLimit(); // should pass expectThrows(CircuitBreakingException.class, cache::checkCompilationLimit); cache = new ScriptCache(size, expire, new ScriptCache.CompilationRate(2, TimeValue.timeValueMinutes(1)), rateSettingName);