From 20abba8433daf78075ec7f9a84747170128112ef Mon Sep 17 00:00:00 2001 From: Stuart Tettemer Date: Thu, 18 Jun 2020 11:54:23 -0600 Subject: [PATCH] Scripting: Deprecate general cache settings (#55753) (#58283) Backport: ef543b0 --- .../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, 629 insertions(+), 390 deletions(-) create mode 100644 docs/reference/migration/migrate_7_9.asciidoc create 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 04c49d24d68..330e112c3b2 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,7 +420,11 @@ class ClusterFormationTasks { esConfig['cluster.routing.allocation.disk.watermark.flood_stage'] = '1b' } // increase script compilation limit since tests can rapid-fire script compilations - esConfig['script.max_compilations_rate'] = '2048/1m' + if (node.nodeVersion.onOrAfter('7.9.0')) { + esConfig['script.disable_max_compilations_rate'] = 'true' + } else { + 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 b8cf207694e..0c057168598 100644 --- a/buildSrc/src/main/java/org/elasticsearch/gradle/testclusters/ElasticsearchNode.java +++ b/buildSrc/src/main/java/org/elasticsearch/gradle/testclusters/ElasticsearchNode.java @@ -1093,7 +1093,11 @@ 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 - baseConfig.put("script.max_compilations_rate", "2048/1m"); + 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"); + } 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 ec6730f6dd5..87d1309f5d4 100644 --- a/distribution/docker/docker-compose.yml +++ b/distribution/docker/docker-compose.yml @@ -15,7 +15,6 @@ 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 @@ -55,7 +54,6 @@ 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 @@ -95,7 +93,6 @@ 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 @@ -120,7 +117,6 @@ 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 1895e563bc0..2b72fbb7e85 100644 --- a/docs/reference/migration/index.asciidoc +++ b/docs/reference/migration/index.asciidoc @@ -11,6 +11,7 @@ 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 new file mode 100644 index 00000000000..1e4866092b5 --- /dev/null +++ b/docs/reference/migration/migrate_7_9.asciidoc @@ -0,0 +1,71 @@ +[[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 4b1351abe82..36fd5fc495f 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.max_compilations_rate`:: +`script.context.$CONTEXT.max_compilations_rate`:: Limit for the number of unique dynamic scripts within a certain interval - that are allowed to be compiled. Defaults to 75/5m, meaning 75 every 5 - minutes. + that are allowed to be compiled for a given context. Defaults to `75/5m`, + meaning 75 every 5 minutes. diff --git a/docs/reference/release-notes.asciidoc b/docs/reference/release-notes.asciidoc index f18a1d2a1d2..5bf4bd0cb4e 100644 --- a/docs/reference/release-notes.asciidoc +++ b/docs/reference/release-notes.asciidoc @@ -6,6 +6,7 @@ This section summarizes the changes in each release. +* <> * <> * <> * <> @@ -34,6 +35,7 @@ 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 new file mode 100644 index 00000000000..2ab14c90a6e --- /dev/null +++ b/docs/reference/release-notes/7.9.asciidoc @@ -0,0 +1,11 @@ +[[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 3e26882c281..d0aa95cd4c0 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 abbec0a5565..4e4f0126179 100644 --- a/docs/reference/scripting/using.asciidoc +++ b/docs/reference/scripting/using.asciidoc @@ -98,9 +98,11 @@ 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 15 inline scripts per -minute will be compiled. You can change this setting dynamically by setting -`script.max_compilations_rate`. +`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`. ======================================== @@ -135,7 +137,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` @@ -241,9 +243,17 @@ 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 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`. +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 +|==== 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 5cd891373c5..1a77d1bc999 100644 --- a/qa/remote-clusters/docker-compose-oss.yml +++ b/qa/remote-clusters/docker-compose-oss.yml @@ -15,7 +15,6 @@ 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 @@ -50,7 +49,6 @@ 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 05af5541785..51bf9c8d050 100644 --- a/qa/remote-clusters/docker-compose.yml +++ b/qa/remote-clusters/docker-compose.yml @@ -15,7 +15,6 @@ 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 @@ -65,7 +64,6 @@ 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 5b910cf706f..e53ed54864b 100644 --- a/server/src/main/java/org/elasticsearch/common/settings/ClusterSettings.java +++ b/server/src/main/java/org/elasticsearch/common/settings/ClusterSettings.java @@ -415,6 +415,7 @@ 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 5d59a91b207..aa2c16ec9b7 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,14 +51,14 @@ 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, + CompilationRate 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.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 @@ 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.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 @@ public class ScriptCache { 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,77 @@ 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 4f37beac8fe..f78ab24b75e 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.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; @@ -47,7 +46,6 @@ 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; @@ -68,51 +66,19 @@ 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); + 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); + 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 = - 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); + 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); // Per-context settings static final String CONTEXT_PREFIX = "script.context."; @@ -133,15 +99,18 @@ 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); public static final String ALLOW_NONE = "none"; @@ -160,7 +129,8 @@ public class ScriptService implements Closeable, ClusterStateApplier { private int maxSizeInBytes; - private final AtomicReference cacheHolder; + // package private for tests + final AtomicReference cacheHolder = new AtomicReference<>(); public ScriptService(Settings settings, Map engines, Map> contexts) { this.engines = Objects.requireNonNull(engines); @@ -243,7 +213,7 @@ public class ScriptService implements Closeable, ClusterStateApplier { // Validation requires knowing which contexts exist. this.validateCacheSettings(settings); - cacheHolder = new AtomicReference<>(new CacheHolder(settings, contexts.values(), compilationLimitsEnabled())); + this.setCacheHolder(settings); } /** @@ -259,23 +229,25 @@ 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().updateContextSettings(settings, context), + (settings) -> cacheHolder.get().set(context.name, contextCache(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) -> cacheHolder.set(cacheHolder.get().withUpdatedCacheSettings(settings)), + (settings) -> setCacheHolder(settings), Arrays.asList(SCRIPT_GENERAL_MAX_COMPILATIONS_RATE_SETTING, SCRIPT_GENERAL_CACHE_EXPIRE_SETTING, SCRIPT_GENERAL_CACHE_SIZE_SETTING, SCRIPT_MAX_COMPILATIONS_RATE_SETTING, + SCRIPT_DISABLE_MAX_COMPILATIONS_RATE_SETTING, SCRIPT_CACHE_EXPIRE_SETTING, SCRIPT_CACHE_SIZE_SETTING), this::validateCacheSettings @@ -290,31 +262,39 @@ 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) { - keys.addAll(getConcreteSettingKeys(affix, settings)); + 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); + } + } } 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 + "]"); } - } - - /** - * 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 + "]"); + 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() + "]"); } - concreteKeys.add(s); } - concreteKeys.sort(Comparator.naturalOrder()); - return concreteKeys; } @Override @@ -413,7 +393,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); } @@ -577,6 +557,67 @@ 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. @@ -586,80 +627,16 @@ public class ScriptService implements Closeable, ClusterStateApplier { final ScriptCache general; final Map> contextCache; - 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(int cacheMaxSize, TimeValue cacheExpire, ScriptCache.CompilationRate maxCompilationRate, String contextRateSetting) { + contextCache = null; + general = new ScriptCache(cacheMaxSize, cacheExpire, maxCompilationRate, contextRateSetting); } - /** - * 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; + 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; } /** @@ -689,25 +666,25 @@ public class ScriptService implements Closeable, ClusterStateApplier { return new ScriptCacheStats(general.stats()); } Map context = new HashMap<>(contextCache.size()); - for (ScriptContext ctx: contexts) { - context.put(ctx.name, contextCache.get(ctx.name).get().stats()); + for (String name: contextCache.keySet()) { + context.put(name, contextCache.get(name).get().stats()); } return new ScriptCacheStats(context); } /** - * Update settings for the context cache, if we're in the context cache mode otherwise no-op. + * Update a single context cache if we're in the context cache mode otherwise no-op. */ - void updateContextSettings(Settings settings, ScriptContext context) { + void set(String name, ScriptCache cache) { if (general != null) { return; } - 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"); + 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"); } } } diff --git a/server/src/test/java/org/elasticsearch/indices/IndicesServiceCloseTests.java b/server/src/test/java/org/elasticsearch/indices/IndicesServiceCloseTests.java index 92520d6bf97..1ef1ccb8829 100644 --- a/server/src/test/java/org/elasticsearch/indices/IndicesServiceCloseTests.java +++ b/server/src/test/java/org/elasticsearch/indices/IndicesServiceCloseTests.java @@ -39,7 +39,6 @@ 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; @@ -69,7 +68,6 @@ 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 6c53624ab0c..281adc7cdd7 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,25 @@ 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); + ScriptCache.CompilationRate 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 5dc3b5756f0..820f7e5de1e 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,23 +38,19 @@ 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_CACHE_SIZE_SETTING; import static org.elasticsearch.script.ScriptService.SCRIPT_GENERAL_MAX_COMPILATIONS_RATE_SETTING; +import static org.elasticsearch.script.ScriptService.SCRIPT_GENERAL_CACHE_SIZE_SETTING; import static org.elasticsearch.script.ScriptService.SCRIPT_MAX_COMPILATIONS_RATE_SETTING; -import static org.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; @@ -110,8 +106,8 @@ public class ScriptServiceTests extends ESTestCase { } 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]"); @@ -131,7 +127,7 @@ public class ScriptServiceTests extends ESTestCase { } 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)); } @@ -233,26 +229,31 @@ public class ScriptServiceTests extends ESTestCase { } public void testCompilationStatsOnCacheHit() throws IOException { - Settings.Builder builder = Settings.builder(); - builder.put(SCRIPT_GENERAL_CACHE_SIZE_SETTING.getKey(), 1); + Settings.Builder builder = Settings.builder() + .put(SCRIPT_GENERAL_CACHE_SIZE_SETTING.getKey(), 1) + .put(SCRIPT_GENERAL_MAX_COMPILATIONS_RATE_SETTING.getKey(), "2/1m"); buildScriptService(builder.build()); Script script = new Script(ScriptType.INLINE, "test", "1+1", Collections.emptyMap()); ScriptContext context = randomFrom(contexts.values()); scriptService.compile(script, context); 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); - scriptService.compile(new Script(ScriptType.STORED, null, "script", Collections.emptyMap()), randomFrom(contexts.values())); + ScriptContext ctx = randomFrom(contexts.values()); + scriptService.compile(new Script(ScriptType.STORED, null, "script", Collections.emptyMap()), ctx); assertEquals(1L, scriptService.stats().getCompilations()); - assertEquals(1L, scriptService.cacheStats().getGeneralStats().getCompilations()); + assertEquals(1L, scriptService.cacheStats().getContextStats().get(ctx.name).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())); @@ -260,6 +261,8 @@ 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 { @@ -273,7 +276,6 @@ 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) @@ -382,6 +384,7 @@ 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 " + @@ -391,6 +394,7 @@ 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()); }); @@ -401,6 +405,7 @@ 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()); }); @@ -414,7 +419,8 @@ 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") - .put(ScriptService.SCRIPT_GENERAL_MAX_COMPILATIONS_RATE_SETTING.getKey(), ScriptService.USE_CONTEXT_RATE_KEY).build()); + .build()); + assertSettingDeprecationsAndWarnings(new Setting[]{SCRIPT_GENERAL_MAX_COMPILATIONS_RATE_SETTING}); } public void testFallbackContextSettings() { @@ -429,7 +435,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(ScriptService.SCRIPT_GENERAL_CACHE_EXPIRE_SETTING.getKey(), cacheExpireBackup) + .put(SCRIPT_GENERAL_CACHE_EXPIRE_SETTING.getKey(), cacheExpireBackup) .put(ScriptService.SCRIPT_CACHE_EXPIRE_SETTING.getConcreteSettingForNamespace("foo").getKey(), cacheExpireFoo) .build(); @@ -438,6 +444,7 @@ 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() { @@ -454,67 +461,42 @@ 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() { + public void testCacheHolderGeneralConstructor() throws IOException { String compilationRate = "77/5m"; - 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 + buildScriptService( + Settings.builder().put(SCRIPT_GENERAL_MAX_COMPILATIONS_RATE_SETTING.getKey(), compilationRate).build() ); + ScriptService.CacheHolder holder = scriptService.cacheHolder.get(); + assertNotNull(holder.general); assertNull(holder.contextCache); - assertEquals(holder.general.rate, 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)); + assertEquals(holder.general.rate, new ScriptCache.CompilationRate(compilationRate)); + assertSettingDeprecationsAndWarnings(new Setting[]{SCRIPT_GENERAL_MAX_COMPILATIONS_RATE_SETTING}); } - public void testCacheHolderContextConstructor() { - String fooCompilationRate = "77/5m"; - String barCompilationRate = "78/6m"; - boolean compilationLimitsEnabled = true; + 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"; - 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); + buildScriptService(Settings.builder() + .put(SCRIPT_MAX_COMPILATIONS_RATE_SETTING.getConcreteSettingForNamespace(a).getKey(), aCompilationRate) + .put(SCRIPT_MAX_COMPILATIONS_RATE_SETTING.getConcreteSettingForNamespace(b).getKey(), bCompilationRate) + .build()); - assertNull(holder.general); - assertNotNull(holder.contextCache); - assertEquals(3, holder.contextCache.size()); - assertEquals(contexts.stream().map(c -> c.name).collect(Collectors.toSet()), holder.contextCache.keySet()); + assertNull(scriptService.cacheHolder.get().general); + assertNotNull(scriptService.cacheHolder.get().contextCache); + assertEquals(contexts.keySet(), scriptService.cacheHolder.get().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(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); + 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); } public void testCompilationRateUnlimitedContextOnly() throws IOException { @@ -533,155 +515,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 testCacheHolderChangeSettings() { - String fooCompilationRate = "77/5m"; - String barCompilationRate = "78/6m"; + 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)); 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) .build(); - Collection> contexts = new HashSet<>(Arrays.asList(newContext("foo"), newContext("bar"), - newContext("baz"), newContext("qux"))); - ScriptService.CacheHolder holder = new ScriptService.CacheHolder(s, contexts, true); - assertNotNull(holder.general); - assertNull(holder.contextCache); - assertEquals(generalRate, holder.general.rate); + buildScriptService(s); - 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() + assertNotNull(scriptService.cacheHolder.get().general); + // Set should not throw when using general cache + scriptService.cacheHolder.get().set(c, scriptService.contextCache(s, contexts.get(c))); + assertNull(scriptService.cacheHolder.get().contextCache); + assertEquals(generalRate, scriptService.cacheHolder.get().general.rate); + + scriptService.setCacheHolder(Settings.builder() + .put(SCRIPT_MAX_COMPILATIONS_RATE_SETTING.getConcreteSettingForNamespace(a).getKey(), aRate) + .put(SCRIPT_MAX_COMPILATIONS_RATE_SETTING.getConcreteSettingForNamespace(b).getKey(), bRate) + .put(SCRIPT_MAX_COMPILATIONS_RATE_SETTING.getConcreteSettingForNamespace(c).getKey(), + ScriptService.UNLIMITED_COMPILATION_RATE_KEY) + .build() ); - assertNull(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") + 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))) ); - assertEquals(ScriptService.MAX_COMPILATION_RATE_FUNCTION.apply(fooCompilationRate), holder.contextCache.get("bar").get().rate); + assertEquals(contexts.keySet(), scriptService.cacheHolder.get().contextCache.keySet()); - holder = holder.withUpdatedCacheSettings(s); - assertNotNull(holder.general); - assertNull(holder.contextCache); - assertEquals(generalRate, holder.general.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( - Settings.builder().put(SCRIPT_GENERAL_MAX_COMPILATIONS_RATE_SETTING.getKey(), barCompilationRate).build() + 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() ); - assertNotNull(holder.general); - assertNull(holder.contextCache); - assertEquals(MAX_COMPILATION_RATE_FUNCTION.apply(barCompilationRate), holder.general.rate); + assertNotNull(scriptService.cacheHolder.get().general); + assertNull(scriptService.cacheHolder.get().contextCache); + assertEquals(new ScriptCache.CompilationRate(bRate), scriptService.cacheHolder.get().general.rate); - ScriptService.CacheHolder update = holder.withUpdatedCacheSettings( - Settings.builder().put(SCRIPT_GENERAL_MAX_COMPILATIONS_RATE_SETTING.getKey(), barCompilationRate).build() + ScriptService.CacheHolder holder = scriptService.cacheHolder.get(); + scriptService.setCacheHolder( + Settings.builder().put(SCRIPT_GENERAL_MAX_COMPILATIONS_RATE_SETTING.getKey(), bRate).build() ); - assertSame(holder, update); + assertEquals(holder, scriptService.cacheHolder.get()); + + assertSettingDeprecationsAndWarnings(new Setting[]{SCRIPT_GENERAL_MAX_COMPILATIONS_RATE_SETTING}); } - 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)); - + 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)); + buildScriptService( + Settings.builder().put(SCRIPT_GENERAL_MAX_COMPILATIONS_RATE_SETTING.getKey(), "75/5m").build() + ); + + String name = "ingest"; + // Use context specific - ScriptService.CacheHolder contextCache = new ScriptService.CacheHolder( - Settings.builder() - .put(SCRIPT_CACHE_SIZE_SETTING.getConcreteSettingForNamespace(name).getKey(), contextCacheSize) - .put(SCRIPT_CACHE_EXPIRE_SETTING.getConcreteSettingForNamespace(name).getKey(), contextExpire) - .put(SCRIPT_MAX_COMPILATIONS_RATE_SETTING.getConcreteSettingForNamespace(name).getKey(), contextRateStr) - .put(SCRIPT_GENERAL_CACHE_SIZE_SETTING.getKey(), generalCacheSize) - .put(SCRIPT_GENERAL_CACHE_EXPIRE_SETTING.getKey(), generalExpire) - .put(SCRIPT_GENERAL_MAX_COMPILATIONS_RATE_SETTING.getKey(), USE_CONTEXT_RATE_KEY) - .build(), - Arrays.asList(foo), - true); + 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() + ); - assertNotNull(contextCache.contextCache); - assertNotNull(contextCache.contextCache.get(name)); - assertNotNull(contextCache.contextCache.get(name).get()); + ScriptService.CacheHolder holder = scriptService.cacheHolder.get(); + assertNotNull(holder.contextCache); + assertNotNull(holder.contextCache.get(name)); + assertNotNull(holder.contextCache.get(name).get()); - assertEquals(contextRate, contextCache.contextCache.get(name).get().rate); - assertEquals(contextCacheSize, contextCache.contextCache.get(name).get().cacheSize); - assertEquals(contextExpire, contextCache.contextCache.get(name).get().cacheExpire); - - // Fallback to general - contextCache = new ScriptService.CacheHolder( - Settings.builder() - .put(SCRIPT_GENERAL_CACHE_SIZE_SETTING.getKey(), generalCacheSize) - .put(SCRIPT_GENERAL_CACHE_EXPIRE_SETTING.getKey(), generalExpire) - .put(SCRIPT_GENERAL_MAX_COMPILATIONS_RATE_SETTING.getKey(), USE_CONTEXT_RATE_KEY) - .build(), - 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); + assertEquals(contextRate, holder.contextCache.get(name).get().rate); + assertEquals(contextCacheSize, holder.contextCache.get(name).get().cacheSize); + assertEquals(contextExpire, holder.contextCache.get(name).get().cacheExpire); + ScriptContext ingest = contexts.get(name); // Fallback to context defaults - contextCache = new ScriptService.CacheHolder( - Settings.builder() - .put(SCRIPT_GENERAL_MAX_COMPILATIONS_RATE_SETTING.getKey(), USE_CONTEXT_RATE_KEY) - .build(), - Arrays.asList(foo), - true); + buildScriptService(Settings.EMPTY); - assertNotNull(contextCache.contextCache); - assertNotNull(contextCache.contextCache.get(name)); - assertNotNull(contextCache.contextCache.get(name).get()); + holder = scriptService.cacheHolder.get(); + assertNotNull(holder.contextCache); + assertNotNull(holder.contextCache.get(name)); + assertNotNull(holder.contextCache.get(name).get()); - assertEquals(contextDefaultRate, contextCache.contextCache.get(name).get().rate); - assertEquals(foo.cacheSizeDefault, contextCache.contextCache.get(name).get().cacheSize); - assertEquals(foo.cacheExpireDefault, contextCache.contextCache.get(name).get().cacheExpire); + 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); - // 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); + assertSettingDeprecationsAndWarnings(new Setting[]{SCRIPT_GENERAL_MAX_COMPILATIONS_RATE_SETTING}); } private void assertCompileRejected(String lang, String script, ScriptType scriptType, ScriptContext scriptContext) { @@ -700,8 +682,4 @@ 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 ac1a330e6cc..f8640506949 100644 --- a/test/framework/src/main/java/org/elasticsearch/test/InternalTestCluster.java +++ b/test/framework/src/main/java/org/elasticsearch/test/InternalTestCluster.java @@ -106,6 +106,7 @@ 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,11 +521,14 @@ public final class InternalTestCluster extends TestCluster { } if (random.nextBoolean()) { - builder.put(ScriptService.SCRIPT_GENERAL_CACHE_SIZE_SETTING.getKey(), RandomNumbers.randomIntBetween(random, 0, 2000)); + String ctx = randomFrom(random, ScriptModule.CORE_CONTEXTS.keySet()); + builder.put(ScriptService.SCRIPT_CACHE_SIZE_SETTING.getConcreteSettingForNamespace(ctx).getKey(), + RandomNumbers.randomIntBetween(random, 0, 2000)); } if (random.nextBoolean()) { - builder.put(ScriptService.SCRIPT_GENERAL_CACHE_EXPIRE_SETTING.getKey(), - timeValueMillis(RandomNumbers.randomIntBetween(random, 750, 10000000)).getStringRep()); + 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()); } if (random.nextBoolean()) { int initialMillisBound = RandomNumbers.randomIntBetween(random,10, 100); 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 ec1236196ca..1e7dc8ba198 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,7 +61,6 @@ 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; @@ -220,7 +219,6 @@ 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 293badb81d9..904288c9394 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,6 +51,9 @@ 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 271fd56d087..0e934382f8a 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,6 +12,7 @@ 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; @@ -145,6 +146,39 @@ 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, @@ -173,6 +207,36 @@ 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 c83554570de..4a4d4ec4eac 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,6 +13,7 @@ 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; @@ -181,6 +182,54 @@ 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(); @@ -272,5 +321,4 @@ 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")); } - }