Add CountRequest/Response classes to server and use them in RestCountAction (cat and doc). #32711

mrdjen · 2018-08-08T12:09:06Z

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, and changes RestCountAction so it will use the newly added classes.

This was initiated during review of #31868, it was decided to separate #31868 into two PRs and this is first part.

relates #31868
@hub-cap

…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.

elasticmachine · 2018-08-09T12:53:05Z

Pinging @elastic/es-core-infra

hub-cap · 2018-08-13T14:47:05Z

Hey @mrdjen ive been on vacation for a week. Im going to look at this in the next day or two, i promise :)

I noticed you wrapped the SearchRequest in the CountRequest and we had previously discussed creating a new one w/o any mention of SearchRequest. Did you attempt this and see that the code was too similar? Im fine with this approach, but I would prefer to have a separate request if possible.

mrdjen · 2018-08-13T18:07:06Z

Hi @hub-cap, good to have you back! Hope you enjoyed your vacation, as I'll have mine soon as well :)

Had a quick look to have separate Count and SearchRequests, here's the list of members that would have to be duplicated in both classes

IndicesOptions indicesOptions
SearchSourceBuilder source
String[] indices
String[] types
String routing
String preference

So CountRequest would have to evolve in tandem with SearchRequest, looks fragile to me (double the work and chance to make an error), so given the above I went for delegation. If you see some issue with this approach let me know and I can work it out.

There's one thing I think needs to change though,
validate() could/should return null, instead of delegating to searchRequest.validate()

Let me know what you think.

hub-cap · 2018-08-13T20:37:57Z

Im only ok with this approach assuming there is no way to get to the search request itself. For instance, you have a getSearchRequest() and you use that in some other spots, and this will give a user the ability to getSearchRequest().doWhateverTheyWantToSaidRequest() and that removes the usefulness of encapsulating the class and only providing access to the bits that the count request uses. If you have to expose the search request, i think it might make more sense to make a new class.

Looking at SearchRequest, of the 13 private variables in master, it looks like only 6 of those are duplicated, and 2 of those 6 are already built out for you (the non primitive types), so you do not need to do anything WRT their transport read/write code, which is a lot of the effort in the actions here. Im still on the fence, but I will definitely look more at this code tomorrow, and probably try to implement it just to see how duplicated it is.

mrdjen · 2018-08-13T22:34:47Z

Right getSearchRequest() is used in AbstractClient.count() and avoiding getSearchRequest().doWhateverTheyWantToSaidRequest() is a good motivation to go direction your suggesting.

Another problem is the need to expose SearchSourceBuilder via source() for the same reason as above (breaking encapsulation).

But maybe I'm missing something, its too late, curious to see how it goes from here :)

hub-cap · 2018-08-27T18:07:10Z

Hi @mrdjen, sorry for being M.I.A. again, as I have been trying to figure out the future of the rest client and its dependencies (or lack thereof!). Ill have more info in a day or two, so hold tight. This will probably not affect you, but Ill definitely be giving you an approval or ask you to fix something in the next day or two.

mrdjen · 2018-08-29T16:38:34Z

Hi @hub-cap, if you have any comments let me know,
Thanks

hub-cap · 2018-08-29T17:37:39Z

Hi, @mrdjen did you ever submit a PR that did not use a search request as a member variable to the count request? Coming back to this, I think I would like to see what it looks like w/o holding the state of the search request. We are moving away from reusing these classes in the rest client, and implementing ones that do not extend ActionRequest/ActionResponse, so it would be good to split them up and not tie them together. Lets go that route and see what it looks like. You can keep this PR open for now, but I think duplicating the code is fine now. Hopefully one day all of the client code will be generated anyway ;) ;)

mrdjen · 2018-08-29T18:08:32Z

Hi @hub-cap no I didn't submit a PR without SearchRequest as a member variable, yet.

If we go this way, not using directly SearchRequest, but using its 6 members (including SearchSourceBuilder), what's your suggestion on having SearchSourceBuilder as member variable of CountRequest, as SearchSourceBuilder exposes significant number of methods (and functionality) used in the Search context but not in Count context?
(and yes I can make a new PR (or work in this one) but lets first agree on SearchSourceBuilder in CountRequest)

hub-cap · 2018-09-07T15:15:42Z

Do you expect that the SearchSourceBuilder will be exposed or encapsulated in the CountRequest? Im ok with it if the SearchSourceBuilder is not easily exposed and "messed around with". :)

…RestCountAction (cat and doc). Changes requested - instead of wrapping SearchRequest, now CountRequest inlines required members from SearchRequest - SearchSourceBuilder as member - validate to return null elastic#32711

