Skip to content

fix: keep track of indexSlice explicitly in bulk helper response handling loop #1759

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 2 commits into from
Jun 6, 2023
Merged

fix: keep track of indexSlice explicitly in bulk helper response handling loop #1759

merged 2 commits into from
Jun 6, 2023

Conversation

karlriis
Copy link
Contributor

@karlriis karlriis commented Sep 1, 2022

Closes #1751

Fixes the aforementioned issue where a bulk helper call can return wrong information about the erroring operation/document via onDrop(), or cause a deserialization error when the request contains a delete action combined with any other action.

The fix is to keep track of indexSlice separately in the response handling loop of the tryBulk method, instead of deriving it based on the loop counter. The previous approach didn't account for different types of operations which were handled in previous iterations of the loop.

@elasticmachine
Copy link
Collaborator

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

@JoshMock
Copy link
Member

@karlriis Thanks for this fix! can you rebase it against latest main and I'll take a look?

@karlriis
Copy link
Contributor Author

@JoshMock Thanks, rebased against latest main now

@JoshMock
Copy link
Member

jenkins test this

@JoshMock
Copy link
Member

buildkite test this

@abendi
Copy link

abendi commented Jun 1, 2023

@JoshMock any updates on getting this over the line?

Copy link
Member

@JoshMock JoshMock left a comment

Choose a reason for hiding this comment

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

Sorry for the delay @karlriis! This looks right. Merging, and will add a test to verify the change. 😎

@JoshMock
Copy link
Member

JoshMock commented Jun 6, 2023

jenkins test this

@JoshMock
Copy link
Member

JoshMock commented Jun 6, 2023

Tests are failing on Node 14.x for unrelated reasons that I can fix elsewhere. (14.x is also EOL but we'll address that in #1855.)

@JoshMock JoshMock merged commit de17dc0 into elastic:main Jun 6, 2023
JoshMock added a commit that referenced this pull request Jun 7, 2023
JoshMock added a commit that referenced this pull request Jun 7, 2023
JoshMock added a commit that referenced this pull request Jun 7, 2023
JoshMock added a commit that referenced this pull request Jun 8, 2023
@JoshMock JoshMock mentioned this pull request Jun 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Issue with the bulk helper handling errors when a delete action is combined with other actions
4 participants