Scripting: Per-context script cache, default off #52855

stu-elastic · 2020-02-26T23:12:56Z

Adds per context settings:
script.context.${CONTEXT}.cache_max_size ~
script.cache.max_size

script.context.${CONTEXT}.cache_expire ~
script.cache.expire

script.context.${CONTEXT}.max_compilations_rate ~
script.max_compilations_rate
Context cache is used if:
script.max_compilations_rate=use-context. This
value is dynamically updatable, so users can
switch back to the general cache if desired.
Settings for context caches take the first value
that applies:
1. Context specific settings if set, eg
  script.context.ingest.cache_max_size
2. Correlated general setting is set to the non-default
  value, eg script.cache.max_size
3. Context default

The reason for 2's inclusion is to allow an easy
transition for users who've customized their general
cache settings.

Using the general cache settings for the context caches
results in higher effective settings, since they are
multiplied across the number of contexts. So a general
cache max size of 200 will become 200 * # of contexts.
However, this behavior it will avoid users snapping to a
value that is too low for them.

Refs: #50152

* Adds per context settings: `script.context.${CONTEXT}.cache_max_size` ~ `script.cache.max_size` `script.context.${CONTEXT}.cache_expire` ~ `script.cache.expire` `script.context.${CONTEXT}.max_compilations_rate` ~ `script.max_compilations_rate` * General script cache is used if there are no cache settings or if the general settings are used. * Context script cache is used if there are any context cache settings and no general cache settings. * Mixed context and general cache settings are an error. * Context cache settings are dynamically updatable. If the general cache is used, these updates are rejected. * Existing general cache setting, `script.max_compilations_rate` remains dynamically updatable. Updates are rejected if context caches are in use.

jdconrad

I spent more time thinking about how we use SCRIPT_MAX_COMPILATIONS_RATE_DEPRECATED as a sentinel value for determining whether we have overlapping deprecated settings versus new context settings, and I think we need to come up with something different :/ See my comment at those specific lines.

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

elasticmachine · 2020-02-27T19:38:11Z

Pinging @elastic/es-core-infra (:Core/Infra/Scripting)

rjernst

I have a few comments

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

…TEXT_PREFIX, use `exists` for non-affix settings; Remove general compilation rate exists check in dynamic validator

stu-elastic · 2020-02-27T23:50:27Z

Blocking this change on fixing affix settings in #52933

Allow AffixSetting as validator dependencies. If a validator specifies AffixSettings as a dependency, then `validate(T, Map)` will have the concrete setting in a map. Fixes: elastic#52933

…tings

… process updates

…d general caches

…context_specific

…he checking

…152-painless-limit-per-context__02__context_specific

jdconrad

LGTM. Thanks for all the work here. One very minor comment.

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

…152-painless-limit-per-context__02__context_specific

rjernst

This is looking good. I like the new semantics. However I think we can further simplify.

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

…-> ScriptService, throw to assert

…lambda in SCRIPT_GENERAL_MAX_COMPILATIONS_RATE_SETTING

…eral settings

…152-painless-limit-per-context__02__context_specific

jdconrad

LGTM after all these changes. Thanks for cleaning this up so much.

rjernst

LGTM

* Adds per context settings: `script.context.${CONTEXT}.cache_max_size` ~ `script.cache.max_size` `script.context.${CONTEXT}.cache_expire` ~ `script.cache.expire` `script.context.${CONTEXT}.max_compilations_rate` ~ `script.max_compilations_rate` * Context cache is used if: `script.max_compilations_rate=use-context`. This value is dynamically updatable, so users can switch back to the general cache if desired. * Settings for context caches take the first value that applies: 1) Context specific settings if set, eg `script.context.ingest.cache_max_size` 2) Correlated general setting is set to the non-default value, eg `script.cache.max_size` 3) Context default The reason for 2's inclusion is to allow an easy transition for users who've customized their general cache settings. Using the general cache settings for the context caches results in higher effective settings, since they are multiplied across the number of contexts. So a general cache max size of 200 will become 200 * # of contexts. However, this behavior it will avoid users snapping to a value that is too low for them. Refs: elastic#50152

* Adds per context settings: `script.context.${CONTEXT}.cache_max_size` ~ `script.cache.max_size` `script.context.${CONTEXT}.cache_expire` ~ `script.cache.expire` `script.context.${CONTEXT}.max_compilations_rate` ~ `script.max_compilations_rate` * Context cache is used if: `script.max_compilations_rate=use-context`. This value is dynamically updatable, so users can switch back to the general cache if desired. * Settings for context caches take the first value that applies: 1) Context specific settings if set, eg `script.context.ingest.cache_max_size` 2) Correlated general setting is set to the non-default value, eg `script.cache.max_size` 3) Context default The reason for 2's inclusion is to allow an easy transition for users who've customized their general cache settings. Using the general cache settings for the context caches results in higher effective settings, since they are multiplied across the number of contexts. So a general cache max size of 200 will become 200 * # of contexts. However, this behavior it will avoid users snapping to a value that is too low for them. Backport of: #52855 Refs: #50152

stu-elastic · 2020-03-18T20:44:52Z

master: 070ea7e
7.x: cdbee32

