Skip to content

Conversation

@joegallo
Copy link
Contributor

@joegallo joegallo commented Dec 7, 2022

Internal cleanup/refactoring -- each commit is pretty much independent, I'm trying to shed some WIP from a bigger PR that I'm still bringing into existence, and these seemed like the best candidates to be merged sooner.

for consistency with executePipelines
Now the arguments for executeBulkRequest, executePipelines, and
innerExecute are all final.
If somehow zero was passed in as the numberOfActionRequests, then we'd
never call any of the onWhatever handlers, including (crucially) the
onCompletion handler, which means that the whole method would end up
no-oping and we'd never respond to anybody.

If you parse the code TransportBulkAction (the only caller of
executeBulkRequest), everything's already been validated to be >0, but
I don't think it harms anybody to be explicit inside the entry-point
of the IngestService itself.
@joegallo joegallo added >non-issue :Data Management/Ingest Node Execution or management of Ingest Pipelines including GeoIP Team:Data Management Meta label for data/management team v8.7.0 labels Dec 7, 2022
@joegallo joegallo requested a review from masseyke December 7, 2022 14:43
@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-data-management (Team:Data Management)

@masseyke
Copy link
Member

masseyke commented Dec 7, 2022

I don't see any problems with it, but seems like a lot of effort just to move one argument up two spots doesn't it? I assume there's a reason for it?

@joegallo
Copy link
Contributor Author

joegallo commented Dec 7, 2022

"you are in a maze of twisty little passages, all alike" <-- me

For as much time as I've spent in IngestService over the last few weeks, I keep stumbling on the order of the arguments to executeBulkRequest and going "wait, which handler is this 40 lines of code inline lambda again?". Reordering them gets bulkRequestModifier::markItemAsDropped above the fold (right after bulkRequestModifier::markItemAsFailed, rather than sorta dangling at the end there below the fold) and it's clearly the onDropped handler based on its name.

Also it gets order of the handler arguments into the same order between executeBulkRequest and executePipelines -- onDropped and onFailure come first, and let you signal that a document should be dropped or that it failed, and then onCompletion which is the last thing you should call when you've processed all the documents (so there's a "last-i-ness" to it both in terms of order of arguments and actual execution semantics).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

:Data Management/Ingest Node Execution or management of Ingest Pipelines including GeoIP >non-issue Team:Data Management Meta label for data/management team v8.7.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants