Skip to content

Commit 2f60ff4

Browse files
stu-elasticstu-elastic
authored
Scripting: Remove general cache settings (elastic#59262)
Removed settings: * `script.cache.max_size` * `script.cache.expire` * `script.max_compilations_rate` Refs: elastic#50152
1 parent 2c879eb commit 2f60ff4

File tree

4 files changed

+60
-272
lines changed

4 files changed

+60
-272
lines changed

‎server/src/main/java/org/elasticsearch/common/settings/ClusterSettings.java

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -372,9 +372,6 @@ public void apply(Settings value, Settings current, Settings previous) {
372372
NetworkService.TCP_RECEIVE_BUFFER_SIZE,
373373
IndexSettings.QUERY_STRING_ANALYZE_WILDCARD,
374374
IndexSettings.QUERY_STRING_ALLOW_LEADING_WILDCARD,
375-
ScriptService.SCRIPT_GENERAL_CACHE_SIZE_SETTING,
376-
ScriptService.SCRIPT_GENERAL_CACHE_EXPIRE_SETTING,
377-
ScriptService.SCRIPT_GENERAL_MAX_COMPILATIONS_RATE_SETTING,
378375
ScriptService.SCRIPT_CACHE_SIZE_SETTING,
379376
ScriptService.SCRIPT_CACHE_EXPIRE_SETTING,
380377
ScriptService.SCRIPT_DISABLE_MAX_COMPILATIONS_RATE_SETTING,

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

Lines changed: 3 additions & 90 deletions
Original file line numberDiff line numberDiff line change
@@ -86,7 +86,7 @@ public class ScriptService implements Closeable, ClusterStateApplier {
8686
if (rate < 0) {
8787
throw new IllegalArgumentException("rate [" + rate + "] must be positive");
8888
}
89-
TimeValue timeValue = TimeValue.parseTimeValue(time, "script.max_compilations_rate");
89+
TimeValue timeValue = TimeValue.parseTimeValue(time, "script.context.$CONTEXT.max_compilations_rate");
9090
if (timeValue.nanos() <= 0) {
9191
throw new IllegalArgumentException("time value [" + time + "] must be positive");
9292
}
@@ -101,16 +101,8 @@ public class ScriptService implements Closeable, ClusterStateApplier {
101101
}
102102
};
103103

104-
public static final Setting<Integer> SCRIPT_GENERAL_CACHE_SIZE_SETTING =
105-
Setting.intSetting("script.cache.max_size", 100, 0, Property.NodeScope, Property.Deprecated);
106-
public static final Setting<TimeValue> SCRIPT_GENERAL_CACHE_EXPIRE_SETTING =
107-
Setting.positiveTimeSetting("script.cache.expire", TimeValue.timeValueMillis(0), Property.NodeScope, Property.Deprecated);
108104
public static final Setting<Integer> SCRIPT_MAX_SIZE_IN_BYTES =
109105
Setting.intSetting("script.max_size_in_bytes", 65535, 0, Property.Dynamic, Property.NodeScope);
110-
public static final Setting<Tuple<Integer, TimeValue>> SCRIPT_GENERAL_MAX_COMPILATIONS_RATE_SETTING =
111-
new Setting<>("script.max_compilations_rate", USE_CONTEXT_RATE_KEY,
112-
(String value) -> value.equals(USE_CONTEXT_RATE_KEY) ? USE_CONTEXT_RATE_VALUE: MAX_COMPILATION_RATE_FUNCTION.apply(value),
113-
Property.Dynamic, Property.NodeScope, Property.Deprecated);
114106

115107
// Per-context settings
116108
static final String CONTEXT_PREFIX = "script.context.";
@@ -242,7 +234,7 @@ public ScriptService(Settings settings, Map<String, ScriptEngine> engines, Map<S
242234

243235
// Validation requires knowing which contexts exist.
244236
this.validateCacheSettings(settings);
245-
this.setCacheHolder(settings);
237+
this.cacheHolder.set(contextCacheHolder(settings));
246238
}
247239

248240
/**
@@ -261,33 +253,17 @@ void registerClusterSettingsListeners(ClusterSettings clusterSettings) {
261253
(settings) -> cacheHolder.get().set(context.name, contextCache(settings, context)),
262254
List.of(SCRIPT_CACHE_SIZE_SETTING.getConcreteSettingForNamespace(context.name),
263255
SCRIPT_CACHE_EXPIRE_SETTING.getConcreteSettingForNamespace(context.name),
264-
SCRIPT_MAX_COMPILATIONS_RATE_SETTING.getConcreteSettingForNamespace(context.name),
265-
SCRIPT_GENERAL_CACHE_EXPIRE_SETTING, // general settings used for fallbacks
266-
SCRIPT_GENERAL_CACHE_SIZE_SETTING
256+
SCRIPT_MAX_COMPILATIONS_RATE_SETTING.getConcreteSettingForNamespace(context.name)
267257
)
268258
);
269259
}
270-
271-
// Handle all settings for context and general caches, this flips between general and context caches.
272-
clusterSettings.addSettingsUpdateConsumer(
273-
(settings) -> setCacheHolder(settings),
274-
List.of(SCRIPT_GENERAL_MAX_COMPILATIONS_RATE_SETTING,
275-
SCRIPT_GENERAL_CACHE_EXPIRE_SETTING,
276-
SCRIPT_GENERAL_CACHE_SIZE_SETTING,
277-
SCRIPT_MAX_COMPILATIONS_RATE_SETTING,
278-
SCRIPT_DISABLE_MAX_COMPILATIONS_RATE_SETTING,
279-
SCRIPT_CACHE_EXPIRE_SETTING,
280-
SCRIPT_CACHE_SIZE_SETTING),
281-
this::validateCacheSettings
282-
);
283260
}
284261

285262
/**
286263
* Throw an IllegalArgumentException if any per-context setting does not match a context or if per-context settings are configured
287264
* when using the general cache.
288265
*/
289266
void validateCacheSettings(Settings settings) {
290-
boolean useContext = SCRIPT_GENERAL_MAX_COMPILATIONS_RATE_SETTING.get(settings).equals(USE_CONTEXT_RATE_VALUE);
291267
List<Setting.AffixSetting<?>> affixes = List.of(SCRIPT_MAX_COMPILATIONS_RATE_SETTING, SCRIPT_CACHE_EXPIRE_SETTING,
292268
SCRIPT_CACHE_SIZE_SETTING);
293269
List<String> customRates = new ArrayList<>();
@@ -304,24 +280,13 @@ void validateCacheSettings(Settings settings) {
304280
}
305281
}
306282
}
307-
if (useContext == false && keys.isEmpty() == false) {
308-
keys.sort(Comparator.naturalOrder());
309-
throw new IllegalArgumentException("Context cache settings [" + String.join(", ", keys) + "] requires [" +
310-
SCRIPT_GENERAL_MAX_COMPILATIONS_RATE_SETTING.getKey() + "] to be [" + USE_CONTEXT_RATE_KEY + "]");
311-
}
312283
if (SCRIPT_DISABLE_MAX_COMPILATIONS_RATE_SETTING.get(settings)) {
313284
if (customRates.size() > 0) {
314285
customRates.sort(Comparator.naturalOrder());
315286
throw new IllegalArgumentException("Cannot set custom context compilation rates [" +
316287
String.join(", ", customRates) + "] if compile rates disabled via [" +
317288
SCRIPT_DISABLE_MAX_COMPILATIONS_RATE_SETTING.getKey() + "]");
318289
}
319-
if (useContext == false) {
320-
throw new IllegalArgumentException("Cannot set custom general compilation rates [" +
321-
SCRIPT_GENERAL_MAX_COMPILATIONS_RATE_SETTING.getKey() + "] to [" +
322-
SCRIPT_GENERAL_MAX_COMPILATIONS_RATE_SETTING.get(settings) + "] if compile rates disabled via [" +
323-
SCRIPT_DISABLE_MAX_COMPILATIONS_RATE_SETTING.getKey() + "]");
324-
}
325290
}
326291
}
327292

