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

Script: Restore the scripting general cache #79453

Merged
merged 14 commits into from
Oct 21, 2021

Conversation

stu-elastic
Copy link
Contributor

@stu-elastic stu-elastic commented Oct 19, 2021

Restore the scripting general cache in preparation for deprecation of
the context cache, then removal of the context cache in master.

Brings back the general cache settings:

  • script.max_compilations_rate
  • script.cache.expire
  • script.cache.max_size

The context cache is still default, the switch to general cache as default will happen on context cache deprecation.

System script contexts can now opt-out of compilation rate limiting using a flag rather than a sentinel rate limit value. Duplicated defaults for system contexts have been coalesced.

Refs: #62899

Restore the scripting general cache in preparation for deprecation of
the context cache, then removal of the context cache in master.

Brings back the general cache settings:
* `script.max_compilations_rate`
* `script.cache.expire`
* `script.cache.max_size`

`script.cache.max_size` and `script.cache.expire` are now dynamic settings.

The context cache is still default, the switch to general cache as default will happen on context cache deprecation.

System script contexts can now opt-out of compilation rate limiting using a flag rather than a sentinel rate limit value.  Duplicated defaults for system contexts have been coalesced.

Other than that, this code is the same as what was in 7.x to make a `master`-first commit and backport strategy easy.

Refs: elastic#62899
@stu-elastic stu-elastic requested a review from rjernst October 19, 2021 13:01
@stu-elastic stu-elastic added :Core/Infra/Scripting Scripting abstractions, Painless, and Mustache v7.16.0 v7.16.1 labels Oct 19, 2021
@stu-elastic stu-elastic marked this pull request as ready for review October 19, 2021 13:02
@elasticmachine elasticmachine added the Team:Core/Infra Meta label for core/infra team label Oct 19, 2021
@elasticmachine
Copy link
Collaborator

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

@stu-elastic
Copy link
Contributor Author

@elasticmachine update branch

@stu-elastic
Copy link
Contributor Author

This PR does not incorporate the feedback from: #79508 and #79459

Deprecate the script context cache in favor of the general cache.

Users should use the following settings:
`script.max_compilations_rate` to set the max compilation rate
  for user scripts such as filter scripts.  Certain script contexts
  that submit scripts outside of the control of the user are
  exempted from this rate limit.  Examples include runtime fields,
  ingest and watcher.

`script.cache.max_size` to set the max size of the cache.

`script.cache.expire` to set the expiration time for entries in
the cache.

Whats deprecated?
`script.max_compilations_rate: use-context`.  This special
setting value was used to turn on the script context-specific caches.

`script.context.$CONTEXT.cache_max_size`, use `script.cache.max_size`
instead.

`script.context.$CONTEXT.cache_expire`, use `script.cache.expire`
instead.

`script.context.$CONTEXT.max_compilations_rate`, use
`script.max_compilations_rate` instead.

The default cache size was increased from `100` to `3000`, which
was approximately the max cache size when using context-specific caches.

The default compilation rate limit was increased from `75/5m` to
`150/5m` to account for increasing uses of scripts.

System script contexts can now opt-out of compilation rate limiting
using a flag rather than a sentinel rate limit value.

7.16: Script: Deprecate script context cache elastic#79508
Refs: elastic#62899

7.16: Script: Opt-out system contexts from script compilation rate limit elastic#79459
Refs: elastic#62899
@stu-elastic
Copy link
Contributor Author

This is missing the deprecation check tests from #79508 and #79459, everything else is ported.

@stu-elastic
Copy link
Contributor Author

@elasticmachine update branch

@stu-elastic
Copy link
Contributor Author

@elasticmachine update branch

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.

Looks good once it passes CI, a couple minor things

public ScriptStats(StreamInput in) throws IOException {
compilations = in.readVLong();
cacheEvictions = in.readVLong();
compilationLimitTriggered = in.readVLong();
contextStats = in.readList(ScriptContextStats::new);
compilationLimitTriggered = in.getVersion().onOrAfter(Version.V_7_0_0) ? in.readVLong() : 0;
Copy link
Member

Choose a reason for hiding this comment

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

I don't th ink these conditions are needed since this is master, which is only compatible with 7.16+

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

}

@Override
public void writeTo(StreamOutput out) throws IOException {
out.writeVLong(compilations);
out.writeVLong(cacheEvictions);
out.writeVLong(compilationLimitTriggered);
out.writeList(contextStats);
if (out.getVersion().onOrAfter(Version.V_7_0_0)) {
Copy link
Member

Choose a reason for hiding this comment

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

Same here, this will always be true

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@Override
public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException {
builder.startObject(Fields.SCRIPT_STATS);
builder.field(Fields.COMPILATIONS, compilations);
builder.field(Fields.CACHE_EVICTIONS, cacheEvictions);
builder.field(Fields.COMPILATION_LIMIT_TRIGGERED, compilationLimitTriggered);
/* TODO(stu): master only
Copy link
Member

Choose a reason for hiding this comment

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

Uncomment?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@stu-elastic
Copy link
Contributor Author

@elasticmachine update branch

@stu-elastic stu-elastic merged commit 808b70d into elastic:master Oct 21, 2021
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 Team:Core/Infra Meta label for core/infra team v8.0.0-beta1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants