Skip to content

Pre-compile inline scripts in Ingest Script processors #57960

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

Merged
merged 3 commits into from
Jun 15, 2020

Conversation

talevy
Copy link
Contributor

@talevy talevy commented Jun 10, 2020

This commit introduces an optimization for inline scripts.
It keeps the compiled ingest script that the ScriptProcessor.Factory
has been creating for validation purposes. Previously, the Script Service's
cache was leveraged because it was the best way to handle caching of both
stored and inline scripts. Since inline scripts are so widely used in
Ingest Node, it is probably best to ensure we are using the pre-compiled version
from the beginning.

This commit introduces an optimization for inline scripts.
It keeps the compiled ingest script that the ScriptProcessor.Factory
has been creating for validation purposes. Previously, the Script Service's
cache was leveraged because it was the best way to handle caching of both
stored and inline scripts. Since inline scripts are so widely used in
Ingest Node, it is probably best to ensure we are using the pre-compiled version
from the beginning.
@talevy talevy added >enhancement >non-issue :Core/Infra/Scripting Scripting abstractions, Painless, and Mustache :Data Management/Ingest Node Execution or management of Ingest Pipelines including GeoIP labels Jun 10, 2020
@talevy talevy requested review from jdconrad and stu-elastic June 10, 2020 23:02
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-features (:Core/Features/Ingest)

@elasticmachine elasticmachine added the Team:Data Management Meta label for data/management team label Jun 10, 2020
@elasticmachine
Copy link
Collaborator

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

@elasticmachine elasticmachine added the Team:Core/Infra Meta label for core/infra team label Jun 10, 2020
@talevy
Copy link
Contributor Author

talevy commented Jun 10, 2020

Hey @stu-elastic and @jdconrad 👋! I was speaking to Ryan about this improvement for inline scripts in Ingest, and we thought it would be a welcomed change. I've been away from ingest and painless land for a while and felt like pushing the change through anyways. Mind reviewing it and letting me know if I'm misusing or not using any of the scripting tools right? thanks!

Copy link
Contributor

@stu-elastic stu-elastic left a comment

Choose a reason for hiding this comment

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

Script changes look good. Tests look good. Nice change.

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.

LGTM. I wonder if with this change we still need a (large?) cache for the ingest processor scripts since it's now being cached locally.

@talevy
Copy link
Contributor Author

talevy commented Jun 11, 2020

thanks for taking a look team!

I wonder if with this change we still need a (large?) cache for the ingest processor scripts since it's now being cached locally.

that is up to you! the only thing I'll add is that caching is still being used for stored scripts. I do not know how much stored scripts are used in Ingest in practice. I suspect not a lot. It may be justified to remove support for stored scripts here entirely, but that will be breaking. Do you have any thoughts on that?

@jdconrad
Copy link
Contributor

@talevy I would like to removed stored scripts altogether, but I'm not sure that's a great idea, yet, especially considering some scripts are getting quite large.

@talevy talevy merged commit 6416d74 into elastic:master Jun 15, 2020
@talevy talevy deleted the precompileinline branch June 15, 2020 18:15
talevy added a commit to talevy/elasticsearch that referenced this pull request Jun 15, 2020
This commit introduces an optimization for inline scripts.
It keeps the compiled ingest script that the ScriptProcessor.Factory
has been creating for validation purposes. Previously, the Script Service's
cache was leveraged because it was the best way to handle caching of both
stored and inline scripts. Since inline scripts are so widely used in
Ingest Node, it is probably best to ensure we are using the pre-compiled version
from the beginning.
talevy added a commit that referenced this pull request Jun 15, 2020
This commit introduces an optimization for inline scripts.
It keeps the compiled ingest script that the ScriptProcessor.Factory
has been creating for validation purposes. Previously, the Script Service's
cache was leveraged because it was the best way to handle caching of both
stored and inline scripts. Since inline scripts are so widely used in
Ingest Node, it is probably best to ensure we are using the pre-compiled version
from the beginning.
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 :Data Management/Ingest Node Execution or management of Ingest Pipelines including GeoIP >enhancement >non-issue Team:Core/Infra Meta label for core/infra team Team:Data Management Meta label for data/management team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants