Skip to content

Fix "size" field in the body of AbstractBulkByScrollRequest #35742

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

Closed
wants to merge 2 commits into from

Conversation

PnPie
Copy link
Contributor

@PnPie PnPie commented Nov 20, 2018

The "size" field in the body of AbstractBulkByScrollRequest(DeleteByQuery/UpdateByQuery) is not taken into consideration anymore during some previous changes, and currently only passing "size" as parameter works for DeleteByQuery or UpdateByQuery. This is to fix it and make it worked again.

Close #35636

The "size" field in the body of AbstractBulkByScrollRequest(DeleteByQuery/UpdateByQuery) is not taken into consideration anymore during some previous changes, this is to fix it.
@cbuescher cbuescher added the :Distributed Indexing/CRUD A catch all label for issues around indexing, updating and getting a doc by id. Not search. label Nov 20, 2018
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-distributed

@ywelsch ywelsch requested a review from nik9000 November 20, 2018 21:56
Copy link
Member

@nik9000 nik9000 left a comment

Choose a reason for hiding this comment

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

I think this is almost right, but it has an issue for transport client users. I think. The trouble is that we didn't have any real testing of the default transport client behavior. But I'm fairly sure we should keep that default size setting. And that means changing Abstract rest controlled slightly. But I think this is on the right track!

@@ -130,7 +130,6 @@ public AbstractBulkByScrollRequest(SearchRequest searchRequest, boolean setDefau
if (setDefaults) {
searchRequest.scroll(DEFAULT_SCROLL_TIMEOUT);
searchRequest.source(new SearchSourceBuilder());
searchRequest.source().size(DEFAULT_SCROLL_SIZE);
Copy link
Member

Choose a reason for hiding this comment

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

I think this should stay as it was or else transport client users will start to get a default scroll size of 10 again which won't perform well..

@PnPie
Copy link
Contributor Author

PnPie commented Dec 8, 2018

Hi @nik9000 I changed search XContent body parsing part and passed a setSize consumer inside to be able to "consume" size field in search request's body.
Otherwise it seems a little bit tricky on Abstract rest level to idenfity whether the size field has been set or not (like this), as by default it's 1000.
What do you think of this ?

@henningandersen
Copy link
Contributor

@PnPie thanks for your efforts on providing this solution. Unfortunately, we have since implemented a different solution in #41894 where we renamed size to max_docs, fixing confusion on the different size parameters in place. I will therefore close this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>bug :Distributed Indexing/CRUD A catch all label for issues around indexing, updating and getting a doc by id. Not search. v7.3.0 v8.0.0-alpha1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

delete_by_query API "size" parameter not supported in the json request in 6.x
8 participants