Skip to content

Commit 8601c9b

Browse files
stu-elasticstu-elastic
authored
Script: Opt-out system contexts from script compilation rate limit (#79459)
System script contexts can now opt-out of compilation rate limiting using a flag rather than a sentinel rate limit value. Duplicated defaults for system contexts have been coalesced. Refs: #62899
1 parent 679beda commit 8601c9b

File tree

13 files changed

+116
-42
lines changed

13 files changed

+116
-42
lines changed

‎server/src/main/java/org/elasticsearch/script/AbstractFieldScript.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -52,7 +52,7 @@ static <F> ScriptContext<F> newContext(String name, Class<F> factoryClass) {
5252
* source of runaway script compilations. We think folks will
5353
* mostly reuse scripts though.
5454
*/
55-
ScriptCache.UNLIMITED_COMPILATION_RATE.asTuple(),
55+
false,
5656
/*
5757
* Disable runtime fields scripts from being allowed
5858
* to be stored as part of the script meta data.

‎server/src/main/java/org/elasticsearch/script/IngestConditionalScript.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -21,7 +21,7 @@ public abstract class IngestConditionalScript {
2121

2222
/** The context used to compile {@link IngestConditionalScript} factories. */
2323
public static final ScriptContext<Factory> CONTEXT = new ScriptContext<>("processor_conditional", Factory.class,
24-
200, TimeValue.timeValueMillis(0), ScriptCache.UNLIMITED_COMPILATION_RATE.asTuple(), true);
24+
200, TimeValue.timeValueMillis(0), false, true);
2525

2626
/** The generic runtime parameters for the script. */
2727
private final Map<String, Object> params;

‎server/src/main/java/org/elasticsearch/script/IngestScript.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,7 @@ public abstract class IngestScript {
2222

2323
/** The context used to compile {@link IngestScript} factories. */
2424
public static final ScriptContext<Factory> CONTEXT = new ScriptContext<>("ingest", Factory.class,
25-
200, TimeValue.timeValueMillis(0), ScriptCache.UNLIMITED_COMPILATION_RATE.asTuple(), true);
25+
200, TimeValue.timeValueMillis(0), false, true);
2626

2727
/** The generic runtime parameters for the script. */
2828
private final Map<String, Object> params;

‎server/src/main/java/org/elasticsearch/script/ScriptCache.java‎

Lines changed: 5 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -44,12 +44,7 @@ public class ScriptCache {
4444
private final double compilesAllowedPerNano;
4545
private final String contextRateSetting;
4646

47-
ScriptCache(
48-
int cacheMaxSize,
49-
TimeValue cacheExpire,
50-
CompilationRate maxCompilationRate,
51-
String contextRateSetting
52-
) {
47+
ScriptCache(int cacheMaxSize, TimeValue cacheExpire, CompilationRate maxCompilationRate, String contextRateSetting) {
5348
this.cacheSize = cacheMaxSize;
5449
this.cacheExpire = cacheExpire;
5550
this.contextRateSetting = contextRateSetting;
@@ -94,8 +89,10 @@ <FactoryType> FactoryType compile(
9489
logger.trace("context [{}]: compiling script, type: [{}], lang: [{}], options: [{}]", context.name, type,
9590
lang, options);
9691
}
97-
// Check whether too many compilations have happened
98-
checkCompilationLimit();
92+
if (context.compilationRateLimited) {
93+
// Check whether too many compilations have happened
94+
checkCompilationLimit();
95+
}
9996
Object compiledScript = scriptEngine.compile(id, idOrCode, context, options);
10097
// Since the cache key is the script content itself we don't need to
10198
// invalidate/check the cache if an indexed script changes.

‎server/src/main/java/org/elasticsearch/script/ScriptCacheStats.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -21,7 +21,7 @@
2121
import java.util.Objects;
2222
import java.util.stream.Collectors;
2323

24-
// This class is deprecated in favor of ScriptStats and ScriptContextStats. It is removed in 8.
24+
// This class is deprecated in favor of ScriptStats and ScriptContextStats
2525
public class ScriptCacheStats implements Writeable, ToXContentFragment {
2626
private final Map<String, ScriptStats> context;
2727
private final ScriptStats general;

‎server/src/main/java/org/elasticsearch/script/ScriptContext.java‎

Lines changed: 8 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -47,6 +47,8 @@
4747
* be {@code boolean needs_score()}.
4848
*/
4949
public final class ScriptContext<FactoryType> {
50+
/** The default compilation rate limit for contexts with compilation rate limiting enabled */
51+
public static final Tuple<Integer, TimeValue> DEFAULT_COMPILATION_RATE_LIMIT = new Tuple<>(150, TimeValue.timeValueMinutes(5));
5052

5153
/** A unique identifier for this context. */
5254
public final String name;
@@ -66,15 +68,15 @@ public final class ScriptContext<FactoryType> {
6668
/** The default expiration of a script in the cache for the context, if not overridden */
6769
public final TimeValue cacheExpireDefault;
6870

69-
/** The default max compilation rate for scripts in this context. Script compilation is throttled if this is exceeded */
70-
public final Tuple<Integer, TimeValue> maxCompilationRateDefault;
71+
/** Is compilation rate limiting enabled for this context? */
72+
public final boolean compilationRateLimited;
7173

7274
/** Determines if the script can be stored as part of the cluster state. */
7375
public final boolean allowStoredScript;
7476

7577
/** Construct a context with the related instance and compiled classes with caller provided cache defaults */
7678
public ScriptContext(String name, Class<FactoryType> factoryClazz, int cacheSizeDefault, TimeValue cacheExpireDefault,
77-
Tuple<Integer, TimeValue> maxCompilationRateDefault, boolean allowStoredScript) {
79+
boolean compilationRateLimited, boolean allowStoredScript) {
7880
this.name = name;
7981
this.factoryClazz = factoryClazz;
8082
Method newInstanceMethod = findMethod("FactoryType", factoryClazz, "newInstance");
@@ -98,15 +100,15 @@ public ScriptContext(String name, Class<FactoryType> factoryClazz, int cacheSize
98100

99101
this.cacheSizeDefault = cacheSizeDefault;
100102
this.cacheExpireDefault = cacheExpireDefault;
101-
this.maxCompilationRateDefault = maxCompilationRateDefault;
103+
this.compilationRateLimited = compilationRateLimited;
102104
this.allowStoredScript = allowStoredScript;
103105
}
104106

105107
/** Construct a context with the related instance and compiled classes with defaults for cacheSizeDefault, cacheExpireDefault and
106-
* maxCompilationRateDefault and allow scripts of this context to be stored scripts */
108+
* compilationRateLimited and allow scripts of this context to be stored scripts */
107109
public ScriptContext(String name, Class<FactoryType> factoryClazz) {
108110
// cache size default, cache expire default, max compilation rate are defaults from ScriptService.
109-
this(name, factoryClazz, 100, TimeValue.timeValueMillis(0), new Tuple<>(75, TimeValue.timeValueMinutes(5)), true);
111+
this(name, factoryClazz, 100, TimeValue.timeValueMillis(0), true, true);
110112
}
111113

112114
/** Returns a method with the given name, or throws an exception if multiple are found. */

‎server/src/main/java/org/elasticsearch/script/ScriptService.java‎

Lines changed: 9 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -231,7 +231,7 @@ void registerClusterSettingsListeners(ClusterSettings clusterSettings) {
231231

232232
// Handle all settings for context and general caches, this flips between general and context caches.
233233
clusterSettings.addSettingsUpdateConsumer(
234-
(settings) -> setCacheHolder(settings),
234+
this::setCacheHolder,
235235
Arrays.asList(SCRIPT_GENERAL_MAX_COMPILATIONS_RATE_SETTING,
236236
SCRIPT_GENERAL_CACHE_EXPIRE_SETTING,
237237
SCRIPT_GENERAL_CACHE_SIZE_SETTING,
@@ -565,8 +565,9 @@ void setCacheHolder(Settings settings) {
565565
} else if (current.general == null) {
566566
// Flipping to general
567567
cacheHolder.set(generalCacheHolder(settings));
568-
} else if (current.general.rate.equals(SCRIPT_GENERAL_MAX_COMPILATIONS_RATE_SETTING.get(settings)) == false) {
569-
// General compilation rate changed, that setting is the only dynamically updated general setting
568+
} else if (current.general.rate.equals(SCRIPT_GENERAL_MAX_COMPILATIONS_RATE_SETTING.get(settings)) == false ||
569+
current.general.cacheExpire.equals(SCRIPT_GENERAL_CACHE_EXPIRE_SETTING.get(settings)) == false ||
570+
current.general.cacheSize != SCRIPT_GENERAL_CACHE_SIZE_SETTING.get(settings)) {
570571
cacheHolder.set(generalCacheHolder(settings));
571572
}
572573
}
@@ -592,13 +593,15 @@ ScriptCache contextCache(Settings settings, ScriptContext<?> context) {
592593

593594
Setting<ScriptCache.CompilationRate> rateSetting =
594595
SCRIPT_MAX_COMPILATIONS_RATE_SETTING.getConcreteSettingForNamespace(context.name);
595-
ScriptCache.CompilationRate rate = null;
596-
if (SCRIPT_DISABLE_MAX_COMPILATIONS_RATE_SETTING.get(settings) || compilationLimitsEnabled() == false) {
596+
ScriptCache.CompilationRate rate;
597+
if (SCRIPT_DISABLE_MAX_COMPILATIONS_RATE_SETTING.get(settings)
598+
|| compilationLimitsEnabled() == false
599+
|| context.compilationRateLimited == false) {
597600
rate = SCRIPT_COMPILATION_RATE_ZERO;
598601
} else if (rateSetting.existsOrFallbackExists(settings)) {
599602
rate = rateSetting.get(settings);
600603
} else {
601-
rate = new ScriptCache.CompilationRate(context.maxCompilationRateDefault);
604+
rate = new ScriptCache.CompilationRate(ScriptContext.DEFAULT_COMPILATION_RATE_LIMIT);
602605
}
603606

604607
return new ScriptCache(cacheSize, cacheExpire, rate, rateSetting.getKey());

‎server/src/main/java/org/elasticsearch/script/TemplateScript.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -42,5 +42,5 @@ public interface Factory {
4242
// rate limiting. MustacheScriptEngine explicitly checks for TemplateScript. Rather than complicating the implementation there by
4343
// creating a new Script class (as would be customary), this context is used to avoid the default rate limit.
4444
public static final ScriptContext<Factory> INGEST_CONTEXT = new ScriptContext<>("ingest_template", Factory.class,
45-
200, TimeValue.timeValueMillis(0), ScriptCache.UNLIMITED_COMPILATION_RATE.asTuple(), true);
45+
200, TimeValue.timeValueMillis(0), false, true);
4646
}

‎server/src/test/java/org/elasticsearch/script/ScriptCacheTests.java‎

Lines changed: 60 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -8,14 +8,54 @@
88
package org.elasticsearch.script;
99

1010
import org.elasticsearch.common.breaker.CircuitBreakingException;
11+
import org.elasticsearch.common.settings.Setting;
1112
import org.elasticsearch.common.settings.Settings;
1213
import org.elasticsearch.core.TimeValue;
1314
import org.elasticsearch.test.ESTestCase;
1415

16+
import java.util.stream.Collectors;
17+
1518
public class ScriptCacheTests extends ESTestCase {
1619
// even though circuit breaking is allowed to be configured per minute, we actually weigh this over five minutes
1720
// simply by multiplying by five, so even setting it to one, requires five compilations to break
1821
public void testCompilationCircuitBreaking() throws Exception {
22+
String context = randomFrom(
23+
ScriptModule.CORE_CONTEXTS.values().stream().filter(
24+
c -> c.compilationRateLimited
25+
).collect(Collectors.toList())
26+
).name;
27+
final TimeValue expire = ScriptService.SCRIPT_CACHE_EXPIRE_SETTING.getConcreteSettingForNamespace(context).get(Settings.EMPTY);
28+
final Integer size = ScriptService.SCRIPT_CACHE_SIZE_SETTING.getConcreteSettingForNamespace(context).get(Settings.EMPTY);
29+
Setting<ScriptCache.CompilationRate> rateSetting =
30+
ScriptService.SCRIPT_MAX_COMPILATIONS_RATE_SETTING.getConcreteSettingForNamespace(context);
31+
ScriptCache.CompilationRate rate =
32+
ScriptService.SCRIPT_MAX_COMPILATIONS_RATE_SETTING.getConcreteSettingForNamespace(context).get(Settings.EMPTY);
33+
String rateSettingName = rateSetting.getKey();
34+
ScriptCache cache = new ScriptCache(size, expire,
35+
new ScriptCache.CompilationRate(1, TimeValue.timeValueMinutes(1)), rateSettingName);
36+
cache.checkCompilationLimit(); // should pass
37+
expectThrows(CircuitBreakingException.class, cache::checkCompilationLimit);
38+
cache = new ScriptCache(size, expire, new ScriptCache.CompilationRate(2, TimeValue.timeValueMinutes(1)), rateSettingName);
39+
cache.checkCompilationLimit(); // should pass
40+
cache.checkCompilationLimit(); // should pass
41+
expectThrows(CircuitBreakingException.class, cache::checkCompilationLimit);
42+
int count = randomIntBetween(5, 50);
43+
cache = new ScriptCache(size, expire, new ScriptCache.CompilationRate(count, TimeValue.timeValueMinutes(1)), rateSettingName);
44+
for (int i = 0; i < count; i++) {
45+
cache.checkCompilationLimit(); // should pass
46+
}
47+
expectThrows(CircuitBreakingException.class, cache::checkCompilationLimit);
48+
cache = new ScriptCache(size, expire, new ScriptCache.CompilationRate(0, TimeValue.timeValueMinutes(1)), rateSettingName);
49+
expectThrows(CircuitBreakingException.class, cache::checkCompilationLimit);
50+
cache = new ScriptCache(size, expire,
51+
new ScriptCache.CompilationRate(Integer.MAX_VALUE, TimeValue.timeValueMinutes(1)), rateSettingName);
52+
int largeLimit = randomIntBetween(1000, 10000);
53+
for (int i = 0; i < largeLimit; i++) {
54+
cache.checkCompilationLimit();
55+
}
56+
}
57+
58+
public void testGeneralCompilationCircuitBreaking() throws Exception {
1959
final TimeValue expire = ScriptService.SCRIPT_GENERAL_CACHE_EXPIRE_SETTING.get(Settings.EMPTY);
2060
final Integer size = ScriptService.SCRIPT_GENERAL_CACHE_SIZE_SETTING.get(Settings.EMPTY);
2161
String settingName = ScriptService.SCRIPT_GENERAL_MAX_COMPILATIONS_RATE_SETTING.getKey();
@@ -35,14 +75,33 @@ public void testCompilationCircuitBreaking() throws Exception {
3575
cache = new ScriptCache(size, expire, new ScriptCache.CompilationRate(0, TimeValue.timeValueMinutes(1)), settingName);
3676
expectThrows(CircuitBreakingException.class, cache::checkCompilationLimit);
3777
cache = new ScriptCache(size, expire,
38-
new ScriptCache.CompilationRate(Integer.MAX_VALUE, TimeValue.timeValueMinutes(1)), settingName);
78+
new ScriptCache.CompilationRate(Integer.MAX_VALUE, TimeValue.timeValueMinutes(1)), settingName);
3979
int largeLimit = randomIntBetween(1000, 10000);
4080
for (int i = 0; i < largeLimit; i++) {
4181
cache.checkCompilationLimit();
4282
}
4383
}
4484

4585
public void testUnlimitedCompilationRate() {
86+
String context = randomFrom(
87+
ScriptModule.CORE_CONTEXTS.values().stream().filter(
88+
c -> c.compilationRateLimited
89+
).collect(Collectors.toList())
90+
).name;
91+
final Integer size = ScriptService.SCRIPT_CACHE_SIZE_SETTING.getConcreteSettingForNamespace(context).get(Settings.EMPTY);
92+
final TimeValue expire = ScriptService.SCRIPT_CACHE_EXPIRE_SETTING.getConcreteSettingForNamespace(context).get(Settings.EMPTY);
93+
String settingName = ScriptService.SCRIPT_MAX_COMPILATIONS_RATE_SETTING.getConcreteSettingForNamespace(context).getKey();
94+
ScriptCache cache = new ScriptCache(size, expire, ScriptCache.UNLIMITED_COMPILATION_RATE, settingName);
95+
ScriptCache.TokenBucketState initialState = cache.tokenBucketState.get();
96+
for(int i=0; i < 3000; i++) {
97+
cache.checkCompilationLimit();
98+
ScriptCache.TokenBucketState currentState = cache.tokenBucketState.get();
99+
assertEquals(initialState.lastInlineCompileTime, currentState.lastInlineCompileTime);
100+
assertEquals(initialState.availableTokens, currentState.availableTokens, 0.0); // delta of 0.0 because it should never change
101+
}
102+
}
103+
104+
public void testGeneralUnlimitedCompilationRate() {
46105
final Integer size = ScriptService.SCRIPT_GENERAL_CACHE_SIZE_SETTING.get(Settings.EMPTY);
47106
final TimeValue expire = ScriptService.SCRIPT_GENERAL_CACHE_EXPIRE_SETTING.get(Settings.EMPTY);
48107
String settingName = ScriptService.SCRIPT_GENERAL_MAX_COMPILATIONS_RATE_SETTING.getKey();

0 commit comments

Comments
 (0)