compile ScriptProcessor inline scripts when creating ingest pipelines #21858
Conversation
talevy
added
:Data Management/Ingest Node
| return new ScriptProcessor(processorTag, script, scriptService); | ||
| CompiledScript compiledInlineScript = null; | ||
| if (script.getType() == ScriptType.INLINE) { | ||
| compiledInlineScript = scriptService.compile(script, ScriptContext.Standard.INGEST, script.getOptions()); |
There was a problem hiding this comment.
I don't think the compile script should be stashed here. Instead, just check that the script can be resolved with the script service (don't just check inline; for example, it would then also ensure if they use a file script it exists).
There was a problem hiding this comment.
++ this is why I open WIP PRs! thanks – I'll check out what it means to resolve a script
There was a problem hiding this comment.
oh, getting back to this now... by resolve, you mean just run through a compile, but don't actually store it. That sounds good to me too, guess I was cutting out the middle-man cacher.
I'll update to run compile on file and inline scripts
talevy
force-pushed
the
compiledinline
branch
3 times, most recently
from
1213451 to
89c4252
Compare
|
@rjernst updated, please let me know if this is what you meant. thanks! |
| @@ -121,6 +131,15 @@ public ScriptProcessor create(Map<String, Processor.Factory> registry, String pr | |||
| throw newConfigurationException(TYPE, processorTag, null, "Could not initialize script"); | |||
| } | |||
|
|
|||
| // compile inline and file-backed scripts to ensure they are valid before constructing the processor. | |||
| if (script.getType() == ScriptType.INLINE || script.getType() == ScriptType.FILE) { | |||
There was a problem hiding this comment.
I don't see why we need to restrict types here. This should work fine for stored scripts as well, and they should be validated too.
There was a problem hiding this comment.
I guess my assumption was that stored scripts are compiled when being stored. is this not the case?
There was a problem hiding this comment.
but, yeah, I'm good with it. less branching
There was a problem hiding this comment.
Yes they are, but you still want to ensure the script id exists?
There was a problem hiding this comment.
yes 😞. working on this in-between logstash builds is not helping my thought process. will update. thanks for review
talevy
force-pushed
the
compiledinline
branch
from
89c4252 to
d3109cb
Compare
|
retest this please |
talevy
force-pushed
the
compiledinline
branch
from
d3109cb to
5011ecb
Compare
|
retest this please |
1 similar comment
|
retest this please |
|
@rjernst mind taking another gander when you find the time? thanks |
| import static org.elasticsearch.script.ScriptType.FILE; | ||
| import static org.elasticsearch.script.ScriptType.INLINE; | ||
| import static org.elasticsearch.script.ScriptType.STORED; | ||
|
|
||
| /** |
There was a problem hiding this comment.
Can you keep some basic docs here?
| @Override | ||
| public void execute(IngestDocument document) { | ||
| ExecutableScript executableScript = scriptService.executable(script, ScriptContext.Standard.INGEST); | ||
| ExecutableScript executableScript; | ||
| executableScript = scriptService.executable(script, ScriptContext.Standard.INGEST); |
There was a problem hiding this comment.
revert splitting this over 2 lines?
talevy
force-pushed
the
compiledinline
branch
from
5011ecb to
9f612ec
Compare
Inline scripts defined in Ingest Pipelines are not compiled at creation time and compile-time errors lazily occur upon initial execution of the pipeline.
WIP because it lacks tests.
Fixes #21842.