Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Scripting: Remove general cache settings #59262

Merged
merged 1 commit into from
Jul 9, 2020
Merged

Scripting: Remove general cache settings #59262

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 0 additions & 3 deletions server/src/main/java/org/elasticsearch/common/settings/ClusterSettings.java
View file
Open in desktop
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
Open in desktop
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
Open in desktop
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