Skip to content
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

Fix wrong result when executing bulk requests with and without pipeline #60818

Merged
merged 7 commits into from Aug 19, 2020

Conversation

gaobinlong
Copy link
Contributor

@gaobinlong gaobinlong commented Aug 6, 2020

Closes #60437 .

When executing bulk api, if one request has no ingest pipeline and others have ingest pipeline, then the response is not correct, that's because in IngestService#executeBulkRequest() method, the cursor i doesn't move to next if the request has no ingest pipeline.

The main changes are:

  1. fix the bug in IngestService#executeBulkRequest().
  2. add some unit tests.
  3. add a yaml test.

@mayya-sharipova mayya-sharipova added the :Data Management/Ingest Node Execution or management of Ingest Pipelines including GeoIP label Aug 6, 2020
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-features (:Core/Features/Ingest)

@elasticmachine elasticmachine added the Team:Data Management Meta label for data/management team label Aug 6, 2020
@mayya-sharipova mayya-sharipova added >bug and removed Team:Data Management Meta label for data/management team labels Aug 6, 2020
@danhermann danhermann self-assigned this Aug 10, 2020
@danhermann danhermann self-requested a review August 10, 2020 21:33
@danhermann danhermann removed their assignment Aug 10, 2020
Copy link
Contributor

@danhermann danhermann left a comment

Choose a reason for hiding this comment

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

@gaobinlong, thanks for fixing this. I'll get it merged in for the next release.

@danhermann
Copy link
Contributor

@elasticmachine ok to test

@danhermann
Copy link
Contributor

@elasticmachine update branch

@danhermann
Copy link
Contributor

@gaobinlong
Copy link
Contributor Author

@danhermann, maybe because we have not done the backport, so the new yaml test executes failed when there is a mixed cluster. I'm not clear about how to handle this situation, maybe we can remove the new yaml test and only execute the newly added unit test ?

@danhermann
Copy link
Contributor

@gaobinlong, ah, right. I was not paying attention to the fact that it was a backwards compatibility test that failed. If you will add the following lines (including indentation) to the 90_pipeline.yml file right after "One request has pipeline and another not":

  - skip:
      version: " - 7.99.99"
      reason: "change after backporting"

That will skip the mixed cluster tests for now and then after I backport the change for earlier releases, I'll change that so that the mixed cluster tests include that for the appropriate releases.

@gaobinlong
Copy link
Contributor Author

@danhermann, I have done that, and all checks have passed yet.

@danhermann danhermann merged commit 651242b into elastic:master Aug 19, 2020
@danhermann
Copy link
Contributor

@gaobinlong, great, thank you! I'll get this merged and backported.

@tvernum
Copy link
Contributor

tvernum commented Sep 1, 2020

@danhermann I think we can remove backport pending now. Right?

@danhermann
Copy link
Contributor

Finished the last backport to 7.x and removed "backport pending" label.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>bug :Data Management/Ingest Node Execution or management of Ingest Pipelines including GeoIP v7.9.1 v7.10.0 v8.0.0-alpha1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bug in bulk requests with mix of requests with and without pipelines
6 participants