mrdjen · 2018-09-17T13:03:11Z

Hi @hub-cap

I've addressed/made following changes

instead of wrapping SearchRequest, now CountRequest inlines required members from SearchRequest
validate returns null
SearchSourceBuilder as member

When it comes to "not messing around with SearchSourceBuilder" - unless I'm wrong - CountRequest supports only QueryBuilder and any other (Aggregation, Highlighter, Slice ..) throws parse exception. For example Aggregations will return "[type=parsing_exception, reason=request does not support [aggregations]", so this part hopefully should be ok.
Are you aware of anything else? Its 1500 lines class packed with functionality and any input from you or other Elastic members is appreciated.

Let me know your comments, hopefully I can resolve whatever is left and if/once this is accepted I'll merge it into original #31868

hub-cap · 2018-09-17T15:20:36Z

Awesome, tyvm! Ill have a look shortly.

hub-cap

I think this is totally the right direction. There are a few minor style nits but this is exactly what we should be doing. Lets wait to merge anything in to your other code until we get this merged in to server. I will also find someone who has more familiarity with search to review it.

hub-cap · 2018-09-17T15:26:50Z

server/src/main/java/org/elasticsearch/action/search/CountResponse.java

+    public CountResponse(SearchResponse searchResponse) {
+        this.count = searchResponse.getHits().totalHits;
+        this.terminatedEarly = searchResponse.isTerminatedEarly();
+        this.shardStats = new ShardStats(searchResponse.getSuccessfulShards(), searchResponse.getTotalShards(),searchResponse


getTotalShards(),searchResponse can you put searchResponse on the next line?

hub-cap · 2018-09-17T15:27:15Z

server/src/main/java/org/elasticsearch/action/search/CountResponse.java

+        }
+    }
+
+


Remove these empty lines pls

hub-cap · 2018-09-17T15:27:51Z

server/src/main/java/org/elasticsearch/action/search/CountResponse.java

+                    parser.skipChildren();
+                }
+            }
+            return new ShardStats(successfulShards,totalShards,skippedShards,failures.toArray(new ShardSearchFailure[failures.size()]));


spaces between commas

hub-cap · 2018-09-17T15:37:38Z

server/src/main/java/org/elasticsearch/action/search/CountResponse.java

+        }
+
+        @Override
+        public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException {


the XContent here does not match what you removed in the REST API. There was a bit about early termination, as well as count that you need to include. You likely need to also include the begin/end calls, or else this will fail tests.

You can check the tests with ./gradlew :server:check from the base of the checkout. If tests dont fail then we need some better tests around the response hehe

Hey think you're looking at XContent of ShardStats (which innner class of CountRequest), and ShardStats indeed handles only the _shards part of the response.

count is added up at line 150 of CountRequest, terminated_early at line 152, and then at line 154 CountRequest will make a call to inner class ShardStats.toXContent

I've added the radnomized tests which also include ShardFailures and terminated_early, and ./gradlew clean server:check runs fine

So as much as I'm aware of this part is ok (at least regarding this comment), can you please check again?

Hi, I was referring to this code, https://github.com/elastic/elasticsearch/blob/master/server/src/main/java/org/elasticsearch/rest/action/document/RestCountAction.java#L95-L104 , which you deleted as part of your PR.

Wow, you are totally right, I see that now :)

elastic#32711

mrdjen · 2018-09-17T17:03:45Z

hi @hub-cap
I've addressed the styling changes, regarding XContent see my comment inside, guess you've been looking at inner class ShardStats (which indeed handles only part of the XContent, while the count and termnate_early are in enclosing class CountRequest)

hub-cap · 2018-09-17T17:25:28Z

@elasticmachine test this please

hub-cap · 2018-09-17T17:26:00Z

@colings86 Ive added you as a reviewer because this is something we have discussed. Feel free to delegate :)

hub-cap · 2018-09-26T14:44:23Z

hey @mrdjen I had a chat w/ @colings86 and there is some history here that I was unaware of, that caused him to reach out to me. Historically we had server side count actions, like the ones youve proposed above. They were removed in favor of only a rest API in server. So this means that we will not be putting the request/response in server.

That being said, these classes are 100% necessary for the high level rest API. We are no longer reusing the server side APIs so we need to start putting them in the high level client project themselves. So the work you did was not in vain, we just need to move them into client and use them there, and not touch anything in server. Does this make sense?

mrdjen · 2018-09-27T09:23:42Z

No problem hub-cap, let me just do a quick recap -

Initially, I've submitted 31868, and as javanna suggested, CountRequest/Response was in HL client (also in 31868 CountRequest wraps SearchRequest).

