Skip to content

Commit

Permalink
Scripting: Remove general cache settings
Browse files Browse the repository at this point in the history 
Removed settings:
 * `script.cache.max_size`
 * `script.cache.expire`
 * `script.max_compilations_rate`

Refs: elastic#50152
  • Loading branch information
@stu-elastic
stu-elastic committed Jul 8, 2020
1 parent 09867ec commit ee58a4c
Show file tree
Hide file tree
Showing 4 changed files with 60 additions and 272 deletions.
3 changes: 0 additions & 3 deletions server/src/main/java/org/elasticsearch/common/settings/ClusterSettings.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -372,9 +372,6 @@ public void apply(Settings value, Settings current, Settings previous) {
NetworkService.TCP_RECEIVE_BUFFER_SIZE,
IndexSettings.QUERY_STRING_ANALYZE_WILDCARD,
IndexSettings.QUERY_STRING_ALLOW_LEADING_WILDCARD,
ScriptService.SCRIPT_GENERAL_CACHE_SIZE_SETTING,
ScriptService.SCRIPT_GENERAL_CACHE_EXPIRE_SETTING,
ScriptService.SCRIPT_GENERAL_MAX_COMPILATIONS_RATE_SETTING,
ScriptService.SCRIPT_CACHE_SIZE_SETTING,
ScriptService.SCRIPT_CACHE_EXPIRE_SETTING,
ScriptService.SCRIPT_DISABLE_MAX_COMPILATIONS_RATE_SETTING,
Expand Down
93 changes: 3 additions & 90 deletions server/src/main/java/org/elasticsearch/script/ScriptService.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,7 @@ public class ScriptService implements Closeable, ClusterStateApplier {
if (rate < 0) {
throw new IllegalArgumentException("rate [" + rate + "] must be positive");
}
TimeValue timeValue = TimeValue.parseTimeValue(time, "script.max_compilations_rate");
TimeValue timeValue = TimeValue.parseTimeValue(time, "script.context.$CONTEXT.max_compilations_rate");
if (timeValue.nanos() <= 0) {
throw new IllegalArgumentException("time value [" + time + "] must be positive");
}
Expand All @@ -101,16 +101,8 @@ public class ScriptService implements Closeable, ClusterStateApplier {
}
};

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

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

// Validation requires knowing which contexts exist.
this.validateCacheSettings(settings);
this.setCacheHolder(settings);
this.cacheHolder.set(contextCacheHolder(settings));
}

/**
Expand All @@ -261,33 +253,17 @@ void registerClusterSettingsListeners(ClusterSettings clusterSettings) {
(settings) -> cacheHolder.get().set(context.name, contextCache(settings, context)),
List.of(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_MAX_COMPILATIONS_RATE_SETTING.getConcreteSettingForNamespace(context.name)
)
);
}

// Handle all settings for context and general caches, this flips between general and context caches.
clusterSettings.addSettingsUpdateConsumer(
(settings) -> setCacheHolder(settings),
List.of(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
);
}

/**
* Throw an IllegalArgumentException if any per-context setting does not match a context or if per-context settings are configured
* when using the general cache.
*/
void validateCacheSettings(Settings settings) {
boolean useContext = SCRIPT_GENERAL_MAX_COMPILATIONS_RATE_SETTING.get(settings).equals(USE_CONTEXT_RATE_VALUE);
List<Setting.AffixSetting<?>> affixes = List.of(SCRIPT_MAX_COMPILATIONS_RATE_SETTING, SCRIPT_CACHE_EXPIRE_SETTING,
SCRIPT_CACHE_SIZE_SETTING);
List<String> customRates = new ArrayList<>();
Expand All @@ -304,24 +280,13 @@ void validateCacheSettings(Settings settings) {
}
}
}
if (useContext == false && keys.isEmpty() == false) {
keys.sort(Comparator.naturalOrder());
throw new IllegalArgumentException("Context cache settings [" + String.join(", ", keys) + "] requires [" +
SCRIPT_GENERAL_MAX_COMPILATIONS_RATE_SETTING.getKey() + "] to be [" + USE_CONTEXT_RATE_KEY + "]");
}
if (SCRIPT_DISABLE_MAX_COMPILATIONS_RATE_SETTING.get(settings)) {
if (customRates.size() > 0) {
customRates.sort(Comparator.naturalOrder());
throw new IllegalArgumentException("Cannot set custom context compilation rates [" +
String.join(", ", customRates) + "] if compile rates disabled via [" +
SCRIPT_DISABLE_MAX_COMPILATIONS_RATE_SETTING.getKey() + "]");
}
if (useContext == false) {
throw new IllegalArgumentException("Cannot set custom general compilation rates [" +
SCRIPT_GENERAL_MAX_COMPILATIONS_RATE_SETTING.getKey() + "] to [" +
SCRIPT_GENERAL_MAX_COMPILATIONS_RATE_SETTING.get(settings) + "] if compile rates disabled via [" +
SCRIPT_DISABLE_MAX_COMPILATIONS_RATE_SETTING.getKey() + "]");
}
}
}