@@ -585,39 +550,6 @@ public void applyClusterState(ClusterChangedEvent event) {
585550
clusterState = event.state();
586551
}
587552

588-
void setCacheHolder(Settings settings) {
589-
CacheHolder current = cacheHolder.get();
590-
boolean useContext = SCRIPT_GENERAL_MAX_COMPILATIONS_RATE_SETTING.get(settings).equals(USE_CONTEXT_RATE_VALUE);
591-
592-
if (current == null) {
593-
if (useContext) {
594-
cacheHolder.set(contextCacheHolder(settings));
595-
} else {
596-
cacheHolder.set(generalCacheHolder(settings));
597-
}
598-
return;
599-
}
600-
601-
// Update
602-
if (useContext) {
603-
if (current.general != null) {
604-
// Flipping to context specific
605-
cacheHolder.set(contextCacheHolder(settings));
606-
}
607-
} else if (current.general == null) {
608-
// Flipping to general
609-
cacheHolder.set(generalCacheHolder(settings));
610-
} else if (current.general.rate.equals(SCRIPT_GENERAL_MAX_COMPILATIONS_RATE_SETTING.get(settings)) == false) {
611-
// General compilation rate changed, that setting is the only dynamically updated general setting
612-
cacheHolder.set(generalCacheHolder(settings));
613-
}
614-
}
615-
616-
CacheHolder generalCacheHolder(Settings settings) {
617-
return new CacheHolder(SCRIPT_GENERAL_CACHE_SIZE_SETTING.get(settings), SCRIPT_GENERAL_CACHE_EXPIRE_SETTING.get(settings),
618-
SCRIPT_GENERAL_MAX_COMPILATIONS_RATE_SETTING.get(settings), SCRIPT_GENERAL_MAX_COMPILATIONS_RATE_SETTING.getKey());
619-
}
620-
621553
CacheHolder contextCacheHolder(Settings settings) {
622554
Map<String, ScriptCache> contextCache = new HashMap<>(contexts.size());
623555
contexts.forEach((k, v) -> contextCache.put(k, contextCache(settings, v)));
@@ -651,29 +583,19 @@ ScriptCache contextCache(Settings settings, ScriptContext<?> context) {
651583
* 2) context mode, if the context script cache is configured. There is no general cache in this case.
652584
*/
653585
static class CacheHolder {
654-
final ScriptCache general;
655586
final Map<String, AtomicReference<ScriptCache>> contextCache;
656587

657-
CacheHolder(int cacheMaxSize, TimeValue cacheExpire, Tuple<Integer, TimeValue> maxCompilationRate, String contextRateSetting) {
658-
contextCache = null;
659-
general = new ScriptCache(cacheMaxSize, cacheExpire, maxCompilationRate, contextRateSetting);
660-
}
661-
662588
CacheHolder(Map<String, ScriptCache> context) {
663589
Map<String, AtomicReference<ScriptCache>> refs = new HashMap<>(context.size());
664590
context.forEach((k, v) -> refs.put(k, new AtomicReference<>(v)));
665591
contextCache = Collections.unmodifiableMap(refs);
666-
general = null;
667592
}
668593

669594
/**
670595
* get the cache appropriate for the context. If in general mode, return the general cache. Otherwise return the ScriptCache for
671596
* the given context. Returns null in context mode if the requested context does not exist.
672597
*/
673598
ScriptCache get(String context) {
674-
if (general != null) {
675-
return general;
676-
}
677599
AtomicReference<ScriptCache> ref = contextCache.get(context);
678600
if (ref == null) {
679601
return null;
@@ -682,16 +604,10 @@ ScriptCache get(String context) {
682604
}
683605

684606
ScriptStats stats() {
685-
if (general != null) {
686-
return general.stats();
687-
}
688607
return ScriptStats.sum(contextCache.values().stream().map(AtomicReference::get).map(ScriptCache::stats)::iterator);
689608
}
690609

691610
ScriptCacheStats cacheStats() {
692-
if (general != null) {
693-
return new ScriptCacheStats(general.stats());
694-
}
695611
Map<String, ScriptStats> context = new HashMap<>(contextCache.size());
696612
for (String name: contextCache.keySet()) {
697613
context.put(name, contextCache.get(name).get().stats());
@@ -703,9 +619,6 @@ ScriptCacheStats cacheStats() {
703619
* Update a single context cache if we're in the context cache mode otherwise no-op.
704620
*/
705621
void set(String name, ScriptCache cache) {
706-
if (general != null) {
707-
return;
708-
}
709622
AtomicReference<ScriptCache> ref = contextCache.get(name);
710623
assert ref != null : "expected script cache to exist for context [" + name + "]";
711624
ScriptCache oldCache = ref.get();

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

Lines changed: 28 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -20,44 +20,60 @@
2020

2121
import org.elasticsearch.common.breaker.CircuitBreakingException;
2222
import org.elasticsearch.common.collect.Tuple;
23+
import org.elasticsearch.common.settings.Setting;
2324
import org.elasticsearch.common.settings.Settings;
2425
import org.elasticsearch.common.unit.TimeValue;
2526
import org.elasticsearch.test.ESTestCase;
2627

28+
import java.util.stream.Collectors;
29+
2730
public class ScriptCacheTests extends ESTestCase {
2831
// even though circuit breaking is allowed to be configured per minute, we actually weigh this over five minutes
2932
// simply by multiplying by five, so even setting it to one, requires five compilations to break
3033
public void testCompilationCircuitBreaking() throws Exception {
31-
final TimeValue expire = ScriptService.SCRIPT_GENERAL_CACHE_EXPIRE_SETTING.get(Settings.EMPTY);
32-
final Integer size = ScriptService.SCRIPT_GENERAL_CACHE_SIZE_SETTING.get(Settings.EMPTY);
33-
Tuple<Integer, TimeValue> rate = ScriptService.SCRIPT_GENERAL_MAX_COMPILATIONS_RATE_SETTING.get(Settings.EMPTY);
34-
String settingName = ScriptService.SCRIPT_GENERAL_MAX_COMPILATIONS_RATE_SETTING.getKey();
35-
ScriptCache cache = new ScriptCache(size, expire, Tuple.tuple(1, TimeValue.timeValueMinutes(1)), settingName);
34+
String context = randomFrom(
35+
ScriptModule.CORE_CONTEXTS.values().stream().filter(
36+
c -> c.maxCompilationRateDefault.equals(ScriptCache.UNLIMITED_COMPILATION_RATE) == false
37+
).collect(Collectors.toList())
38+
).name;
39+
final TimeValue expire = ScriptService.SCRIPT_CACHE_EXPIRE_SETTING.getConcreteSettingForNamespace(context).get(Settings.EMPTY);
40+
final Integer size = ScriptService.SCRIPT_CACHE_SIZE_SETTING.getConcreteSettingForNamespace(context).get(Settings.EMPTY);
41+
Setting<Tuple<Integer, TimeValue>> rateSetting =
42+
ScriptService.SCRIPT_MAX_COMPILATIONS_RATE_SETTING.getConcreteSettingForNamespace(context);
43+
Tuple<Integer, TimeValue> rate =
44+
ScriptService.SCRIPT_MAX_COMPILATIONS_RATE_SETTING.getConcreteSettingForNamespace(context).get(Settings.EMPTY);
45+
String rateSettingName = rateSetting.getKey();
46+
ScriptCache cache = new ScriptCache(size, expire, Tuple.tuple(1, TimeValue.timeValueMinutes(1)), rateSettingName);
3647
cache.checkCompilationLimit(); // should pass
3748
expectThrows(CircuitBreakingException.class, cache::checkCompilationLimit);
38-
cache = new ScriptCache(size, expire, (Tuple.tuple(2, TimeValue.timeValueMinutes(1))), settingName);
49+
cache = new ScriptCache(size, expire, (Tuple.tuple(2, TimeValue.timeValueMinutes(1))), rateSettingName);
3950
cache.checkCompilationLimit(); // should pass
4051
cache.checkCompilationLimit(); // should pass
4152
expectThrows(CircuitBreakingException.class, cache::checkCompilationLimit);
4253
int count = randomIntBetween(5, 50);
43-
cache = new ScriptCache(size, expire, (Tuple.tuple(count, TimeValue.timeValueMinutes(1))), settingName);
54+
cache = new ScriptCache(size, expire, (Tuple.tuple(count, TimeValue.timeValueMinutes(1))), rateSettingName);
4455
for (int i = 0; i < count; i++) {
4556
cache.checkCompilationLimit(); // should pass
4657
}
4758
expectThrows(CircuitBreakingException.class, cache::checkCompilationLimit);
48-
cache = new ScriptCache(size, expire, (Tuple.tuple(0, TimeValue.timeValueMinutes(1))), settingName);
59+
cache = new ScriptCache(size, expire, (Tuple.tuple(0, TimeValue.timeValueMinutes(1))), rateSettingName);
4960
expectThrows(CircuitBreakingException.class, cache::checkCompilationLimit);
50-
cache = new ScriptCache(size, expire, (Tuple.tuple(Integer.MAX_VALUE, TimeValue.timeValueMinutes(1))), settingName);
61+
cache = new ScriptCache(size, expire, (Tuple.tuple(Integer.MAX_VALUE, TimeValue.timeValueMinutes(1))), rateSettingName);
5162
int largeLimit = randomIntBetween(1000, 10000);
5263
for (int i = 0; i < largeLimit; i++) {
5364
cache.checkCompilationLimit();
5465
}
5566
}
5667

5768
public void testUnlimitedCompilationRate() {
58-
final Integer size = ScriptService.SCRIPT_GENERAL_CACHE_SIZE_SETTING.get(Settings.EMPTY);
59-
final TimeValue expire = ScriptService.SCRIPT_GENERAL_CACHE_EXPIRE_SETTING.get(Settings.EMPTY);
60-
String settingName = ScriptService.SCRIPT_GENERAL_MAX_COMPILATIONS_RATE_SETTING.getKey();
69+
String context = randomFrom(
70+
ScriptModule.CORE_CONTEXTS.values().stream().filter(
71+
c -> c.maxCompilationRateDefault.equals(ScriptCache.UNLIMITED_COMPILATION_RATE) == false
72+
).collect(Collectors.toList())
73+
).name;
74+
final Integer size = ScriptService.SCRIPT_CACHE_SIZE_SETTING.getConcreteSettingForNamespace(context).get(Settings.EMPTY);
75+
final TimeValue expire = ScriptService.SCRIPT_CACHE_EXPIRE_SETTING.getConcreteSettingForNamespace(context).get(Settings.EMPTY);
76+
String settingName = ScriptService.SCRIPT_MAX_COMPILATIONS_RATE_SETTING.getConcreteSettingForNamespace(context).getKey();
6177
ScriptCache cache = new ScriptCache(size, expire, ScriptCache.UNLIMITED_COMPILATION_RATE, settingName);
6278
ScriptCache.TokenBucketState initialState = cache.tokenBucketState.get();
6379
for(int i=0; i < 3000; i++) {

0 commit comments

Comments
 (0)