A new PR was requested (this one) by hub-cap, move CountReq/Resp to server & inline ServerRequest members in CountRequest.

The last comment suggests using CountReq/Res as they are in this PR but moving back into HLRC (as in 31868) and use RestCountAction(s) as they are on master (and 31868), so overall solution is similar to 31868, but CountReq will not wrap SearchReq, instead CountReq inlines members like in this PR.

This does not sound like a big work and I'm eager to push this over the line. Can you guys please confirm/clarify if my understanding is correct and do you have any preferences regarding PR's (which to use to complete the remaining work)? @javanna, @hub-cap, @colings86

Note to the casual reader - read the conversation from the start of 31868, submitted back on July 6th.

Cheers! Milan

colings86 · 2018-09-27T09:32:00Z

@mrdjen Sorry for the back and forth on this one.

I'm +1 on having the CountRequest/Response in the HLRC and not in the server but with CountRequest not wrapping SearchRequest and would be happy for that to either be done in this PR or in a new PR as you see fit.

mrdjen · 2018-09-27T09:42:52Z

Thanks @colings86
I'll pick it up in next couple of days (and to give time for some other comments (in case there are))

javanna · 2018-09-27T10:13:13Z

thanks for your patience @mrdjen , greatly appreciated! A lot of work is going into the high-level REST client these days by the team and we were still defining the overall direction, hence the back and forth ;)

hub-cap · 2018-09-28T18:40:42Z

yea, we did totally drop the ball on this and give you a lot of rework. I take blame/responsibility. I am sorry @mrdjen , as it was my fault to tell you to move it in to server, and I did not know the history. Thank you for your persistence.

mrdjen · 2018-09-28T19:37:29Z

Hey @hub-cap no problem, I'm glad to contribute and there are many moving parts.
I'll make a new PR in couple of days and we can take it from there.
Cheers! Milan

mrdjen · 2018-10-03T12:13:57Z

Hi guys,
please see related #34267
@hub-cap @colings86 @javanna

hub-cap · 2018-10-30T16:05:06Z

@mrdjen we can close this right?

mrdjen · 2018-10-30T16:27:07Z

Yeah, closing it now @hub-cap

mrdjen · 2018-10-30T16:27:18Z

Thanks!

mrdjen mentioned this pull request Aug 8, 2018

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

Closed

colings86 added >enhancement :Core/Java High Level REST Client labels Aug 9, 2018

danielmitterdorfer requested a review from hub-cap August 10, 2018 05:49

mrdjen added 2 commits September 13, 2018 21:45

Merge branch 'master' into use_count_req_resp_in_restcountaction

e692d6f

hub-cap suggested changes Sep 17, 2018

View reviewed changes

resolve code review requested changes - styling

75dc066

elastic#32711

hub-cap requested a review from colings86 September 17, 2018 17:25

hub-cap approved these changes Sep 17, 2018

View reviewed changes

mrdjen closed this Oct 30, 2018

Add CountRequest/Response classes to server and use them in RestCountAction (cat and doc). #32711

Add CountRequest/Response classes to server and use them in RestCountAction (cat and doc). #32711

Uh oh!

Conversation

mrdjen commented Aug 8, 2018

Uh oh!

elasticmachine commented Aug 9, 2018

Uh oh!

hub-cap commented Aug 13, 2018

Uh oh!

mrdjen commented Aug 13, 2018

Uh oh!

hub-cap commented Aug 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mrdjen commented Aug 13, 2018

Uh oh!

hub-cap commented Aug 27, 2018

Uh oh!

mrdjen commented Aug 29, 2018

Uh oh!

hub-cap commented Aug 29, 2018

Uh oh!

mrdjen commented Aug 29, 2018

Uh oh!

hub-cap commented Sep 7, 2018

Uh oh!

mrdjen commented Sep 17, 2018

Uh oh!

hub-cap commented Sep 17, 2018

Uh oh!

hub-cap left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mrdjen commented Sep 17, 2018

Uh oh!

hub-cap commented Sep 17, 2018

Uh oh!

hub-cap commented Sep 17, 2018

Uh oh!

hub-cap commented Sep 26, 2018

Uh oh!

mrdjen commented Sep 27, 2018

Uh oh!

colings86 commented Sep 27, 2018

Uh oh!

mrdjen commented Sep 27, 2018

Uh oh!

javanna commented Sep 27, 2018

Uh oh!

hub-cap commented Sep 28, 2018

Uh oh!

mrdjen commented Sep 28, 2018

Uh oh!

mrdjen commented Oct 3, 2018

Uh oh!

hub-cap commented Oct 30, 2018

Uh oh!

hub-cap commented Aug 13, 2018 •

edited

Loading