Skip to content

Scripting: Deprecate general cache settings #55038

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

Conversation

stu-elastic
Copy link
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 rjernst and jdconrad April 9, 2020 23:54
@elasticmachine
Copy link
Collaborator

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

*/
void updateContextSettings(Settings settings, ScriptContext<?> context) {
void set(String name, ScriptCache context) {
Copy link
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
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
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
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
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
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
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