* Adds per context settings: `script.context.${CONTEXT}.cache_max_size` ~ `script.cache.max_size` `script.context.${CONTEXT}.cache_expire` ~ `script.cache.expire` `script.context.${CONTEXT}.max_compilations_rate` ~ `script.max_compilations_rate` * Context cache is used if: `script.max_compilations_rate=use-context`. This value is dynamically updatable, so users can switch back to the general cache if desired. * Settings for context caches take the first value that applies: 1) Context specific settings if set, eg `script.context.ingest.cache_max_size` 2) Correlated general setting is set to the non-default value, eg `script.cache.max_size` 3) Context default The reason for 2's inclusion is to allow an easy transition for users who've customized their general cache settings. Using the general cache settings for the context caches results in higher effective settings, since they are multiplied across the number of contexts. So a general cache max size of 200 will become 200 * # of contexts. However, this behavior it will avoid users snapping to a value that is too low for them. Refs: elastic#50152

stu-elastic requested review from rjernst and jdconrad February 26, 2020 23:12

jdconrad requested changes Feb 27, 2020

View reviewed changes

server/src/main/java/org/elasticsearch/script/ScriptService.java Outdated Show resolved Hide resolved

jdconrad added the :Core/Infra/Scripting Scripting abstractions, Painless, and Mustache label Feb 27, 2020

jdconrad added >enhancement v8.0.0 labels Feb 27, 2020

rjernst reviewed Feb 27, 2020

View reviewed changes

Setting SCRIPT_*_DEPRECATED -> SCRIPT_GENERAL_*; contextPrefix -> CON…

cc55177

…TEXT_PREFIX, use `exists` for non-affix settings; Remove general compilation rate exists check in dynamic validator

stu-elastic added 11 commits February 28, 2020 15:08

Settings: AffixSettings as validator dependencies

3e0cbb9

Allow AffixSetting as validator dependencies. If a validator specifies AffixSettings as a dependency, then `validate(T, Map)` will have the concrete setting in a map. Fixes: elastic#52933

Use validators for TimeSettings, use atomic ref to change context set…

c516495

…tings

Get affix match any

5762bd5

WIP flag

ab9ad44

Add script.context.cache_enabled setting, use single cache updater to…

06f0bd8

… process updates

Allow "use-context" setting to flip back and forth between context an…

bfa97f9

…d general caches

Merge branch 'master' into fix/50152-painless-limit-per-context__02__…

c7b6ef8

…context_specific

Remove commented out tests, unnecessary settings updates, simpile cac…

5ce234a

…he checking

Add some cache object tests

9c59b35

Test per context nodes

f8ae8bb

Merge branch 'master' of github.com:elastic/elasticsearch into fix/50…

6310977

…152-painless-limit-per-context__02__context_specific

jdconrad approved these changes Mar 9, 2020

View reviewed changes

server/src/main/java/org/elasticsearch/script/ScriptService.java Outdated Show resolved Hide resolved

stu-elastic added 2 commits March 10, 2020 09:17

Merge branch 'master' of github.com:elastic/elasticsearch into fix/50…

020cc27

…152-painless-limit-per-context__02__context_specific

ZERO -> SCRIPT_COMPILATION_RATE_ZERO

893f187

rjernst requested changes Mar 10, 2020

View reviewed changes

stu-elastic added 4 commits March 11, 2020 11:17

SCRIPT_MAX_COMPILATIONS_RATE += _SETTING, CacheUpdater:contextExists …

b9b626a

…-> ScriptService, throw to assert

MAX_COMPILATION_RATE_FUNCTION check of USE_CONTEXT_RATE_KEY moved to …

6672e64

…lambda in SCRIPT_GENERAL_MAX_COMPILATIONS_RATE_SETTING

flip assert

bfb77ed

script.context.$CONTEXT.{cache_max_size,cache_expire} fallback to gen…

6deb708

…eral settings

stu-elastic added 5 commits March 13, 2020 10:06

use validator in group settings updater, validate all settings together

821cc54

Update dependencies on context cache updaters

76ec2ab

Add conflict, fallback, and validator tests

b588673

CacheHolder settings tests

63ffa65

Merge branch 'master' of github.com:elastic/elasticsearch into fix/50…

557c856

…152-painless-limit-per-context__02__context_specific

jdconrad approved these changes Mar 17, 2020

View reviewed changes

rjernst approved these changes Mar 18, 2020

View reviewed changes

stu-elastic merged commit 070ea7e into elastic:master Mar 18, 2020

stu-elastic mentioned this pull request Mar 18, 2020

Scripting: Per-context script cache, default off (#52855) #53756

Merged

stu-elastic added the v7.7.0 label Mar 18, 2020

codebrain mentioned this pull request Apr 1, 2020

7.7.0 meta ticket (Part 2) elastic/elasticsearch-net#4533

Closed

jakelandis added v8.0.0-alpha1 and removed v8.0.0 labels Jul 26, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Scripting: Per-context script cache, default off #52855

Scripting: Per-context script cache, default off #52855

stu-elastic commented Feb 26, 2020 •

edited

Loading

jdconrad left a comment

elasticmachine commented Feb 27, 2020

rjernst left a comment

stu-elastic commented Feb 27, 2020

jdconrad left a comment

rjernst left a comment

jdconrad left a comment

rjernst left a comment

stu-elastic commented Mar 18, 2020

Scripting: Per-context script cache, default off #52855

Scripting: Per-context script cache, default off #52855

Conversation

stu-elastic commented Feb 26, 2020 • edited Loading

jdconrad left a comment

Choose a reason for hiding this comment

elasticmachine commented Feb 27, 2020

rjernst left a comment

Choose a reason for hiding this comment

stu-elastic commented Feb 27, 2020

jdconrad left a comment

Choose a reason for hiding this comment

rjernst left a comment

Choose a reason for hiding this comment

jdconrad left a comment

Choose a reason for hiding this comment

rjernst left a comment

Choose a reason for hiding this comment

stu-elastic commented Mar 18, 2020

stu-elastic commented Feb 26, 2020 •

edited

Loading