Skip to content

Scripting: Deprecate general cache settings#55038

Merged
stu-elastic merged 23 commits intoelastic:masterfrom
stu-elastic:fix/50152-painless-limit-per-context__05__deprecate
Apr 22, 2020
Merged

Scripting: Deprecate general cache settings#55038
stu-elastic merged 23 commits intoelastic:masterfrom
stu-elastic:fix/50152-painless-limit-per-context__05__deprecate

Conversation

@stu-elastic
Copy link
Copy Markdown
Contributor

Deprecate and remove as fallback settings:

  • script.cache.max_size in favor of script.context.$CONTEXT.cache_max_size
  • script.cache.expire in favor of script.context.$CONTEXT.cache_expire
  • script.max_compilations_rate in favor of script.context.$CONTEXT.max_compilations_rate

Default to context cache.

Add script.disable_max_compilations_rate, default false to totally disable compilation rates. This setting is intended for integration tests.

Refs: #50152

@stu-elastic stu-elastic added >enhancement :Core/Infra/Scripting Scripting abstractions, Painless, and Mustache labels Apr 9, 2020
@stu-elastic stu-elastic requested review from jdconrad and rjernst April 9, 2020 23:54
@elasticmachine
Copy link
Copy Markdown
Collaborator

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

* Update a single context cache if we're in the context cache mode otherwise no-op.
*/
void updateContextSettings(Settings settings, ScriptContext<?> context) {
void set(String name, ScriptCache context) {
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rename to cache

Copy link
Copy Markdown
Contributor

@jdconrad jdconrad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the walkthrough. This looks good to me. Please also have @rjernst review as he's more familiar with the testing framework.

static class CacheHolder {
final ScriptCache general;
final Map<String, AtomicReference<ScriptCache>> contextCache;
void setCacheHolder(Settings settings, AtomicReference<CacheHolder> cacheHolder) {
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Make this package protected.

@jrodewig jrodewig self-requested a review April 21, 2020 15:07
Copy link
Copy Markdown
Contributor

@jrodewig jrodewig left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Docs LGTM.

Copy link
Copy Markdown
Member

@rjernst rjernst left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM after #55560 is merged.

Merge branch 'master' of github.com:elastic/elasticsearch into fix/50152-painless-limit-per-context__05__deprecate
@stu-elastic
Copy link
Copy Markdown
Contributor Author

@elasticmachine run elasticsearch-ci/2

@stu-elastic stu-elastic merged commit bd64da0 into elastic:master Apr 22, 2020
stu-elastic added a commit to stu-elastic/elasticsearch that referenced this pull request Apr 23, 2020
* Test: don't modify defaultConfig on upgrade
@stu-elastic
Copy link
Copy Markdown
Contributor Author

Master: ef543b0
7.x (7.9): 88e8b34

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

:Core/Infra/Scripting Scripting abstractions, Painless, and Mustache >enhancement

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants