From 01795d1925cf3962fd2b103a8f0e4a730d6ff3ed Mon Sep 17 00:00:00 2001 From: Stuart Tettemer Date: Tue, 16 Jun 2020 14:58:18 -0600 Subject: [PATCH] Revert "Scripting: Deprecate general cache settings (#55753)" (#58201) This reverts commit 88e8b34fc2d672060a82979cb782b8cf491a3985. --- .../gradle/test/ClusterFormationTasks.groovy | 8 +- .../testclusters/ElasticsearchNode.java | 6 +- distribution/docker/docker-compose.yml | 4 + docs/reference/migration/index.asciidoc | 1 - docs/reference/migration/migrate_7_9.asciidoc | 71 ---- .../modules/indices/circuit_breaker.asciidoc | 6 +- docs/reference/release-notes.asciidoc | 2 - docs/reference/release-notes/7.9.asciidoc | 11 - .../release-notes/highlights.asciidoc | 20 +- docs/reference/scripting/using.asciidoc | 24 +- qa/remote-clusters/docker-compose-oss.yml | 2 + qa/remote-clusters/docker-compose.yml | 2 + .../common/settings/ClusterSettings.java | 1 - .../org/elasticsearch/script/ScriptCache.java | 89 +---- .../elasticsearch/script/ScriptService.java | 275 +++++++------- .../indices/IndicesServiceCloseTests.java | 2 + .../script/ScriptCacheTests.java | 14 +- .../script/ScriptServiceTests.java | 352 ++++++++++-------- .../test/InternalTestCluster.java | 10 +- .../elasticsearch/xpack/CcrIntegTestCase.java | 2 + .../xpack/deprecation/DeprecationChecks.java | 3 - .../deprecation/NodeDeprecationChecks.java | 64 ---- .../NodeDeprecationChecksTests.java | 50 +-- 23 files changed, 390 insertions(+), 629 deletions(-) delete mode 100644 docs/reference/migration/migrate_7_9.asciidoc delete mode 100644 docs/reference/release-notes/7.9.asciidoc diff --git a/buildSrc/src/main/groovy/org/elasticsearch/gradle/test/ClusterFormationTasks.groovy b/buildSrc/src/main/groovy/org/elasticsearch/gradle/test/ClusterFormationTasks.groovy index 330e112c3b2..04c49d24d68 100644 --- a/buildSrc/src/main/groovy/org/elasticsearch/gradle/test/ClusterFormationTasks.groovy +++ b/buildSrc/src/main/groovy/org/elasticsearch/gradle/test/ClusterFormationTasks.groovy @@ -222,7 +222,7 @@ class ClusterFormationTasks { if (distro.equals("oss")) { snapshotProject = "oss-" + snapshotProject } - + BwcVersions.UnreleasedVersionInfo unreleasedInfo = null if (project.hasProperty('bwcVersions')) { @@ -420,11 +420,7 @@ class ClusterFormationTasks { esConfig['cluster.routing.allocation.disk.watermark.flood_stage'] = '1b' } // increase script compilation limit since tests can rapid-fire script compilations - if (node.nodeVersion.onOrAfter('7.9.0')) { - esConfig['script.disable_max_compilations_rate'] = 'true' - } else { - esConfig['script.max_compilations_rate'] = '2048/1m' - } + esConfig['script.max_compilations_rate'] = '2048/1m' // Temporarily disable the real memory usage circuit breaker. It depends on real memory usage which we have no full control // over and the REST client will not retry on circuit breaking exceptions yet (see #31986 for details). Once the REST client // can retry on circuit breaking exceptions, we can revert again to the default configuration. diff --git a/buildSrc/src/main/java/org/elasticsearch/gradle/testclusters/ElasticsearchNode.java b/buildSrc/src/main/java/org/elasticsearch/gradle/testclusters/ElasticsearchNode.java index 0c057168598..b8cf207694e 100644 --- a/buildSrc/src/main/java/org/elasticsearch/gradle/testclusters/ElasticsearchNode.java +++ b/buildSrc/src/main/java/org/elasticsearch/gradle/testclusters/ElasticsearchNode.java @@ -1093,11 +1093,7 @@ public class ElasticsearchNode implements TestClusterConfiguration { baseConfig.put("cluster.routing.allocation.disk.watermark.low", "1b"); baseConfig.put("cluster.routing.allocation.disk.watermark.high", "1b"); // increase script compilation limit since tests can rapid-fire script compilations - if (getVersion().onOrAfter(Version.fromString("7.9.0"))) { - baseConfig.put("script.disable_max_compilations_rate", "true"); - } else { - baseConfig.put("script.max_compilations_rate", "2048/1m"); - } + baseConfig.put("script.max_compilations_rate", "2048/1m"); if (getVersion().getMajor() >= 6) { baseConfig.put("cluster.routing.allocation.disk.watermark.flood_stage", "1b"); } diff --git a/distribution/docker/docker-compose.yml b/distribution/docker/docker-compose.yml index 87d1309f5d4..ec6730f6dd5 100644 --- a/distribution/docker/docker-compose.yml +++ b/distribution/docker/docker-compose.yml @@ -15,6 +15,7 @@ services: - cluster.routing.allocation.disk.watermark.low=1b - cluster.routing.allocation.disk.watermark.high=1b - cluster.routing.allocation.disk.watermark.flood_stage=1b + - script.max_compilations_rate=2048/1m - node.store.allow_mmap=false - xpack.security.enabled=true - xpack.security.transport.ssl.enabled=true @@ -54,6 +55,7 @@ services: - cluster.routing.allocation.disk.watermark.low=1b - cluster.routing.allocation.disk.watermark.high=1b - cluster.routing.allocation.disk.watermark.flood_stage=1b + - script.max_compilations_rate=2048/1m - node.store.allow_mmap=false - xpack.security.enabled=true - xpack.security.transport.ssl.enabled=true @@ -93,6 +95,7 @@ services: - cluster.routing.allocation.disk.watermark.low=1b - cluster.routing.allocation.disk.watermark.high=1b - cluster.routing.allocation.disk.watermark.flood_stage=1b + - script.max_compilations_rate=2048/1m - node.store.allow_mmap=false volumes: - ./build/oss-repo:/tmp/es-repo @@ -117,6 +120,7 @@ services: - cluster.routing.allocation.disk.watermark.low=1b - cluster.routing.allocation.disk.watermark.high=1b - cluster.routing.allocation.disk.watermark.flood_stage=1b + - script.max_compilations_rate=2048/1m - node.store.allow_mmap=false volumes: - ./build/oss-repo:/tmp/es-repo diff --git a/docs/reference/migration/index.asciidoc b/docs/reference/migration/index.asciidoc index 2b72fbb7e85..1895e563bc0 100644 --- a/docs/reference/migration/index.asciidoc +++ b/docs/reference/migration/index.asciidoc @@ -11,7 +11,6 @@ For information about how to upgrade your cluster, see <>. -- -include::migrate_7_9.asciidoc[] include::migrate_7_8.asciidoc[] include::migrate_7_7.asciidoc[] include::migrate_7_6.asciidoc[] diff --git a/docs/reference/migration/migrate_7_9.asciidoc b/docs/reference/migration/migrate_7_9.asciidoc deleted file mode 100644 index 1e4866092b5..00000000000 --- a/docs/reference/migration/migrate_7_9.asciidoc +++ /dev/null @@ -1,71 +0,0 @@ -[[breaking-changes-7.9]] -== Breaking changes in 7.9 -++++ -7.9 -++++ - -This section discusses the changes that you need to be aware of when migrating -your application to {es} 7.9. - -See also <> and <>. - -* <> - -//NOTE: The notable-breaking-changes tagged regions are re-used in the -//Installation and Upgrade Guide - -//tag::notable-breaking-changes[] -[discrete] -[[breaking_79_script_cache_changes]] -=== Script cache changes -[[deprecate_general_script_cache_size]] -.The `script.cache.max_size` setting is deprecated. - -[%collapsible] -==== -*Details* + -The `script.cache.max_size` setting is deprecated. In {es} 8.0.0, this is -set per-context. - -*Impact* + -To avoid deprecation warnings, discontinue use of the `script.cache.max_size` -setting. You may use `script.context.$CONTEXT.context_max_size` for the particular context. -For example, for the `ingest` context, use `script.context.ingest.context_max_size`. - -==== - -[discrete] -[[deprecate_general_script_expire]] -.The `script.cache.expire` setting is deprecated. - -[%collapsible] -==== -*Details* + -The `script.cache.expire` setting is deprecated. In {es} 8.0.0, this is -set per-context. - -*Impact* + -To avoid deprecation warnings, discontinue use of the `script.cache.expire` -setting. You may use `script.context.$CONTEXT.cache_expire` for the particular context. -For example, for the `update` context, use `script.context.update.cache_expire`. - -==== - -[discrete] -[[deprecate_general_script_compile_rate]] -.The `script.max_compilations_rate` setting is deprecated. - -[%collapsible] -==== -*Details* + -The `script.max_compilations_rate` setting is deprecated. In {es} 8.0.0, this is -set per-context. - -*Impact* + -To avoid deprecation warnings, discontinue use of the `script.max_compilations_rate` -setting. You may use `script.context.$CONTEXT.max_compilations_rate` for the particular -context. For example, for the `processor_conditional` context, use -`script.context.processor_conditional.max_compilations_rate`. - -==== -//end::notable-breaking-changes[] diff --git a/docs/reference/modules/indices/circuit_breaker.asciidoc b/docs/reference/modules/indices/circuit_breaker.asciidoc index 36fd5fc495f..4b1351abe82 100644 --- a/docs/reference/modules/indices/circuit_breaker.asciidoc +++ b/docs/reference/modules/indices/circuit_breaker.asciidoc @@ -111,8 +111,8 @@ 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 75/5m, meaning 75 every 5 + minutes. diff --git a/docs/reference/release-notes.asciidoc b/docs/reference/release-notes.asciidoc index 5bf4bd0cb4e..f18a1d2a1d2 100644 --- a/docs/reference/release-notes.asciidoc +++ b/docs/reference/release-notes.asciidoc @@ -6,7 +6,6 @@ This section summarizes the changes in each release. -* <> * <> * <> * <> @@ -35,7 +34,6 @@ This section summarizes the changes in each release. -- -include::release-notes/7.9.asciidoc[] include::release-notes/7.8.asciidoc[] include::release-notes/7.7.asciidoc[] include::release-notes/7.6.asciidoc[] diff --git a/docs/reference/release-notes/7.9.asciidoc b/docs/reference/release-notes/7.9.asciidoc deleted file mode 100644 index 2ab14c90a6e..00000000000 --- a/docs/reference/release-notes/7.9.asciidoc +++ /dev/null @@ -1,11 +0,0 @@ -[[release-notes-7.9.0]] -== {es} version 7.9.0 - -Also see <>. - -[[breaking-7.9.0]] -[float] -=== Breaking changes - -Script Cache:: -* Script cache size and rate limiting are per-context {pull}55753[#55753] (issue: {issue}50152[#50152]) diff --git a/docs/reference/release-notes/highlights.asciidoc b/docs/reference/release-notes/highlights.asciidoc index d0aa95cd4c0..3e26882c281 100644 --- a/docs/reference/release-notes/highlights.asciidoc +++ b/docs/reference/release-notes/highlights.asciidoc @@ -3,16 +3,16 @@ coming[{minor-version}] -Here are the highlights of what's new and improved in {es} {minor-version}! +Here are the highlights of what's new and improved in {es} {minor-version}! ifeval::["{release-state}"!="unreleased"] -For detailed information about this release, see the -<> and +For detailed information about this release, see the +<> and <>. endif::[] // Add previous release to the list -Other versions: -{ref-bare}/7.8/release-highlights.html[7.8] +Other versions: +{ref-bare}/7.8/release-highlights.html[7.8] | {ref-bare}/7.7/release-highlights.html[7.7] | {ref-bare}/7.6/release-highlights-7.6.0.html[7.6] | {ref-bare}/7.5/release-highlights-7.5.0.html[7.5] @@ -24,17 +24,17 @@ Other versions: -// Use the notable-highlights tag to mark entries that +// Use the notable-highlights tag to mark entries that // should be featured in the Stack Installation and Upgrade Guide: -// tag::notable-highlights[] +// tag::notable-highlights[] // [discrete] // === Heading // -// Description. +// Description. // end::notable-highlights[] // Omit the notable highlights tag for entries that only need to appear in the ES ref: -// [float] +// [float] // === Heading // -// Description. +// Description. diff --git a/docs/reference/scripting/using.asciidoc b/docs/reference/scripting/using.asciidoc index 4e4f0126179..abbec0a5565 100644 --- a/docs/reference/scripting/using.asciidoc +++ b/docs/reference/scripting/using.asciidoc @@ -98,11 +98,9 @@ second version is only compiled once. If you compile too many unique scripts within a small amount of time, Elasticsearch will reject the new dynamic scripts with a -`circuit_breaking_exception` error. By default, up to 75 scripts per -5 minutes will be compiled for most contexts and 375 scripts per 5 minutes -for `ingest` contexts. You can change these settings dynamically by setting -`script.context.$CONTEXT.max_compilations_rate` e.g., -`script.context.field.max_compilations_rate=100/10m`. +`circuit_breaking_exception` error. By default, up to 15 inline scripts per +minute will be compiled. You can change this setting dynamically by setting +`script.max_compilations_rate`. ======================================== @@ -137,7 +135,7 @@ The same script in the normal form: Scripts may be stored in and retrieved from the cluster state using the `_scripts` end-point. -If the {es} {security-features} are enabled, you must have the following +If the {es} {security-features} are enabled, you must have the following privileges to create, retrieve, and delete stored scripts: * cluster: `all` or `manage` @@ -243,17 +241,9 @@ templating language]. See <> for more information and examples. 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, but -you can configure the size of this cache using the -`script.context.$CONTEXT.cache_expire` setting. -By default, the cache size is `100` for all contexts except the `ingest` and the -`processor_conditional` context, where it is `200`. - -|==== -| Context | Default Cache Size -| `ingest` | 200 -| `processor_conditional` | 200 -| default | 100 -|==== +you can change this behavior by using the `script.cache.expire` setting. +You can configure the size of this cache by using the `script.cache.max_size` setting. +By default, the cache size is `100`. NOTE: The size of scripts is limited to 65,535 bytes. This can be changed by setting `script.max_size_in_bytes` setting to increase that soft diff --git a/qa/remote-clusters/docker-compose-oss.yml b/qa/remote-clusters/docker-compose-oss.yml index 1a77d1bc999..5cd891373c5 100644 --- a/qa/remote-clusters/docker-compose-oss.yml +++ b/qa/remote-clusters/docker-compose-oss.yml @@ -15,6 +15,7 @@ services: - cluster.routing.allocation.disk.watermark.low=1b - cluster.routing.allocation.disk.watermark.high=1b - cluster.routing.allocation.disk.watermark.flood_stage=1b + - script.max_compilations_rate=2048/1m - node.store.allow_mmap=false volumes: - ./build/oss-repo:/tmp/es-repo @@ -49,6 +50,7 @@ services: - cluster.routing.allocation.disk.watermark.low=1b - cluster.routing.allocation.disk.watermark.high=1b - cluster.routing.allocation.disk.watermark.flood_stage=1b + - script.max_compilations_rate=2048/1m - node.store.allow_mmap=false volumes: - ./build/oss-repo:/tmp/es-repo diff --git a/qa/remote-clusters/docker-compose.yml b/qa/remote-clusters/docker-compose.yml index 51bf9c8d050..05af5541785 100644 --- a/qa/remote-clusters/docker-compose.yml +++ b/qa/remote-clusters/docker-compose.yml @@ -15,6 +15,7 @@ services: - cluster.routing.allocation.disk.watermark.low=1b - cluster.routing.allocation.disk.watermark.high=1b - cluster.routing.allocation.disk.watermark.flood_stage=1b + - script.max_compilations_rate=2048/1m - node.store.allow_mmap=false - xpack.security.enabled=true - xpack.security.transport.ssl.enabled=true @@ -64,6 +65,7 @@ services: - cluster.routing.allocation.disk.watermark.low=1b - cluster.routing.allocation.disk.watermark.high=1b - cluster.routing.allocation.disk.watermark.flood_stage=1b + - script.max_compilations_rate=2048/1m - node.store.allow_mmap=false - xpack.security.enabled=true - xpack.security.transport.ssl.enabled=true diff --git a/server/src/main/java/org/elasticsearch/common/settings/ClusterSettings.java b/server/src/main/java/org/elasticsearch/common/settings/ClusterSettings.java index 253b61e7d61..4b0c164bf20 100644 --- a/server/src/main/java/org/elasticsearch/common/settings/ClusterSettings.java +++ b/server/src/main/java/org/elasticsearch/common/settings/ClusterSettings.java @@ -412,7 +412,6 @@ public final class ClusterSettings extends AbstractScopedSettings { ScriptService.SCRIPT_GENERAL_MAX_COMPILATIONS_RATE_SETTING, ScriptService.SCRIPT_CACHE_SIZE_SETTING, ScriptService.SCRIPT_CACHE_EXPIRE_SETTING, - ScriptService.SCRIPT_DISABLE_MAX_COMPILATIONS_RATE_SETTING, ScriptService.SCRIPT_MAX_COMPILATIONS_RATE_SETTING, ScriptService.SCRIPT_MAX_SIZE_IN_BYTES, ScriptService.TYPES_ALLOWED_SETTING, diff --git a/server/src/main/java/org/elasticsearch/script/ScriptCache.java b/server/src/main/java/org/elasticsearch/script/ScriptCache.java index aa2c16ec9b7..5d59a91b207 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 CompilationRate UNLIMITED_COMPILATION_RATE = new CompilationRate(0, TimeValue.ZERO); + static final Tuple UNLIMITED_COMPILATION_RATE = new Tuple<>(0, TimeValue.ZERO); private final Cache cache; private final ScriptMetrics scriptMetrics; @@ -51,14 +51,14 @@ public class ScriptCache { // Cache settings or derived from settings final int cacheSize; final TimeValue cacheExpire; - final CompilationRate rate; + final Tuple rate; private final double compilesAllowedPerNano; private final String contextRateSetting; ScriptCache( int cacheMaxSize, TimeValue cacheExpire, - CompilationRate maxCompilationRate, + Tuple maxCompilationRate, String contextRateSetting ) { this.cacheSize = cacheMaxSize; @@ -78,9 +78,9 @@ public class ScriptCache { this.cache = cacheBuilder.removalListener(new ScriptCacheRemovalListener()).build(); this.rate = maxCompilationRate; - this.compilesAllowedPerNano = ((double) rate.count) / rate.time.nanos(); + this.compilesAllowedPerNano = ((double) rate.v1()) / rate.v2().nanos(); this.scriptMetrics = new ScriptMetrics(); - this.tokenBucketState = new AtomicReference(new TokenBucketState(this.rate.count)); + this.tokenBucketState = new AtomicReference(new TokenBucketState(this.rate.v1())); } FactoryType compile( @@ -156,8 +156,8 @@ public class ScriptCache { double scriptsPerTimeWindow = current.availableTokens + (timePassed) * compilesAllowedPerNano; // It's been over the time limit anyway, readjust the bucket to be level - if (scriptsPerTimeWindow > rate.count) { - scriptsPerTimeWindow = rate.count; + if (scriptsPerTimeWindow > rate.v1()) { + scriptsPerTimeWindow = rate.v1(); } // If there is enough tokens in the bucket, allow the request and decrease the tokens by 1 @@ -173,7 +173,7 @@ public class ScriptCache { scriptMetrics.onCompilationLimit(); // Otherwise reject the request throw new CircuitBreakingException("[script] Too many dynamic script compilations within, max: [" + - rate + "]; please use indexed, or scripts with parameters instead; " + + rate.v1() + "/" + rate.v2() +"]; please use indexed, or scripts with parameters instead; " + "this limit can be changed by the [" + contextRateSetting + "] setting", CircuitBreaker.Durability.TRANSIENT); } @@ -243,77 +243,4 @@ public class ScriptCache { 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 f78ab24b75e..4f37beac8fe 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 org.elasticsearch.cluster.ClusterStateApplier; 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; @@ -46,6 +47,7 @@ import java.io.IOException; import java.nio.charset.StandardCharsets; import java.util.ArrayList; import java.util.Arrays; +import java.util.Collection; import java.util.Collections; import java.util.Comparator; import java.util.HashMap; @@ -66,19 +68,51 @@ 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 ScriptCache.CompilationRate USE_CONTEXT_RATE_VALUE = new ScriptCache.CompilationRate(-1, TimeValue.MINUS_ONE); + static final Tuple USE_CONTEXT_RATE_VALUE = new Tuple<>(-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); + 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, Property.Deprecated); + 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, Property.Deprecated); + public static final Setting> SCRIPT_GENERAL_MAX_COMPILATIONS_RATE_SETTING = + new Setting<>("script.max_compilations_rate", "75/5m", + (String value) -> value.equals(USE_CONTEXT_RATE_KEY) ? USE_CONTEXT_RATE_VALUE: MAX_COMPILATION_RATE_FUNCTION.apply(value), + Property.Dynamic, Property.NodeScope); // Per-context settings static final String CONTEXT_PREFIX = "script.context."; @@ -99,18 +133,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: - new ScriptCache.CompilationRate(value), + MAX_COMPILATION_RATE_FUNCTION.apply(value), Property.NodeScope, Property.Dynamic)); - 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); + private static final Tuple SCRIPT_COMPILATION_RATE_ZERO = new Tuple<>(0, TimeValue.ZERO); public static final String ALLOW_NONE = "none"; @@ -129,8 +160,7 @@ public class ScriptService implements Closeable, ClusterStateApplier { private int maxSizeInBytes; - // package private for tests - final AtomicReference cacheHolder = new AtomicReference<>(); + private final AtomicReference cacheHolder; public ScriptService(Settings settings, Map engines, Map> contexts) { this.engines = Objects.requireNonNull(engines); @@ -213,7 +243,7 @@ public class ScriptService implements Closeable, ClusterStateApplier { // Validation requires knowing which contexts exist. this.validateCacheSettings(settings); - this.setCacheHolder(settings); + cacheHolder = new AtomicReference<>(new CacheHolder(settings, contexts.values(), compilationLimitsEnabled())); } /** @@ -229,25 +259,23 @@ public class ScriptService implements Closeable, ClusterStateApplier { // Handle all updatable per-context settings at once for each context. for (ScriptContext context: contexts.values()) { clusterSettings.addSettingsUpdateConsumer( - (settings) -> cacheHolder.get().set(context.name, contextCache(settings, context)), + (settings) -> cacheHolder.get().updateContextSettings(settings, context), 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 - ) + 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), + (settings) -> cacheHolder.set(cacheHolder.get().withUpdatedCacheSettings(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 @@ -262,39 +290,31 @@ public class ScriptService implements Closeable, ClusterStateApplier { 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) { - for (String context: affix.getAsMap(settings).keySet()) { - String s = affix.getConcreteSettingForNamespace(context).getKey(); - if (contexts.containsKey(context) == false) { - throw new IllegalArgumentException("Context [" + context + "] doesn't exist for setting [" + s + "]"); - } - keys.add(s); - if (affix.equals(SCRIPT_MAX_COMPILATIONS_RATE_SETTING)) { - customRates.add(s); - } - } + keys.addAll(getConcreteSettingKeys(affix, 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()); - throw new IllegalArgumentException("Cannot set custom context compilation rates [" + - 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() + "]"); + } + + /** + * Get concrete settings keys from affix settings for given Settings. Throws an IllegalArgumentException if the namespace of matching + * affix settings do not match any context name. + */ + List getConcreteSettingKeys(Setting.AffixSetting setting, Settings settings) { + List concreteKeys = new ArrayList<>(); + for (String context: setting.getAsMap(settings).keySet()) { + String s = setting.getConcreteSettingForNamespace(context).getKey(); + if (contexts.containsKey(context) == false) { + throw new IllegalArgumentException("Context [" + context + "] doesn't exist for setting [" + s + "]"); } + concreteKeys.add(s); } + concreteKeys.sort(Comparator.naturalOrder()); + return concreteKeys; } @Override @@ -393,7 +413,7 @@ public class ScriptService implements Closeable, ClusterStateApplier { return typesAllowed == null || typesAllowed.contains(scriptType.getName()); } - public boolean isContextEnabled(ScriptContext scriptContext) { + public boolean isContextEnabled(ScriptContext scriptContext) { return contextsAllowed == null || contextsAllowed.contains(scriptContext.name); } @@ -557,67 +577,6 @@ public class ScriptService implements Closeable, ClusterStateApplier { 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))); - return new CacheHolder(contextCache); - } - - ScriptCache contextCache(Settings settings, ScriptContext context) { - Setting cacheSizeSetting = SCRIPT_CACHE_SIZE_SETTING.getConcreteSettingForNamespace(context.name); - int cacheSize = cacheSizeSetting.existsOrFallbackExists(settings) ? cacheSizeSetting.get(settings) : context.cacheSizeDefault; - - Setting cacheExpireSetting = SCRIPT_CACHE_EXPIRE_SETTING.getConcreteSettingForNamespace(context.name); - TimeValue cacheExpire = cacheExpireSetting.existsOrFallbackExists(settings) ? - cacheExpireSetting.get(settings) : context.cacheExpireDefault; - - 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 = new ScriptCache.CompilationRate(context.maxCompilationRateDefault); - } - - return new ScriptCache(cacheSize, cacheExpire, rate, rateSetting.getKey()); - } - /** * Container for the ScriptCache(s). This class operates in two modes: * 1) general mode, if the general script cache is configured. There are no context caches in this case. @@ -627,16 +586,80 @@ public class ScriptService implements Closeable, ClusterStateApplier { 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); + final Set> contexts; + final boolean compilationLimitsEnabled; + + CacheHolder(Settings settings, Collection> contexts, boolean compilationLimitsEnabled) { + this.compilationLimitsEnabled = compilationLimitsEnabled; + this.contexts = Collections.unmodifiableSet(new HashSet<>(contexts)); + if (SCRIPT_GENERAL_MAX_COMPILATIONS_RATE_SETTING.get(settings).equals(USE_CONTEXT_RATE_VALUE)) { + this.general = null; + Map> contextCache = new HashMap<>(this.contexts.size()); + for (ScriptContext context : this.contexts) { + contextCache.put(context.name, + new AtomicReference<>(contextFromSettings(settings, context, this.compilationLimitsEnabled))); + } + this.contextCache = Collections.unmodifiableMap(contextCache); + } else { + this.contextCache = null; + this.general = new ScriptCache( + SCRIPT_GENERAL_CACHE_SIZE_SETTING.get(settings), + SCRIPT_GENERAL_CACHE_EXPIRE_SETTING.get(settings), + compilationLimitsEnabled ? + SCRIPT_GENERAL_MAX_COMPILATIONS_RATE_SETTING.get(settings) : + SCRIPT_COMPILATION_RATE_ZERO, + SCRIPT_GENERAL_MAX_COMPILATIONS_RATE_SETTING.getKey()); + } } - 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; + /** + * Create a ScriptCache for the given context. + */ + private static ScriptCache contextFromSettings(Settings settings, ScriptContext context, boolean compilationLimitsEnabled) { + String name = context.name; + Tuple compileRate; + Setting> rateSetting = SCRIPT_MAX_COMPILATIONS_RATE_SETTING.getConcreteSettingForNamespace(name); + if (compilationLimitsEnabled == false) { + compileRate = SCRIPT_COMPILATION_RATE_ZERO; + } else if (rateSetting.existsOrFallbackExists(settings)) { + compileRate = rateSetting.get(settings); + } else { + compileRate = context.maxCompilationRateDefault; + } + + Setting cacheExpire = SCRIPT_CACHE_EXPIRE_SETTING.getConcreteSettingForNamespace(name); + Setting cacheSize = SCRIPT_CACHE_SIZE_SETTING.getConcreteSettingForNamespace(name); + + return new ScriptCache(cacheSize.existsOrFallbackExists(settings) ? cacheSize.get(settings) : context.cacheSizeDefault, + cacheExpire.existsOrFallbackExists(settings) ? cacheExpire.get(settings) : context.cacheExpireDefault, + compileRate, + SCRIPT_MAX_COMPILATIONS_RATE_SETTING.getConcreteSettingForNamespace(context.name).getKey()); + } + + /** + * Returns a CacheHolder with the given settings. Flips between general and context caches if necessary. Creates new general + * cache if in general cache mode and {@code script.max_compilations_rate} has changed to any value other than {@code use-context}. + */ + CacheHolder withUpdatedCacheSettings(Settings settings) { + if (SCRIPT_GENERAL_MAX_COMPILATIONS_RATE_SETTING.get(settings).equals(USE_CONTEXT_RATE_VALUE)) { + if (general != null) { + // Flipping to context specific + logger.debug("Switching to context cache from general cache"); + return new CacheHolder(settings, contexts, compilationLimitsEnabled); + } + } else if (general == null) { + // Flipping to general + logger.debug("Switching from context cache to general cache"); + return new CacheHolder(settings, contexts, compilationLimitsEnabled); + } else if (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 + logger.debug("General compilation rate changed from [" + general.rate + "] to [" + + SCRIPT_GENERAL_MAX_COMPILATIONS_RATE_SETTING.get(settings) + "], creating new general cache"); + return new CacheHolder(settings, contexts, compilationLimitsEnabled); + } + + // no-op change, this is possible when context settings change while in context mode + return this; } /** @@ -666,25 +689,25 @@ public class ScriptService implements Closeable, ClusterStateApplier { return new ScriptCacheStats(general.stats()); } Map context = new HashMap<>(contextCache.size()); - for (String name: contextCache.keySet()) { - context.put(name, contextCache.get(name).get().stats()); + for (ScriptContext ctx: contexts) { + context.put(ctx.name, contextCache.get(ctx.name).get().stats()); } return new ScriptCacheStats(context); } /** - * Update a single context cache if we're in the context cache mode otherwise no-op. + * Update settings for the context cache, if we're in the context cache mode otherwise no-op. */ - void set(String name, ScriptCache cache) { + void updateContextSettings(Settings settings, ScriptContext context) { if (general != null) { return; } - AtomicReference ref = contextCache.get(name); - assert ref != null : "expected script cache to exist for context [" + name + "]"; - ScriptCache oldCache = ref.get(); - assert oldCache != null : "expected script cache to be non-null for context [" + name + "]"; - ref.set(cache); - logger.debug("Replaced context [" + name + "] with new settings"); + AtomicReference ref = contextCache.get(context.name); + assert ref != null : "expected script cache to exist for context [" + context.name + "]"; + ScriptCache cache = ref.get(); + assert cache != null : "expected script cache to be non-null for context [" + context.name + "]"; + ref.set(contextFromSettings(settings, context, compilationLimitsEnabled)); + logger.debug("Replaced context [" + context.name + "] with new settings"); } } } diff --git a/server/src/test/java/org/elasticsearch/indices/IndicesServiceCloseTests.java b/server/src/test/java/org/elasticsearch/indices/IndicesServiceCloseTests.java index 1ef1ccb8829..92520d6bf97 100644 --- a/server/src/test/java/org/elasticsearch/indices/IndicesServiceCloseTests.java +++ b/server/src/test/java/org/elasticsearch/indices/IndicesServiceCloseTests.java @@ -39,6 +39,7 @@ import org.elasticsearch.indices.breaker.HierarchyCircuitBreakerService; import org.elasticsearch.node.MockNode; import org.elasticsearch.node.Node; import org.elasticsearch.node.NodeValidationException; +import org.elasticsearch.script.ScriptService; import org.elasticsearch.test.ESTestCase; import org.elasticsearch.test.InternalSettingsPlugin; import org.elasticsearch.test.InternalTestCluster; @@ -68,6 +69,7 @@ public class IndicesServiceCloseTests extends ESTestCase { .put(Environment.PATH_REPO_SETTING.getKey(), tempDir.resolve("repo")) .put(Environment.PATH_SHARED_DATA_SETTING.getKey(), createTempDir().getParent()) .put(Node.NODE_NAME_SETTING.getKey(), nodeName) + .put(ScriptService.SCRIPT_GENERAL_MAX_COMPILATIONS_RATE_SETTING.getKey(), "1000/1m") .put(EsExecutors.NODE_PROCESSORS_SETTING.getKey(), 1) // limit the number of threads created .put("transport.type", getTestTransportType()) .put(Node.NODE_DATA_SETTING.getKey(), true) diff --git a/server/src/test/java/org/elasticsearch/script/ScriptCacheTests.java b/server/src/test/java/org/elasticsearch/script/ScriptCacheTests.java index 281adc7cdd7..6c53624ab0c 100644 --- a/server/src/test/java/org/elasticsearch/script/ScriptCacheTests.java +++ b/server/src/test/java/org/elasticsearch/script/ScriptCacheTests.java @@ -19,6 +19,7 @@ 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; @@ -29,25 +30,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); - ScriptCache.CompilationRate rate = ScriptService.SCRIPT_GENERAL_MAX_COMPILATIONS_RATE_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, new ScriptCache.CompilationRate(1, TimeValue.timeValueMinutes(1)), settingName); + ScriptCache cache = new ScriptCache(size, expire, Tuple.tuple(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 = new ScriptCache(size, expire, (Tuple.tuple(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); + cache = new ScriptCache(size, expire, (Tuple.tuple(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); + cache = new ScriptCache(size, expire, (Tuple.tuple(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); + cache = new ScriptCache(size, expire, (Tuple.tuple(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 820f7e5de1e..5dc3b5756f0 100644 --- a/server/src/test/java/org/elasticsearch/script/ScriptServiceTests.java +++ b/server/src/test/java/org/elasticsearch/script/ScriptServiceTests.java @@ -26,8 +26,8 @@ import org.elasticsearch.cluster.metadata.Metadata; 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; import org.elasticsearch.common.unit.TimeValue; import org.elasticsearch.common.xcontent.XContentFactory; @@ -38,19 +38,23 @@ import org.junit.Before; import java.io.IOException; import java.util.Arrays; +import java.util.Collection; import java.util.Collections; import java.util.HashMap; +import java.util.HashSet; import java.util.Map; -import java.util.Set; import java.util.function.BiFunction; import java.util.function.Function; +import java.util.stream.Collectors; +import static org.elasticsearch.script.ScriptService.MAX_COMPILATION_RATE_FUNCTION; import static org.elasticsearch.script.ScriptService.SCRIPT_CACHE_EXPIRE_SETTING; import static org.elasticsearch.script.ScriptService.SCRIPT_CACHE_SIZE_SETTING; import static org.elasticsearch.script.ScriptService.SCRIPT_GENERAL_CACHE_EXPIRE_SETTING; -import static org.elasticsearch.script.ScriptService.SCRIPT_GENERAL_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; import static org.hamcrest.CoreMatchers.containsString; import static org.hamcrest.Matchers.is; import static org.hamcrest.Matchers.notNullValue; @@ -106,8 +110,8 @@ public class ScriptServiceTests extends ESTestCase { } public void testMaxCompilationRateSetting() throws Exception { - 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)))); + 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)))); 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]"); @@ -127,7 +131,7 @@ public class ScriptServiceTests extends ESTestCase { } private void assertException(String rate, Class clazz, String message) { - Exception e = expectThrows(clazz, () -> new ScriptCache.CompilationRate(rate)); + Exception e = expectThrows(clazz, () -> MAX_COMPILATION_RATE_FUNCTION.apply(rate)); assertThat(e.getMessage(), is(message)); } @@ -229,31 +233,26 @@ public class ScriptServiceTests extends ESTestCase { } public void testCompilationStatsOnCacheHit() 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"); + Settings.Builder builder = Settings.builder(); + builder.put(SCRIPT_GENERAL_CACHE_SIZE_SETTING.getKey(), 1); buildScriptService(builder.build()); Script script = new Script(ScriptType.INLINE, "test", "1+1", Collections.emptyMap()); ScriptContext context = randomFrom(contexts.values()); scriptService.compile(script, context); scriptService.compile(script, context); assertEquals(1L, scriptService.stats().getCompilations()); - assertSettingDeprecationsAndWarnings(new Setting[]{SCRIPT_GENERAL_CACHE_SIZE_SETTING, - SCRIPT_GENERAL_MAX_COMPILATIONS_RATE_SETTING}); } public void testIndexedScriptCountedInCompilationStats() throws IOException { buildScriptService(Settings.EMPTY); - ScriptContext ctx = randomFrom(contexts.values()); - scriptService.compile(new Script(ScriptType.STORED, null, "script", Collections.emptyMap()), ctx); + scriptService.compile(new Script(ScriptType.STORED, null, "script", Collections.emptyMap()), randomFrom(contexts.values())); assertEquals(1L, scriptService.stats().getCompilations()); - assertEquals(1L, scriptService.cacheStats().getContextStats().get(ctx.name).getCompilations()); + assertEquals(1L, scriptService.cacheStats().getGeneralStats().getCompilations()); } public void testCacheEvictionCountedInCacheEvictionsStats() 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"); 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())); @@ -261,8 +260,6 @@ public class ScriptServiceTests extends ESTestCase { assertEquals(2L, scriptService.cacheStats().getGeneralStats().getCompilations()); assertEquals(1L, scriptService.stats().getCacheEvictions()); assertEquals(1L, scriptService.cacheStats().getGeneralStats().getCacheEvictions()); - assertSettingDeprecationsAndWarnings(new Setting[]{SCRIPT_GENERAL_CACHE_SIZE_SETTING, - SCRIPT_GENERAL_MAX_COMPILATIONS_RATE_SETTING}); } public void testContextCacheStats() throws IOException { @@ -276,6 +273,7 @@ public class ScriptServiceTests extends ESTestCase { ".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) @@ -384,7 +382,6 @@ public class ScriptServiceTests extends ESTestCase { 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 " + @@ -394,7 +391,6 @@ public class ScriptServiceTests extends ESTestCase { 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()); }); @@ -405,7 +401,6 @@ public class ScriptServiceTests extends ESTestCase { 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()); }); @@ -419,8 +414,7 @@ public class ScriptServiceTests extends ESTestCase { .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()); - assertSettingDeprecationsAndWarnings(new Setting[]{SCRIPT_GENERAL_MAX_COMPILATIONS_RATE_SETTING}); + .put(ScriptService.SCRIPT_GENERAL_MAX_COMPILATIONS_RATE_SETTING.getKey(), ScriptService.USE_CONTEXT_RATE_KEY).build()); } public void testFallbackContextSettings() { @@ -435,7 +429,7 @@ public class ScriptServiceTests extends ESTestCase { 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_GENERAL_CACHE_EXPIRE_SETTING.getKey(), cacheExpireBackup) .put(ScriptService.SCRIPT_CACHE_EXPIRE_SETTING.getConcreteSettingForNamespace("foo").getKey(), cacheExpireFoo) .build(); @@ -444,7 +438,6 @@ public class ScriptServiceTests extends ESTestCase { 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[]{SCRIPT_GENERAL_CACHE_SIZE_SETTING, SCRIPT_GENERAL_CACHE_EXPIRE_SETTING}); } public void testUseContextSettingValue() { @@ -461,42 +454,67 @@ public class ScriptServiceTests extends ESTestCase { }); assertEquals("parameter must contain a positive integer and a timevalue, i.e. 10/1m, but was [use-context]", illegal.getMessage()); - assertSettingDeprecationsAndWarnings(new Setting[]{SCRIPT_GENERAL_MAX_COMPILATIONS_RATE_SETTING}); } - public void testCacheHolderGeneralConstructor() throws IOException { + public void testCacheHolderGeneralConstructor() { String compilationRate = "77/5m"; - buildScriptService( - Settings.builder().put(SCRIPT_GENERAL_MAX_COMPILATIONS_RATE_SETTING.getKey(), compilationRate).build() + Tuple rate = ScriptService.MAX_COMPILATION_RATE_FUNCTION.apply(compilationRate); + boolean compilationLimitsEnabled = true; + ScriptService.CacheHolder holder = new ScriptService.CacheHolder( + Settings.builder().put(SCRIPT_GENERAL_MAX_COMPILATIONS_RATE_SETTING.getKey(), compilationRate).build(), + Collections.unmodifiableSet(Collections.singleton(newContext("foo"))), + compilationLimitsEnabled ); - ScriptService.CacheHolder holder = scriptService.cacheHolder.get(); - assertNotNull(holder.general); assertNull(holder.contextCache); - assertEquals(holder.general.rate, new ScriptCache.CompilationRate(compilationRate)); - assertSettingDeprecationsAndWarnings(new Setting[]{SCRIPT_GENERAL_MAX_COMPILATIONS_RATE_SETTING}); + assertEquals(holder.general.rate, rate); + + compilationLimitsEnabled = false; + holder = new ScriptService.CacheHolder( + Settings.builder().put(SCRIPT_GENERAL_MAX_COMPILATIONS_RATE_SETTING.getKey(), compilationRate).build(), + new HashSet<>(Collections.singleton(newContext("foo"))), + compilationLimitsEnabled + ); + + assertNotNull(holder.general); + assertNull(holder.contextCache); + assertEquals(holder.general.rate, new Tuple<>(0, TimeValue.ZERO)); } - public void testCacheHolderContextConstructor() throws IOException { - String a = randomFrom(contexts.keySet()); - String b = randomValueOtherThan(a, () -> randomFrom(contexts.keySet())); - String aCompilationRate = "77/5m"; - String bCompilationRate = "78/6m"; + public void testCacheHolderContextConstructor() { + String fooCompilationRate = "77/5m"; + String barCompilationRate = "78/6m"; + boolean compilationLimitsEnabled = true; - buildScriptService(Settings.builder() - .put(SCRIPT_MAX_COMPILATIONS_RATE_SETTING.getConcreteSettingForNamespace(a).getKey(), aCompilationRate) - .put(SCRIPT_MAX_COMPILATIONS_RATE_SETTING.getConcreteSettingForNamespace(b).getKey(), bCompilationRate) - .build()); + Settings s = Settings.builder() + .put(SCRIPT_GENERAL_MAX_COMPILATIONS_RATE_SETTING.getKey(), ScriptService.USE_CONTEXT_RATE_KEY) + .put(SCRIPT_MAX_COMPILATIONS_RATE_SETTING.getConcreteSettingForNamespace("foo").getKey(), fooCompilationRate) + .put(SCRIPT_MAX_COMPILATIONS_RATE_SETTING.getConcreteSettingForNamespace("bar").getKey(), barCompilationRate) + .build(); + Collection> contexts = new HashSet<>(Arrays.asList(newContext("foo"), newContext("bar"), + newContext("baz"))); + ScriptService.CacheHolder holder = new ScriptService.CacheHolder(s, contexts, compilationLimitsEnabled); - assertNull(scriptService.cacheHolder.get().general); - assertNotNull(scriptService.cacheHolder.get().contextCache); - assertEquals(contexts.keySet(), scriptService.cacheHolder.get().contextCache.keySet()); + assertNull(holder.general); + assertNotNull(holder.contextCache); + assertEquals(3, holder.contextCache.size()); + assertEquals(contexts.stream().map(c -> c.name).collect(Collectors.toSet()), holder.contextCache.keySet()); - assertEquals(new ScriptCache.CompilationRate(aCompilationRate), - scriptService.cacheHolder.get().contextCache.get(a).get().rate); - assertEquals(new ScriptCache.CompilationRate(bCompilationRate), - scriptService.cacheHolder.get().contextCache.get(b).get().rate); + assertEquals(ScriptService.MAX_COMPILATION_RATE_FUNCTION.apply(fooCompilationRate), holder.contextCache.get("foo").get().rate); + assertEquals(ScriptService.MAX_COMPILATION_RATE_FUNCTION.apply(barCompilationRate), holder.contextCache.get("bar").get().rate); + assertEquals(ScriptService.SCRIPT_MAX_COMPILATIONS_RATE_SETTING.getDefault(Settings.EMPTY), + holder.contextCache.get("baz").get().rate); + + Tuple zero = new Tuple<>(0, TimeValue.ZERO); + compilationLimitsEnabled = false; + holder = new ScriptService.CacheHolder(s, contexts, compilationLimitsEnabled); + + assertNotNull(holder.contextCache); + assertEquals(3, holder.contextCache.size()); + assertEquals(zero, holder.contextCache.get("foo").get().rate); + assertEquals(zero, holder.contextCache.get("bar").get().rate); + assertEquals(zero, holder.contextCache.get("baz").get().rate); } public void testCompilationRateUnlimitedContextOnly() throws IOException { @@ -515,155 +533,155 @@ public class ScriptServiceTests extends ESTestCase { ScriptService.UNLIMITED_COMPILATION_RATE_KEY) .put(SCRIPT_GENERAL_MAX_COMPILATIONS_RATE_SETTING.getKey(), ScriptService.USE_CONTEXT_RATE_KEY) .build()); - assertSettingDeprecationsAndWarnings(new Setting[]{SCRIPT_GENERAL_MAX_COMPILATIONS_RATE_SETTING}); } - public void testDisableCompilationRateSetting() throws IOException { - IllegalArgumentException illegal = expectThrows(IllegalArgumentException.class, () -> { - buildScriptService(Settings.builder() - .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) - .build()); - }); - assertEquals("Cannot set custom context compilation rates [script.context.field.max_compilations_rate, " + - "script.context.ingest.max_compilations_rate] if compile rates disabled via " + - "[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()); - assertSettingDeprecationsAndWarnings(new Setting[]{SCRIPT_GENERAL_MAX_COMPILATIONS_RATE_SETTING}); - } - - public void testCacheHolderChangeSettings() throws IOException { - Set contextNames = contexts.keySet(); - String a = randomFrom(contextNames); - String aRate = "77/5m"; - String b = randomValueOtherThan(a, () -> randomFrom(contextNames)); - String bRate = "78/6m"; - String c = randomValueOtherThanMany(s -> a.equals(s) || b.equals(s), () -> randomFrom(contextNames)); + public void testCacheHolderChangeSettings() { + String fooCompilationRate = "77/5m"; + String barCompilationRate = "78/6m"; String compilationRate = "77/5m"; - ScriptCache.CompilationRate generalRate = new ScriptCache.CompilationRate(compilationRate); + Tuple generalRate = MAX_COMPILATION_RATE_FUNCTION.apply(compilationRate); Settings s = Settings.builder() .put(SCRIPT_GENERAL_MAX_COMPILATIONS_RATE_SETTING.getKey(), compilationRate) .build(); + Collection> contexts = new HashSet<>(Arrays.asList(newContext("foo"), newContext("bar"), + newContext("baz"), newContext("qux"))); + ScriptService.CacheHolder holder = new ScriptService.CacheHolder(s, contexts, true); - buildScriptService(s); + assertNotNull(holder.general); + assertNull(holder.contextCache); + assertEquals(generalRate, holder.general.rate); - 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() + holder = holder.withUpdatedCacheSettings(Settings.builder() + .put(SCRIPT_GENERAL_MAX_COMPILATIONS_RATE_SETTING.getKey(), ScriptService.USE_CONTEXT_RATE_KEY) + .put(SCRIPT_MAX_COMPILATIONS_RATE_SETTING.getConcreteSettingForNamespace("foo").getKey(), fooCompilationRate) + .put(SCRIPT_MAX_COMPILATIONS_RATE_SETTING.getConcreteSettingForNamespace("bar").getKey(), barCompilationRate) + .put(SCRIPT_MAX_COMPILATIONS_RATE_SETTING.getConcreteSettingForNamespace("qux").getKey(), + ScriptService.UNLIMITED_COMPILATION_RATE_KEY) + .build() ); - assertNull(scriptService.cacheHolder.get().general); - assertNotNull(scriptService.cacheHolder.get().contextCache); - // get of missing context should be null - assertNull(scriptService.cacheHolder.get().get( - randomValueOtherThanMany(contexts.keySet()::contains, () -> randomAlphaOfLength(8))) + assertNull(holder.general); + assertNotNull(holder.contextCache); + assertEquals(4, holder.contextCache.size()); + assertEquals(contexts.stream().map(c -> c.name).collect(Collectors.toSet()), holder.contextCache.keySet()); + + assertEquals(ScriptService.MAX_COMPILATION_RATE_FUNCTION.apply(fooCompilationRate), holder.contextCache.get("foo").get().rate); + assertEquals(ScriptService.MAX_COMPILATION_RATE_FUNCTION.apply(barCompilationRate), holder.contextCache.get("bar").get().rate); + assertEquals(ScriptCache.UNLIMITED_COMPILATION_RATE, holder.contextCache.get("qux").get().rate); + assertEquals(ScriptService.SCRIPT_MAX_COMPILATIONS_RATE_SETTING.getDefault(Settings.EMPTY), + holder.contextCache.get("baz").get().rate); + + holder.updateContextSettings(Settings.builder() + .put(SCRIPT_MAX_COMPILATIONS_RATE_SETTING.getConcreteSettingForNamespace("bar").getKey(), fooCompilationRate).build(), + newContext("bar") ); - assertEquals(contexts.keySet(), scriptService.cacheHolder.get().contextCache.keySet()); + assertEquals(ScriptService.MAX_COMPILATION_RATE_FUNCTION.apply(fooCompilationRate), holder.contextCache.get("bar").get().rate); - String d = randomValueOtherThanMany(Arrays.asList(a, b, c)::contains, () -> randomFrom(contextNames)); - assertEquals(new ScriptCache.CompilationRate(aRate), - scriptService.cacheHolder.get().contextCache.get(a).get().rate); - 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); + holder = holder.withUpdatedCacheSettings(s); + assertNotNull(holder.general); + assertNull(holder.contextCache); + assertEquals(generalRate, holder.general.rate); - scriptService.cacheHolder.get().set(b, scriptService.contextCache(Settings.builder() - .put(SCRIPT_MAX_COMPILATIONS_RATE_SETTING.getConcreteSettingForNamespace(b).getKey(), aRate).build(), - 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() + holder = holder.withUpdatedCacheSettings( + Settings.builder().put(SCRIPT_GENERAL_MAX_COMPILATIONS_RATE_SETTING.getKey(), barCompilationRate).build() ); - assertNotNull(scriptService.cacheHolder.get().general); - assertNull(scriptService.cacheHolder.get().contextCache); - assertEquals(new ScriptCache.CompilationRate(bRate), scriptService.cacheHolder.get().general.rate); + assertNotNull(holder.general); + assertNull(holder.contextCache); + assertEquals(MAX_COMPILATION_RATE_FUNCTION.apply(barCompilationRate), holder.general.rate); - ScriptService.CacheHolder holder = scriptService.cacheHolder.get(); - scriptService.setCacheHolder( - Settings.builder().put(SCRIPT_GENERAL_MAX_COMPILATIONS_RATE_SETTING.getKey(), bRate).build() + ScriptService.CacheHolder update = holder.withUpdatedCacheSettings( + Settings.builder().put(SCRIPT_GENERAL_MAX_COMPILATIONS_RATE_SETTING.getKey(), barCompilationRate).build() ); - assertEquals(holder, scriptService.cacheHolder.get()); - - assertSettingDeprecationsAndWarnings(new Setting[]{SCRIPT_GENERAL_MAX_COMPILATIONS_RATE_SETTING}); + assertSame(holder, update); } - public void testFallbackToContextDefaults() throws IOException { + public void testFallbackToContextDefaults() { + Tuple contextDefaultRate = new Tuple<>(randomIntBetween(10, 1024), + TimeValue.timeValueMinutes(randomIntBetween(10, 200))); + String name = "foo"; + ScriptContext foo = new ScriptContext<>(name, + ScriptContextTests.DummyScript.Factory.class, + randomIntBetween(1, 1024), + TimeValue.timeValueMinutes(randomIntBetween(10, 200)), + contextDefaultRate); + + + int generalCacheSize = randomIntBetween(1, 1024); + TimeValue generalExpire = TimeValue.timeValueMinutes(randomIntBetween(10, 200)); + String contextRateStr = randomIntBetween(10, 1024) + "/" + randomIntBetween(10, 200) + "m"; - ScriptCache.CompilationRate contextRate = new ScriptCache.CompilationRate(contextRateStr); + Tuple contextRate = MAX_COMPILATION_RATE_FUNCTION.apply(contextRateStr); int contextCacheSize = randomIntBetween(1, 1024); TimeValue contextExpire = TimeValue.timeValueMinutes(randomIntBetween(10, 200)); - buildScriptService( - Settings.builder().put(SCRIPT_GENERAL_MAX_COMPILATIONS_RATE_SETTING.getKey(), "75/5m").build() - ); - - String name = "ingest"; - // 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) - .build() - ); + ScriptService.CacheHolder contextCache = new ScriptService.CacheHolder( + Settings.builder() + .put(SCRIPT_CACHE_SIZE_SETTING.getConcreteSettingForNamespace(name).getKey(), contextCacheSize) + .put(SCRIPT_CACHE_EXPIRE_SETTING.getConcreteSettingForNamespace(name).getKey(), contextExpire) + .put(SCRIPT_MAX_COMPILATIONS_RATE_SETTING.getConcreteSettingForNamespace(name).getKey(), contextRateStr) + .put(SCRIPT_GENERAL_CACHE_SIZE_SETTING.getKey(), generalCacheSize) + .put(SCRIPT_GENERAL_CACHE_EXPIRE_SETTING.getKey(), generalExpire) + .put(SCRIPT_GENERAL_MAX_COMPILATIONS_RATE_SETTING.getKey(), USE_CONTEXT_RATE_KEY) + .build(), + Arrays.asList(foo), + true); - ScriptService.CacheHolder holder = scriptService.cacheHolder.get(); - assertNotNull(holder.contextCache); - assertNotNull(holder.contextCache.get(name)); - assertNotNull(holder.contextCache.get(name).get()); + assertNotNull(contextCache.contextCache); + assertNotNull(contextCache.contextCache.get(name)); + assertNotNull(contextCache.contextCache.get(name).get()); - assertEquals(contextRate, holder.contextCache.get(name).get().rate); - assertEquals(contextCacheSize, holder.contextCache.get(name).get().cacheSize); - assertEquals(contextExpire, holder.contextCache.get(name).get().cacheExpire); + assertEquals(contextRate, contextCache.contextCache.get(name).get().rate); + assertEquals(contextCacheSize, contextCache.contextCache.get(name).get().cacheSize); + assertEquals(contextExpire, contextCache.contextCache.get(name).get().cacheExpire); + + // Fallback to general + contextCache = new ScriptService.CacheHolder( + Settings.builder() + .put(SCRIPT_GENERAL_CACHE_SIZE_SETTING.getKey(), generalCacheSize) + .put(SCRIPT_GENERAL_CACHE_EXPIRE_SETTING.getKey(), generalExpire) + .put(SCRIPT_GENERAL_MAX_COMPILATIONS_RATE_SETTING.getKey(), USE_CONTEXT_RATE_KEY) + .build(), + Arrays.asList(foo), + true); + + assertNotNull(contextCache.contextCache); + assertNotNull(contextCache.contextCache.get(name)); + assertNotNull(contextCache.contextCache.get(name).get()); + + assertEquals(contextDefaultRate, contextCache.contextCache.get(name).get().rate); + assertEquals(generalCacheSize, contextCache.contextCache.get(name).get().cacheSize); + assertEquals(generalExpire, contextCache.contextCache.get(name).get().cacheExpire); - ScriptContext ingest = contexts.get(name); // Fallback to context defaults - buildScriptService(Settings.EMPTY); + contextCache = new ScriptService.CacheHolder( + Settings.builder() + .put(SCRIPT_GENERAL_MAX_COMPILATIONS_RATE_SETTING.getKey(), USE_CONTEXT_RATE_KEY) + .build(), + Arrays.asList(foo), + true); - holder = scriptService.cacheHolder.get(); - assertNotNull(holder.contextCache); - assertNotNull(holder.contextCache.get(name)); - assertNotNull(holder.contextCache.get(name).get()); + assertNotNull(contextCache.contextCache); + assertNotNull(contextCache.contextCache.get(name)); + assertNotNull(contextCache.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(contextDefaultRate, contextCache.contextCache.get(name).get().rate); + assertEquals(foo.cacheSizeDefault, contextCache.contextCache.get(name).get().cacheSize); + assertEquals(foo.cacheExpireDefault, contextCache.contextCache.get(name).get().cacheExpire); - assertSettingDeprecationsAndWarnings(new Setting[]{SCRIPT_GENERAL_MAX_COMPILATIONS_RATE_SETTING}); + // Use context specific for ingest + contextCache = new ScriptService.CacheHolder( + Settings.builder() + .put(SCRIPT_GENERAL_MAX_COMPILATIONS_RATE_SETTING.getKey(), USE_CONTEXT_RATE_KEY) + .build(), + Arrays.asList(foo, IngestScript.CONTEXT, IngestConditionalScript.CONTEXT), + true); + assertEquals(new Tuple<>(375, TimeValue.timeValueMinutes(5)), + contextCache.contextCache.get("ingest").get().rate); + assertEquals(200, contextCache.contextCache.get("ingest").get().cacheSize); + assertEquals(TimeValue.timeValueMillis(0), contextCache.contextCache.get("ingest").get().cacheExpire); } private void assertCompileRejected(String lang, String script, ScriptType scriptType, ScriptContext scriptContext) { @@ -682,4 +700,8 @@ public class ScriptServiceTests extends ESTestCase { notNullValue() ); } + + ScriptContext newContext(String name) { + return new ScriptContext<>(name, ScriptContextTests.DummyScript.Factory.class); + } } 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 9b87d3640d0..e8a26a05e23 100644 --- a/test/framework/src/main/java/org/elasticsearch/test/InternalTestCluster.java +++ b/test/framework/src/main/java/org/elasticsearch/test/InternalTestCluster.java @@ -105,7 +105,6 @@ import org.elasticsearch.node.Node; 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; @@ -520,14 +519,11 @@ public final class InternalTestCluster extends TestCluster { } if (random.nextBoolean()) { - String ctx = randomFrom(random, ScriptModule.CORE_CONTEXTS.keySet()); - builder.put(ScriptService.SCRIPT_CACHE_SIZE_SETTING.getConcreteSettingForNamespace(ctx).getKey(), - RandomNumbers.randomIntBetween(random, 0, 2000)); + 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(), - timeValueMillis(RandomNumbers.randomIntBetween(random, 750, 10000000)).getStringRep()); + builder.put(ScriptService.SCRIPT_GENERAL_CACHE_EXPIRE_SETTING.getKey(), + timeValueMillis(RandomNumbers.randomIntBetween(random, 750, 10000000)).getStringRep()); } return builder.build(); diff --git a/x-pack/plugin/ccr/src/test/java/org/elasticsearch/xpack/CcrIntegTestCase.java b/x-pack/plugin/ccr/src/test/java/org/elasticsearch/xpack/CcrIntegTestCase.java index 1e7dc8ba198..ec1236196ca 100644 --- a/x-pack/plugin/ccr/src/test/java/org/elasticsearch/xpack/CcrIntegTestCase.java +++ b/x-pack/plugin/ccr/src/test/java/org/elasticsearch/xpack/CcrIntegTestCase.java @@ -61,6 +61,7 @@ import org.elasticsearch.license.LicenseService; import org.elasticsearch.license.LicensesMetadata; import org.elasticsearch.persistent.PersistentTasksCustomMetadata; import org.elasticsearch.plugins.Plugin; +import org.elasticsearch.script.ScriptService; import org.elasticsearch.search.builder.SearchSourceBuilder; import org.elasticsearch.snapshots.RestoreInfo; import org.elasticsearch.snapshots.RestoreService; @@ -219,6 +220,7 @@ public abstract class CcrIntegTestCase extends ESTestCase { builder.put(DiskThresholdSettings.CLUSTER_ROUTING_ALLOCATION_LOW_DISK_WATERMARK_SETTING.getKey(), "1b"); builder.put(DiskThresholdSettings.CLUSTER_ROUTING_ALLOCATION_HIGH_DISK_WATERMARK_SETTING.getKey(), "1b"); builder.put(DiskThresholdSettings.CLUSTER_ROUTING_ALLOCATION_DISK_FLOOD_STAGE_WATERMARK_SETTING.getKey(), "1b"); + builder.put(ScriptService.SCRIPT_GENERAL_MAX_COMPILATIONS_RATE_SETTING.getKey(), "2048/1m"); // wait short time for other active shards before actually deleting, default 30s not needed in tests builder.put(IndicesStore.INDICES_STORE_DELETE_SHARD_TIMEOUT.getKey(), new TimeValue(1, TimeUnit.SECONDS)); builder.putList(DISCOVERY_SEED_HOSTS_SETTING.getKey()); // empty list disables a port scan for other nodes diff --git a/x-pack/plugin/deprecation/src/main/java/org/elasticsearch/xpack/deprecation/DeprecationChecks.java b/x-pack/plugin/deprecation/src/main/java/org/elasticsearch/xpack/deprecation/DeprecationChecks.java index 904288c9394..293badb81d9 100644 --- a/x-pack/plugin/deprecation/src/main/java/org/elasticsearch/xpack/deprecation/DeprecationChecks.java +++ b/x-pack/plugin/deprecation/src/main/java/org/elasticsearch/xpack/deprecation/DeprecationChecks.java @@ -51,9 +51,6 @@ public class DeprecationChecks { (settings, pluginsAndModules) -> NodeDeprecationChecks.checkThreadPoolListenerSize(settings), NodeDeprecationChecks::checkClusterRemoteConnectSetting, NodeDeprecationChecks::checkNodeLocalStorageSetting, - NodeDeprecationChecks::checkGeneralScriptSizeSetting, - NodeDeprecationChecks::checkGeneralScriptExpireSetting, - NodeDeprecationChecks::checkGeneralScriptCompileSettings, (settings, pluginsAndModules) -> NodeDeprecationChecks.checkNodeBasicLicenseFeatureEnabledSetting(settings, XPackSettings.ENRICH_ENABLED_SETTING), (settings, pluginsAndModules) -> NodeDeprecationChecks.checkNodeBasicLicenseFeatureEnabledSetting(settings, diff --git a/x-pack/plugin/deprecation/src/main/java/org/elasticsearch/xpack/deprecation/NodeDeprecationChecks.java b/x-pack/plugin/deprecation/src/main/java/org/elasticsearch/xpack/deprecation/NodeDeprecationChecks.java index 0e934382f8a..271fd56d087 100644 --- a/x-pack/plugin/deprecation/src/main/java/org/elasticsearch/xpack/deprecation/NodeDeprecationChecks.java +++ b/x-pack/plugin/deprecation/src/main/java/org/elasticsearch/xpack/deprecation/NodeDeprecationChecks.java @@ -12,7 +12,6 @@ import org.elasticsearch.common.settings.Settings; import org.elasticsearch.common.util.concurrent.EsExecutors; import org.elasticsearch.env.Environment; import org.elasticsearch.node.Node; -import org.elasticsearch.script.ScriptService; import org.elasticsearch.threadpool.FixedExecutorBuilder; import org.elasticsearch.transport.RemoteClusterService; import org.elasticsearch.xpack.core.deprecation.DeprecationIssue; @@ -146,39 +145,6 @@ class NodeDeprecationChecks { ); } - public static DeprecationIssue checkGeneralScriptSizeSetting(final Settings settings, final PluginsAndModules pluginsAndModules) { - return checkDeprecatedSetting( - settings, - pluginsAndModules, - ScriptService.SCRIPT_GENERAL_CACHE_SIZE_SETTING, - ScriptService.SCRIPT_CACHE_SIZE_SETTING, - "a script context", - "https://www.elastic.co/guide/en/elasticsearch/reference/7.9/breaking-changes-7.9.html#deprecate_general_script_cache_size" - ); - } - - public static DeprecationIssue checkGeneralScriptExpireSetting(final Settings settings, final PluginsAndModules pluginsAndModules) { - return checkDeprecatedSetting( - settings, - pluginsAndModules, - ScriptService.SCRIPT_GENERAL_CACHE_EXPIRE_SETTING, - ScriptService.SCRIPT_CACHE_EXPIRE_SETTING, - "a script context", - "https://www.elastic.co/guide/en/elasticsearch/reference/7.9/breaking-changes-7.9.html#deprecate_general_script_expire" - ); - } - - public static DeprecationIssue checkGeneralScriptCompileSettings(final Settings settings, final PluginsAndModules pluginsAndModules) { - return checkDeprecatedSetting( - settings, - pluginsAndModules, - ScriptService.SCRIPT_GENERAL_MAX_COMPILATIONS_RATE_SETTING, - ScriptService.SCRIPT_MAX_COMPILATIONS_RATE_SETTING, - "a script context", - "https://www.elastic.co/guide/en/elasticsearch/reference/7.9/breaking-changes-7.9.html#deprecate_general_script_compile_rate" - ); - } - private static DeprecationIssue checkDeprecatedSetting( final Settings settings, final PluginsAndModules pluginsAndModules, @@ -207,36 +173,6 @@ class NodeDeprecationChecks { return new DeprecationIssue(DeprecationIssue.Level.CRITICAL, message, url, details); } - private static DeprecationIssue checkDeprecatedSetting( - final Settings settings, - final PluginsAndModules pluginsAndModules, - final Setting deprecatedSetting, - final Setting.AffixSetting replacementSetting, - final String star, - final String url) { - assert deprecatedSetting.isDeprecated() : deprecatedSetting; - if (deprecatedSetting.exists(settings) == false) { - return null; - } - final String deprecatedSettingKey = deprecatedSetting.getKey(); - final String replacementSettingKey = replacementSetting.getKey(); - final String value = deprecatedSetting.get(settings).toString(); - final String message = String.format( - Locale.ROOT, - "setting [%s] is deprecated in favor of grouped setting [%s]", - deprecatedSettingKey, - replacementSettingKey); - final String details = String.format( - Locale.ROOT, - "the setting [%s] is currently set to [%s], instead set [%s] to [%s] where * is %s", - deprecatedSettingKey, - value, - replacementSettingKey, - value, - star); - return new DeprecationIssue(DeprecationIssue.Level.CRITICAL, message, url, details); - } - static DeprecationIssue checkRemovedSetting(final Settings settings, final Setting removedSetting, final String url) { if (removedSetting.exists(settings) == false) { return null; diff --git a/x-pack/plugin/deprecation/src/test/java/org/elasticsearch/xpack/deprecation/NodeDeprecationChecksTests.java b/x-pack/plugin/deprecation/src/test/java/org/elasticsearch/xpack/deprecation/NodeDeprecationChecksTests.java index 4a4d4ec4eac..c83554570de 100644 --- a/x-pack/plugin/deprecation/src/test/java/org/elasticsearch/xpack/deprecation/NodeDeprecationChecksTests.java +++ b/x-pack/plugin/deprecation/src/test/java/org/elasticsearch/xpack/deprecation/NodeDeprecationChecksTests.java @@ -13,7 +13,6 @@ import org.elasticsearch.common.settings.Settings; import org.elasticsearch.common.util.concurrent.EsExecutors; import org.elasticsearch.env.Environment; import org.elasticsearch.node.Node; -import org.elasticsearch.script.ScriptService; import org.elasticsearch.test.ESTestCase; import org.elasticsearch.transport.RemoteClusterService; import org.elasticsearch.xpack.core.XPackSettings; @@ -182,54 +181,6 @@ public class NodeDeprecationChecksTests extends ESTestCase { assertSettingDeprecationsAndWarnings(new String[]{"thread_pool.listener.size"}); } - public void testGeneralScriptSizeSetting() { - final int size = randomIntBetween(1, 4); - final Settings settings = Settings.builder().put("script.cache.max_size", size).build(); - final PluginsAndModules pluginsAndModules = new PluginsAndModules(Collections.emptyList(), Collections.emptyList()); - final List issues = - DeprecationChecks.filterChecks(DeprecationChecks.NODE_SETTINGS_CHECKS, c -> c.apply(settings, pluginsAndModules)); - final DeprecationIssue expected = new DeprecationIssue( - DeprecationIssue.Level.CRITICAL, - "setting [script.cache.max_size] is deprecated in favor of grouped setting [script.context.*.cache_max_size]", - "https://www.elastic.co/guide/en/elasticsearch/reference/7.9/breaking-changes-7.9.html#deprecate_general_script_cache_size", - "the setting [script.cache.max_size] is currently set to [" + size + "], instead set [script.context.*.cache_max_size] " + - "to [" + size + "] where * is a script context"); - assertThat(issues, contains(expected)); - assertSettingDeprecationsAndWarnings(new Setting[]{ScriptService.SCRIPT_GENERAL_CACHE_SIZE_SETTING}); - } - - public void testGeneralScriptExpireSetting() { - final String expire = randomIntBetween(1, 4) + "m"; - final Settings settings = Settings.builder().put("script.cache.expire", expire).build(); - final PluginsAndModules pluginsAndModules = new PluginsAndModules(Collections.emptyList(), Collections.emptyList()); - final List issues = - DeprecationChecks.filterChecks(DeprecationChecks.NODE_SETTINGS_CHECKS, c -> c.apply(settings, pluginsAndModules)); - final DeprecationIssue expected = new DeprecationIssue( - DeprecationIssue.Level.CRITICAL, - "setting [script.cache.expire] is deprecated in favor of grouped setting [script.context.*.cache_expire]", - "https://www.elastic.co/guide/en/elasticsearch/reference/7.9/breaking-changes-7.9.html#deprecate_general_script_expire", - "the setting [script.cache.expire] is currently set to [" + expire + "], instead set [script.context.*.cache_expire] to " + - "[" + expire + "] where * is a script context"); - assertThat(issues, contains(expected)); - assertSettingDeprecationsAndWarnings(new Setting[]{ScriptService.SCRIPT_GENERAL_CACHE_EXPIRE_SETTING}); - } - - public void testGeneralScriptCompileSettings() { - final String rate = randomIntBetween(1, 100) + "/" + randomIntBetween(1, 200) + "m"; - final Settings settings = Settings.builder().put("script.max_compilations_rate", rate).build(); - final PluginsAndModules pluginsAndModules = new PluginsAndModules(Collections.emptyList(), Collections.emptyList()); - final List issues = - DeprecationChecks.filterChecks(DeprecationChecks.NODE_SETTINGS_CHECKS, c -> c.apply(settings, pluginsAndModules)); - final DeprecationIssue expected = new DeprecationIssue( - DeprecationIssue.Level.CRITICAL, - "setting [script.max_compilations_rate] is deprecated in favor of grouped setting [script.context.*.max_compilations_rate]", - "https://www.elastic.co/guide/en/elasticsearch/reference/7.9/breaking-changes-7.9.html#deprecate_general_script_compile_rate", - "the setting [script.max_compilations_rate] is currently set to [" + rate + - "], instead set [script.context.*.max_compilations_rate] to [" + rate + "] where * is a script context"); - assertThat(issues, contains(expected)); - assertSettingDeprecationsAndWarnings(new Setting[]{ScriptService.SCRIPT_GENERAL_MAX_COMPILATIONS_RATE_SETTING}); - } - public void testClusterRemoteConnectSetting() { final boolean value = randomBoolean(); final Settings settings = Settings.builder().put(RemoteClusterService.ENABLE_REMOTE_CLUSTERS.getKey(), value).build(); @@ -321,4 +272,5 @@ public class NodeDeprecationChecksTests extends ESTestCase { equalTo("the setting [node.removed_setting] is currently set to [value], remove this setting")); assertThat(issue.getUrl(), equalTo("https://removed-setting.example.com")); } + }