Skip to content

[Ingest] Ingest better bulk failure handling#14888

Merged
martijnvg merged 1 commit intoelastic:feature/ingestfrom
martijnvg:ingest_better_bulk_failure_handeling
Nov 24, 2015
Merged

[Ingest] Ingest better bulk failure handling#14888
martijnvg merged 1 commit intoelastic:feature/ingestfrom
martijnvg:ingest_better_bulk_failure_handeling

Conversation

@martijnvg
Copy link
Copy Markdown
Member

With this change if ingest pipeline fails only the index request in the bulk fails whereas right now the entire bulk request is failed.

If pipeline processing fails then the index request gets reported as a failure in the bulk response in its respective place. The index request will not be executed after pipeline processing has been completed.

If pipeline failures occur a new bulk request will be created based on the current bulk request. Only index requests that have successfully been processed by the pipeline will be included in this new bulk request. Then when sending back the bulk response the new bulk request will map its response items in such a way that the order of all items and errors is the same and matches with the original bulk request that was sent.

@martijnvg martijnvg added review :Distributed/Ingest Node Execution or management of Ingest Pipelines labels Nov 20, 2015
@martijnvg martijnvg force-pushed the ingest_better_bulk_failure_handeling branch from 0d4c46a to 6f40576 Compare November 20, 2015 14:27
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

can we call it markCurrentItemAsFailed ? Also can we not retrieve from the bulkrequest given currentSlot the index request rather than requiring it as an argument?

@javanna
Copy link
Copy Markdown
Contributor

javanna commented Nov 20, 2015

change looks great @mvg thanks a lot. I left some comments around code style and design plus tests, let me know what you think.

@martijnvg
Copy link
Copy Markdown
Member Author

@javanna I'v updated the PR.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I see that we are now doing new Object[]{pipelineId}. I don't get why though, wasn't just pipelineId ok?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

at runtime the wrong method was used... and there for the variable placeholder didn't work and the variable was appended at the end of the log line

By using object[] the right format method gets used.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I'll remove the LoggerMessageFormat usage, lets go back the plain old string concatenation.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I see, ok!

@javanna
Copy link
Copy Markdown
Contributor

javanna commented Nov 23, 2015

I left a few more comments, looks good, just a few small things that we can improve left I think. let me know what you think.

@martijnvg martijnvg force-pushed the ingest_better_bulk_failure_handeling branch from 1c90d20 to 8964ebd Compare November 23, 2015 16:21
@martijnvg
Copy link
Copy Markdown
Member Author

@javanna thanks, I updated the PR.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

with the current randomizations, do we ever test this branch? If not would it be worth to write a specific test for it?

@javanna
Copy link
Copy Markdown
Contributor

javanna commented Nov 23, 2015

much better thanks @martijnvg . I left a question around testing, LGTM otherwise

@martijnvg martijnvg force-pushed the ingest_better_bulk_failure_handeling branch from 8964ebd to 8b1f117 Compare November 24, 2015 11:41
@martijnvg martijnvg merged commit 8b1f117 into elastic:feature/ingest Nov 24, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

:Distributed/Ingest Node Execution or management of Ingest Pipelines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants