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.

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

ScriptService: Replace max compilation per minute setting with max compilation rate #26399

Conversation

Projects
None yet
7 participants
@spinscale
Copy link
Member

commented Aug 28, 2017

The current script service has a script compilation limit for a one
minute window. This is set to a small default value of 15. Instead of
increasing that default value, this commit enhances the time window to
five minutes, so that the script service can deal with bursts by being
able to compile the number of scripts allowed to compile in five minutes
also in one.

Also the setting has been renamed.

@spinscale spinscale changed the title ScriptService: Move script compilation to five minute window ScriptService: Replace max compilation per minute setting with max compilation rate Aug 28, 2017

Scripting: Replace script.max_compilations_per_minute with script.max…
…_compilations_rate

The current script service has a script compilation limit for a one
minute window. This is set to a small default value of 15. Instead of
increasing that default value, this commit enhances the time window to
five minutes, so that the script service can deal with bursts by being
able to compile the number of scripts allowed to compile in five minutes
also in one.

@spinscale spinscale force-pushed the spinscale:1708-move-script-service-compilation-limit-to-five-minute-windows branch to 3feb166 Aug 29, 2017

@@ -66,8 +66,8 @@
Setting.positiveTimeSetting("script.cache.expire", TimeValue.timeValueMillis(0), Property.NodeScope);
public static final Setting<Integer> SCRIPT_MAX_SIZE_IN_BYTES =
Setting.intSetting("script.max_size_in_bytes", 65535, Property.NodeScope);
public static final Setting<Integer> SCRIPT_MAX_COMPILATIONS_PER_MINUTE =
Setting.intSetting("script.max_compilations_per_minute", 15, 0, Property.Dynamic, Property.NodeScope);
public static final Setting<Integer> SCRIPT_MAX_COMPILATIONS_RATE =

This comment has been minimized.

Copy link
@rjernst

rjernst Aug 29, 2017

Member

I'm not sure this new name is better. It has no indication what the base of the rate is? One must find docs to know what a value means, while before it was very clear from the name (per minute). Can we have something to indicate this, or have the values not be a plain integer, but instead a string with some kind of unit? I realize this is creeping the scope a bit, but I think this new setting will just be confusing (eg look at the history of size values without units in 1.x days).

This comment has been minimized.

Copy link
@spinscale

spinscale Aug 30, 2017

Author Member

I agree, that it should be obvious by reading the setting how it works.

I changed the setting to be a real rate, which consists of an nonnegative integer and a positive timevalue split by a / defaulting to 75/5m. Is this what you were after?

This comment has been minimized.

Copy link
@rjernst

rjernst Aug 30, 2017

Member

Great, exactly what I was hoping for.

spinscale added some commits Aug 30, 2017

Review comment: Use a real rate as setting
The script max compilations rate parameter now consists
of an integer representing the number of invocations and a
timevalue representing an interval during which these have to happen.
@rjernst
Copy link
Member

left a comment

I left a couple more minor notes.

static final Function<String, Tuple<Integer, TimeValue>> MAX_COMPILATION_RATE_FUNCTION =
(String value) -> {
if (value.contains("/") == false || value.startsWith("/") || value.endsWith("/")) {
throw new IllegalArgumentException("parameter must contain a positive number and a timevalue, i.e. 10/1m");

This comment has been minimized.

Copy link
@rjernst

rjernst Aug 30, 2017

Member

This should contain the value that failed validation?

This comment has been minimized.

Copy link
@spinscale

spinscale Aug 30, 2017

Author Member

indeed. done.

expectThrows(ElasticsearchParseException.class, () -> MAX_COMPILATION_RATE_FUNCTION.apply("6/1.6m"));
expectThrows(NumberFormatException.class, () -> MAX_COMPILATION_RATE_FUNCTION.apply("foo/bar"));
expectThrows(NumberFormatException.class, () -> MAX_COMPILATION_RATE_FUNCTION.apply("6.0/1m"));
expectThrows(IllegalArgumentException.class, () -> MAX_COMPILATION_RATE_FUNCTION.apply("6/-1m"));

This comment has been minimized.

Copy link
@rjernst

rjernst Aug 30, 2017

Member

I think we should be checking all of these expected exceptions. We use IllegalArgumentException all over, so we should be double checking which one. Even within the rate parsing, we throw 3 different IAEs.

This comment has been minimized.

Copy link
@spinscale

spinscale Aug 30, 2017

Author Member

done

@@ -337,7 +337,7 @@ public InternalTestCluster(long clusterSeed, Path baseDir,
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");
// Some tests make use of scripting quite a bit, so increase the limit for integration tests
builder.put(ScriptService.SCRIPT_MAX_COMPILATIONS_PER_MINUTE.getKey(), 1000);
builder.put(ScriptService.SCRIPT_MAX_COMPILATIONS_RATE.getKey(), "1000/1m");

This comment has been minimized.

Copy link
@rjernst

rjernst Aug 30, 2017

Member

As a follow up, I wonder if we really need to support this enormous rate, or if the tweaks to the default might now be sufficient for test clusters?

This comment has been minimized.

Copy link
@spinscale

spinscale Aug 30, 2017

Author Member

yeah, trying to stick with the new default by default might make most sense... I'll give it a go tomorrow along with some test runs.

throw new IllegalArgumentException("rate ["+ rate +"] must be positive");
}
TimeValue timeValue = TimeValue.parseTimeValue(time, "script.max_compilations_rate");
if (timeValue.nanos() <= 0) {

This comment has been minimized.

Copy link
@rjernst

rjernst Aug 30, 2017

Member

I wonder if we should have a minimum time value. For example, if someone does a rate per 1s, this is something we are unlikely able to handle reliably. Perhaps the minimum should be per 1 minute?

This comment has been minimized.

Copy link
@spinscale

spinscale Aug 30, 2017

Author Member

++, set it to 1 minute

spinscale added some commits Aug 30, 2017

Incorporate review comments
- check for exception message content in tests
- add original value when returning throwing exception
- added a minimum time value of one minute as resolution
@rjernst
Copy link
Member

left a comment

LGTM

@spinscale spinscale merged commit 80d0a32 into elastic:master Sep 1, 2017

1 check passed

elasticsearch-ci Build finished.
Details

spinscale added a commit that referenced this pull request Sep 1, 2017

ScriptService: Replace max compilation per minute setting with max co…
…mpilation rate (#26399)

The current script service has a script compilation limit for a one
minute window. This is set to a small default value of 15. Instead of
increasing that default value, this commit introduces a new setting 
that allows to configure a rate per time unit, so that the script service can deal with bursts better.

The new setting is named `script.max_compilations_rate`,
requires a nonnegative number and a positive time value.

The default is `75/5m`, which is equivalent to the existing 15 per minute.

spinscale added a commit that referenced this pull request Sep 1, 2017

ScriptService: Replace max compilation per minute setting with max co…
…mpilation rate (#26399)

The current script service has a script compilation limit for a one
minute window. This is set to a small default value of 15. Instead of
increasing that default value, this commit introduces a new setting
that allows to configure a rate per time unit, so that the script service can deal with bursts better.

The new setting is named `script.max_compilations_rate`,
requires a nonnegative number and a positive time value.

The default is `75/5m`, which is equivalent to the existing 15 per minute.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.