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

Watcher: Reduce script cache churn by checking for mustache tags #33978

Merged

Conversation

spinscale
Copy link
Contributor

@spinscale spinscale commented Sep 24, 2018

Watcher is using a lot of so called TextTemplate fields in a watch
definition, which can use mustache to insert the watch id for example.
For the user it is non-obvious which field is just a string field or
which field is a text template.

This also means, that for every such field, we currently do a script
compilation, even if the field does not contain any mustache syntax.

This will lead to an increased script cache churn, because those
compiled scripts (that only contain a string), will evict other scripts.
On top of that, this also means that an unneeded compilation has
happened, instead of returning that string immediately.

The usages of mustache templating are in all of the actions (most of the time far
more than one compilation) as well as most of the inputs.

Especially when running a lot of watches in parallel, this will reduce
execution times and help reuse of real scripts.

Reviewers note: We already had some discussion about this, for example in #29280, but could not come up with a clean solution to solve this in general, so I'd like to go with this for now within the watcher codebase, as this helps heavy watcher users tremendously.

Watcher is using a lot of so called TextTemplate fields in a watch
definition, which can use mustache to insert the watch id for example.
For the user it is non-obvious which field is just a string field or
which field is a text template.

This also means, that for every such field, we currently do a script
compilation, even if the field does not contain any mustache syntax.

This will lead to an increased script cache churn, because those
compiled scripts (that only contain a string), will evict other scripts.
On top of that, this also means that an unneeded compilation has
happened, instead of returning that string immediately.

The usages of mustache templating are in all of the actions (most of the time far
more than one compilation) as well as most of the inputs.

Especially when running a lot of watches in parallel, this will reduce
execution times and help reuse of real scripts.
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra

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.

I have a couple questions

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

Copy link
Contributor

@hub-cap hub-cap left a comment

Choose a reason for hiding this comment

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

:shipit:

@spinscale spinscale merged commit a15b1b9 into elastic:master Sep 27, 2018
spinscale added a commit that referenced this pull request Sep 27, 2018
)

Watcher is using a lot of so called TextTemplate fields in a watch
definition, which can use mustache to insert the watch id for example.
For the user it is non-obvious which field is just a string field or
which field is a text template.

This also means, that for every such field, we currently do a script
compilation, even if the field does not contain any mustache syntax.

This will lead to an increased script cache churn, because those
compiled scripts (that only contain a string), will evict other scripts.
On top of that, this also means that an unneeded compilation has
happened, instead of returning that string immediately.

The usages of mustache templating are in all of the actions (most of the time far
more than one compilation) as well as most of the inputs.

Especially when running a lot of watches in parallel, this will reduce
execution times and help reuse of real scripts.
jasontedor added a commit to jasontedor/elasticsearch that referenced this pull request Sep 27, 2018
* master: (25 commits)
  [DOCS] Synchronize location of Breaking Changes (elastic#33588)
  [DOCS] Synchronizes captialization in top-level titles (elastic#33605)
  [SQL] Clean up LogicalPlanBuilder#doJoin (elastic#34048)
  Fix remote cluster seeds fallback (elastic#34090)
  [ML][HLRC] Replace REST-based ML test cleanup with the ML client (elastic#34109)
  Handle MatchNoDocsQuery in span query wrappers (elastic#34106)
  Update MovAvgIT AwaitsFix bug url
  Bad regex in CORS settings should throw a nicer error (elastic#34035)
  [HLRC] Support for role mapper expression dsl (elastic#33745)
  Watcher: Reduce script cache churn by checking for mustache tags (elastic#33978)
  Fold EngineSearcher into Engine.Searcher (elastic#34082)
  Mute SpanMultiTermQueryBuilderTests#testToQuery
  TESTS: Enable DEBUG Logging in Flaky Test (elastic#34091)
  TEST: Add engine is closed as expected failure msg
  Adjust bwc version for max_seq_no_of_updates
  Build DocStats from SegmentInfos in ReadOnlyEngine (elastic#34079)
  When creating wildcard queries, use MatchNoDocsQuery when the field type doesn't exist. (elastic#34093)
  [DOCS] Moves graph to docs folder (elastic#33472)
  Mute MovAvgIT#testHoltWintersNotEnoughData
  Security: use default scroll keepalive (elastic#33639)
  ...
kcm pushed a commit that referenced this pull request Oct 30, 2018
)

Watcher is using a lot of so called TextTemplate fields in a watch
definition, which can use mustache to insert the watch id for example.
For the user it is non-obvious which field is just a string field or
which field is a text template.

This also means, that for every such field, we currently do a script
compilation, even if the field does not contain any mustache syntax.

This will lead to an increased script cache churn, because those
compiled scripts (that only contain a string), will evict other scripts.
On top of that, this also means that an unneeded compilation has
happened, instead of returning that string immediately.

The usages of mustache templating are in all of the actions (most of the time far
more than one compilation) as well as most of the inputs.

Especially when running a lot of watches in parallel, this will reduce
execution times and help reuse of real scripts.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants