-
Notifications
You must be signed in to change notification settings - Fork 24.7k
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
Conversation
Pinging @elastic/es-core-features |
@hub-cap Cav you review ? |
There was a problem hiding this 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!
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? |
There was a problem hiding this 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())); |
There was a problem hiding this comment.
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.
fea9e22
to
2bd30e6
Compare
2bd30e6
to
7061e75
Compare
@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 |
@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
7061e75
to
323a3bf
Compare
@elasticmachine test this please |
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
) 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
) 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
) 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
Fix #46446