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

Add document _count API support to Rest High Level Client. #31868

Closed
wants to merge 1 commit into from

Conversation

mrdjen
Copy link
Contributor

@mrdjen mrdjen commented Jul 6, 2018

At the moment there are no CountRequest and CountResponse classes, and RestCountAction will return SearchResponse with XContent generated dynamically (inline).
CountRequest and CountResponse are added to HL Rest Client.
CountRequest aggregates SearchRequest and exposes relevant methods to user of CountRequest, and delegates these to the counterparts in SearchRequest.
CountResponse, on the contrary has no relationship with SearchResponse, instead CountResponse will reuese relevant members (SearchShardFailures).
Relates to #27205
@javanna

At the moment there are no CountRequest and CountResponse classes, and RestCountAction will return SearchResponse with XContent generated dynamically (inline).
CountRequest and CountResponse are added to HL Rest Client.
CountRequest aggregates SearchRequest and exposes relevant methods to user of CountRequest, and delegates these to the counterparts in SearchRequest.
CountResponse, on the contrary has no relationship with SearchResponse, instead CountResponse will reuese relevant members (SearchShardFailures).
@mrdjen mrdjen changed the title Add document _count API support to Rest High Level Client. #27205 Add document _count API support to Rest High Level Client. Jul 6, 2018
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra

@javanna javanna requested a review from hub-cap July 11, 2018 08:41
@hub-cap
Copy link
Contributor

hub-cap commented Jul 23, 2018

Oh my im quite sorry, this slipped thru my email filter. Ill be looking at it tomorrow. Sorry @mrdjen !!!

@hub-cap
Copy link
Contributor

hub-cap commented Jul 25, 2018

@mrdjen This is more of a generalized question, so im not going to leave it on the review. Ive decided to wait for a more thorough review until we've sorted this out.

It looks like the SearchRequest itself can be used in almost all of the places its currently wrapped. Did you consider not creating a CountRequest and just creating and using the CountResponse::fromXContent for the RestHighLevelClient? It would appear to me at first glance that if you had a RequestConverter method that took in the SearchRequest and added _count to the end of that request, that the only additional item to be built would be the CountResponse::fromXContent. What do you think of that, instead?

@mrdjen
Copy link
Contributor Author

mrdjen commented Jul 25, 2018

Hey thanks for response @hub-cap , and no worries about delay, contributors guide says "sit back and wait" :)

Yes its possible to use SearchRequest instead of CountRequest, still, here's my reasoning for adding CountRequest

  • as enduser of API I'd expect to send a CountRequest to _count endpoint (and believe HL client is more or less rather consistent)
  • there are params in SearchRequest which are not used in CountRequest (and believe because of that should not be exposed to the enduser)
  • its easy to hit autocomplete in IDE and set a paramter that is not used
  • if we go with using SearchRequest with _count endpoint, do we have to describe which params of SearchRequest can/can not be used with _count?

So in this context I added CountRequest - let me know what you think - and you might have different/better perspective than I do.

@hub-cap
Copy link
Contributor

hub-cap commented Aug 1, 2018

Ok, I went ahead and had a conversation with some members on the team that have some prior knowledge about the count API. So a while back we decided to remove the count API from being a TransportAction, since it did / does not need to be. Its effectively a search, like you said, but without the ability to set some things.

After looking at the PR, it would make more sense if we created an actual valid count request/response that is not only used by the client, but that is also used by the server. You gave some good reasons to do so above. So, if you would like to take it on, you can separate this into 2 PRs. The first PR should be the count request/response classes, and using them in the RestCountAction instead of the SearchRequest thats currently being used. The second PR, naturally, would be the rest of the work that you have already done here.

I dont think that its a huge undertaking to put the count request/response into server, and get them used in the RestCountAction, and id be happy to help guide you if you have questions on that PR.

What do you think?

Edit: Thanks for your patience.

@mrdjen
Copy link
Contributor Author

mrdjen commented Aug 2, 2018

Thanks for response @hub-cap , yes I can create another PR with count request/response classes and use them in RestCountAction.

Asking from the top of my head - apart from moving these to server (and then using them in RestCountAction and HL client), do you have any other comments on their structure or relationship with SearchReqest and SearchResponse?

@hub-cap
Copy link
Contributor

hub-cap commented Aug 2, 2018

Id say start out leaving them separate and see how different they end up looking. If after an iteration or two it looks like its similar enough, we can talk about just using a SearchRequest as internal state.

<3 for the contribution!

mrdjen added a commit to mrdjen/elasticsearch that referenced this pull request Aug 8, 2018
…RestCountAction (cat and doc).

At the moment there are no CountRequest and CountResponse classes, and RestCountAction will return SearchResponse with XContent generated dynamically (inline), this PR adds CountRequest and CountResponse. During review of elastic#31868 it was decided to separate elastic#31868 into two PRs and this is first part.
@mrdjen
Copy link
Contributor Author

mrdjen commented Aug 8, 2018

Hi @hub-cap see the referenced PR #32711 as part one - moving CountRequest and CountResponse to server and using them in RestCountAction, let me know what you think, Thanks.

@hub-cap
Copy link
Contributor

hub-cap commented Oct 30, 2018

@mrdjen we can close this right?

@mrdjen
Copy link
Contributor Author

mrdjen commented Oct 30, 2018

Right, closing it now @hub-cap
Thanks

@mrdjen mrdjen closed this Oct 30, 2018
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.

4 participants