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

terminateAfter added to the RequestConverter #46474

Merged
merged 1 commit into from
Sep 18, 2019

Conversation

turackangal
Copy link
Contributor

Fix #46446

@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-features

@turackangal
Copy link
Contributor Author

@hub-cap Cav you review ?

Copy link
Member

@martijnvg martijnvg left a comment

Choose a reason for hiding this comment

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

Thanks @turackangal for working on this. Can also you modify the RequestConvertersTests#setRandomCountParams() method to randomly set terminate after?

Something like:

if (randomBoolean()) {
    countRequest.terminateAfter(randomIntBetween(0, Integer.MAX_VALUE));
    expectedParams.put("terminate_after", String.valueOf(countRequest.terminateAfter()));
}

This is used by the testCount() unit test.

Otherwise this PR looks good!

@elasticcla
Copy link

Hi @turackangal, we have found your signature in our records, but it seems like you have signed with a different e-mail than the one used in your Git commit. Can you please add both of these e-mails into your Github profile (they can be hidden), so we can match your e-mails to your Github profile?

Copy link
Member

@martijnvg martijnvg left a comment

Choose a reason for hiding this comment

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

I left one other comment. Also can you remove the commits that have a different email than you signed the CLA with? I think it is best to do an interactive rebase and then force push to your remote branch.

if (randomBoolean()){
countRequest.terminateAfter(randomIntBetween(0, Integer.MAX_VALUE));
}
expectedParams.put("terminateAfter", String.valueOf(countRequest.terminateAfter()));
Copy link
Member

Choose a reason for hiding this comment

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

We should move this statement inside the body of the if statement and then make sure that terminate after is only serialized if it is not equal to 0 in RequestConverters class line 501. This is an optional parameter and otherwise the HLRC would always send it.

@martijnvg
Copy link
Member

@turackangal Thanks for updating the PR. I think that something went wrong when updating your branch, because the PR included over a hundred other commits. I cleaned it up and made a small change (the actual parameter is terminate_after instead of terminateAfter). I will run the build now and if everything is okay then merge the pr.

@martijnvg
Copy link
Member

@elasticmachine test this please

Prior to this commit terminate_after was sent as request body parameter
(via SearchSourceBuilder), which is not possible in the count api.

Closes elastic#46446
@martijnvg
Copy link
Member

@elasticmachine test this please

@martijnvg martijnvg merged commit 084d8d8 into elastic:master Sep 18, 2019
martijnvg pushed a commit that referenced this pull request Sep 18, 2019
#46474)

Prior to this commit terminate_after was sent as request body parameter
(via SearchSourceBuilder), which is not possible in the count api.

Closes #46446
martijnvg pushed a commit that referenced this pull request Sep 18, 2019
#46474)

Prior to this commit terminate_after was sent as request body parameter
(via SearchSourceBuilder), which is not possible in the count api.

Closes #46446
martijnvg added a commit to martijnvg/elasticsearch that referenced this pull request Sep 18, 2019
Prior to this commit min_score was sent as request body parameter
(via SearchSourceBuilder), which is not possible in the count api.

Similar to elastic#46474
martijnvg added a commit that referenced this pull request Sep 20, 2019
)

Prior to this commit min_score was sent as request body parameter
(via SearchSourceBuilder), which is not possible in the count api.

Similar to #46474
martijnvg added a commit that referenced this pull request Sep 20, 2019
)

Prior to this commit min_score was sent as request body parameter
(via SearchSourceBuilder), which is not possible in the count api.

Similar to #46474
martijnvg added a commit that referenced this pull request Sep 20, 2019
)

Prior to this commit min_score was sent as request body parameter
(via SearchSourceBuilder), which is not possible in the count api.

Similar to #46474
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.

high level rest client count request fails with 400 Bad Request
6 participants