Expand Down Expand Up @@ -585,39 +550,6 @@ public void applyClusterState(ClusterChangedEvent event) {
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<String, ScriptCache> contextCache = new HashMap<>(contexts.size());
contexts.forEach((k, v) -> contextCache.put(k, contextCache(settings, v)));
Expand Down Expand Up @@ -651,29 +583,19 @@ ScriptCache contextCache(Settings settings, ScriptContext<?> context) {
* 2) context mode, if the context script cache is configured. There is no general cache in this case.
*/
static class CacheHolder {
final ScriptCache general;
final Map<String, AtomicReference<ScriptCache>> contextCache;

CacheHolder(int cacheMaxSize, TimeValue cacheExpire, Tuple<Integer, TimeValue> maxCompilationRate, String contextRateSetting) {
contextCache = null;
general = new ScriptCache(cacheMaxSize, cacheExpire, maxCompilationRate, contextRateSetting);
}

CacheHolder(Map<String, ScriptCache> context) {
Map<String, AtomicReference<ScriptCache>> refs = new HashMap<>(context.size());
context.forEach((k, v) -> refs.put(k, new AtomicReference<>(v)));
contextCache = Collections.unmodifiableMap(refs);
general = null;
}

/**
* get the cache appropriate for the context. If in general mode, return the general cache. Otherwise return the ScriptCache for
* the given context. Returns null in context mode if the requested context does not exist.
*/
ScriptCache get(String context) {
if (general != null) {
return general;
}
AtomicReference<ScriptCache> ref = contextCache.get(context);
if (ref == null) {
return null;
Expand All @@ -682,16 +604,10 @@ ScriptCache get(String context) {
}

ScriptStats stats() {
if (general != null) {
return general.stats();
}
return ScriptStats.sum(contextCache.values().stream().map(AtomicReference::get).map(ScriptCache::stats)::iterator);
}

ScriptCacheStats cacheStats() {
if (general != null) {
return new ScriptCacheStats(general.stats());
}
Map<String, ScriptStats> context = new HashMap<>(contextCache.size());
for (String name: contextCache.keySet()) {
context.put(name, contextCache.get(name).get().stats());
Expand All @@ -703,9 +619,6 @@ ScriptCacheStats cacheStats() {
* Update a single context cache if we're in the context cache mode otherwise no-op.
*/
void set(String name, ScriptCache cache) {
if (general != null) {
return;
}
AtomicReference<ScriptCache> ref = contextCache.get(name);
assert ref != null : "expected script cache to exist for context [" + name + "]";
ScriptCache oldCache = ref.get();
Expand Down
40 changes: 28 additions & 12 deletions server/src/test/java/org/elasticsearch/script/ScriptCacheTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,44 +20,60 @@

import org.elasticsearch.common.breaker.CircuitBreakingException;
import org.elasticsearch.common.collect.Tuple;
import org.elasticsearch.common.settings.Setting;
import org.elasticsearch.common.settings.Settings;
import org.elasticsearch.common.unit.TimeValue;
import org.elasticsearch.test.ESTestCase;

import java.util.stream.Collectors;

public class ScriptCacheTests extends ESTestCase {
// even though circuit breaking is allowed to be configured per minute, we actually weigh this over five minutes
// simply by multiplying by five, so even setting it to one, requires five compilations to break
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<Integer, TimeValue> 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);
String context = randomFrom(
ScriptModule.CORE_CONTEXTS.values().stream().filter(
c -> c.maxCompilationRateDefault.equals(ScriptCache.UNLIMITED_COMPILATION_RATE) == false
).collect(Collectors.toList())
).name;
final TimeValue expire = ScriptService.SCRIPT_CACHE_EXPIRE_SETTING.getConcreteSettingForNamespace(context).get(Settings.EMPTY);
final Integer size = ScriptService.SCRIPT_CACHE_SIZE_SETTING.getConcreteSettingForNamespace(context).get(Settings.EMPTY);
Setting<Tuple<Integer, TimeValue>> rateSetting =
ScriptService.SCRIPT_MAX_COMPILATIONS_RATE_SETTING.getConcreteSettingForNamespace(context);
Tuple<Integer, TimeValue> rate =
ScriptService.SCRIPT_MAX_COMPILATIONS_RATE_SETTING.getConcreteSettingForNamespace(context).get(Settings.EMPTY);
String rateSettingName = rateSetting.getKey();
ScriptCache cache = new ScriptCache(size, expire, Tuple.tuple(1, TimeValue.timeValueMinutes(1)), rateSettingName);
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, (Tuple.tuple(2, TimeValue.timeValueMinutes(1))), rateSettingName);
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, (Tuple.tuple(count, TimeValue.timeValueMinutes(1))), rateSettingName);
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, (Tuple.tuple(0, TimeValue.timeValueMinutes(1))), rateSettingName);
expectThrows(CircuitBreakingException.class, cache::checkCompilationLimit);
cache = new ScriptCache(size, expire, (Tuple.tuple(Integer.MAX_VALUE, TimeValue.timeValueMinutes(1))), settingName);
cache = new ScriptCache(size, expire, (Tuple.tuple(Integer.MAX_VALUE, TimeValue.timeValueMinutes(1))), rateSettingName);
int largeLimit = randomIntBetween(1000, 10000);
for (int i = 0; i < largeLimit; i++) {
cache.checkCompilationLimit();
}
}

public void testUnlimitedCompilationRate() {
final Integer size = ScriptService.SCRIPT_GENERAL_CACHE_SIZE_SETTING.get(Settings.EMPTY);
final TimeValue expire = ScriptService.SCRIPT_GENERAL_CACHE_EXPIRE_SETTING.get(Settings.EMPTY);
String settingName = ScriptService.SCRIPT_GENERAL_MAX_COMPILATIONS_RATE_SETTING.getKey();
String context = randomFrom(
ScriptModule.CORE_CONTEXTS.values().stream().filter(
c -> c.maxCompilationRateDefault.equals(ScriptCache.UNLIMITED_COMPILATION_RATE) == false
).collect(Collectors.toList())
).name;
final Integer size = ScriptService.SCRIPT_CACHE_SIZE_SETTING.getConcreteSettingForNamespace(context).get(Settings.EMPTY);
final TimeValue expire = ScriptService.SCRIPT_CACHE_EXPIRE_SETTING.getConcreteSettingForNamespace(context).get(Settings.EMPTY);
String settingName = ScriptService.SCRIPT_MAX_COMPILATIONS_RATE_SETTING.getConcreteSettingForNamespace(context).getKey();
ScriptCache cache = new ScriptCache(size, expire, ScriptCache.UNLIMITED_COMPILATION_RATE, settingName);
ScriptCache.TokenBucketState initialState = cache.tokenBucketState.get();
for(int i=0; i < 3000; i++) {
Expand Down
Loading

0 comments on commit ee58a4c

Please sign in to comment.