Make ingest pipeline resolution logic unit testable #47026
Conversation
Extracted ingest pipeline resolution logic into a static method and added unit tests for pipeline resolution logic. Followup from elastic#46847
martijnvg
added
>non-issue
:Data Management/Ingest Node
|
Pinging @elastic/es-core-features |
…eMatch test expected that MetaData#indices() is invoked instead of MetaData#getIndices() method. So changed the resolveRequiredOrDefaultPipeline(...) method to do this.
jasontedor
left a comment
jasontedor
left a comment
There was a problem hiding this comment.
The tests look great. I left one suggestion. Thanks for picking this up.
| } else if (IngestService.NOOP_PIPELINE_NAME.equals(indexRequest.getPipeline()) == false) { | ||
| boolean indexRequestHasPipeline = resolveRequiredOrDefaultPipeline(actionRequest, indexRequest, metaData); | ||
| if (indexRequestHasPipeline) { | ||
| hasIndexRequestsWithPipelines = true; |
There was a problem hiding this comment.
Maybe replace all of this with hasIndexRequestWithPipelines |= indexRequestHasPipeline. This will short-circuit if hasIndexRequestWithPipelines is already true and the branch predictor will love it. I would suggest that we leave a comment that resolveRequiredOrDefaultPipeline mutates indexRequest so that the method call can not be folded into the expression (we have to evaluate it).
There was a problem hiding this comment.
I hate that we mutate like this by the way, but it is what it is for now. 🤷♀
There was a problem hiding this comment.
yeah... not very clean 🤷♂
Extracted ingest pipeline resolution logic into a static method and added unit tests for pipeline resolution logic. Followup from #46847
4 participants
Extracted ingest pipeline resolution logic into a static method
and added unit tests for pipeline resolution logic.
Followup from #46847