Introduce SearchResponseSections base class #24442

javanna · 2017-05-02T20:52:27Z

SearchResponseSections is the common part extracted from InternalSearchResponse that can be shared between high level REST and elasticsearch. The only bits left out are around serialization which is not supported. This way it can accept Aggregations as a constructor argument, without requiring InternalAggregations, as the high level REST client uses its own objects for aggs parsing rather than internal ones.

This change also makes Aggregations implement ToXContent, and Aggregation extend ToXContent. Especially the latter is suboptimal but the best solution that allows to share as much code as possible between core and the client, that doesn't require messing with generics and making the api complicated. Also it doesn't have downsides as all of the current implementations of Aggregation do implement ToXContent already.

The idea is that with this change we can go back to #22533, add fromXContent to SearchResponse, where we will be able to return a search response created by passing in an instance of SearchResponseSections rather than InternalSearchResponse. SearchResponseSections holds Aggregations rather than InternalAggregations, which allows the high level REST client not to use InternalAggregation instances for aggs parsing, rather ParsedAggregation

SearchResponseSections is the common part extracted from InternalSearchResponse that can be shared between high level REST and elasticsearch. The only bits left out are around serialization which is not supported. This way it can accept Aggregations as a constructor argument, without requiring InternalAggregations, as the high level REST client uses its own objects for aggs parsing rather than internal ones. This change also makes Aggregations implement ToXContent, and Aggregation extend ToXContent. Especially the latter is suboptimal but the best solution that allows to share as much code as possible between core and the client, that doesn't require messing with generics and making the api complicated. Also it doesn't have downsides as all of the current implementations of Aggregation do implement ToXContent already.

tlrx

This looks nice, I left some comments/questions

tlrx · 2017-05-03T07:56:49Z

core/src/main/java/org/elasticsearch/action/search/SearchResponseSections.java

Thanks for the documentation. We could also change SearchResponseSections so that it does not implement Writeable here and later cast SearchResponse's internalResponse to InternalSearchResponse in SearchResponse.writeTo(StreamOutput) ? I think it's acceptable since SearchResponse.readFrom() already expects an InternalSearchResponse.

the subtle difference there is that if you try to serialize the parsed search response by calling writeTo you get back a class cast exception rather than an unsupported operation exception. I think it's ok to create internal search response in readFrom as we can control that. I think I will try to have the inner class not implement Writeable, yet leave the writeTo method that throws exception there.

tlrx · 2017-05-03T07:58:12Z

core/src/main/java/org/elasticsearch/action/search/SearchResponse.java

What about BaseSearchResponse? I find this sections suffix confusing, but maybe it's just me.

BaseSearchResponse makes me think that it has to do with its ancestor. I went for sections because it holds the different sections of the search response, in the same spirit as Aggregations.

tlrx · 2017-05-03T08:02:37Z

core/src/main/java/org/elasticsearch/search/aggregations/Aggregations.java

aggregations is not initialized with emptyList() anymore, I think this might cause an issue here

yea I forgot to push a commit. I had already fixed this, good catch.

tlrx · 2017-05-03T08:04:30Z

core/src/main/java/org/elasticsearch/search/internal/InternalSearchResponse.java

Writeable is not necessary here

I left it as SearchREsponseSections doesn't implement Writeable anymore

Sure, thanks. I prefer this, SearchResponseSections not declaring that it implements Writeable but still providing a writeTo method.

tlrx · 2017-05-03T08:09:04Z

core/src/main/java/org/elasticsearch/search/internal/InternalSearchResponse.java

Do you think we could add some @Nullable here?

Maybe, I tried to limit the unrelated changes here as it will go in our branch. Does the need for Nullable come from my changes?

Does the need for Nullable come from my changes?

No, just thinking that it would be useful here.

javanna · 2017-05-03T08:26:08Z

can you have another look @tlrx ?

javanna · 2017-05-03T08:27:33Z

core/src/main/java/org/elasticsearch/search/aggregations/Aggregations.java

+        return builder;
+    }
+
+    //TODO add tests for this method


would you mind if I add tests as a follow-up so we can get this in rather quickly?

tlrx

LGTM, thanks

SearchResponseSections is the common part extracted from InternalSearchResponse that can be shared between high level REST and elasticsearch. The only bits left out are around serialization which is not supported. This way it can accept Aggregations as a constructor argument, without requiring InternalAggregations, as the high level REST client uses its own objects for aggs parsing rather than internal ones. This change also makes Aggregations implement ToXContent, and Aggregation extend ToXContent. Especially the latter is suboptimal but the best solution that allows to share as much code as possible between core and the client, that doesn't require messing with generics and making the api complicated. Also it doesn't have downsides as all of the current implementations of Aggregation do implement ToXContent already.

javanna added :Java High Level REST Client >enhancement labels May 2, 2017

javanna requested a review from tlrx May 2, 2017 20:52

tlrx reviewed May 3, 2017

View reviewed changes

SearchResponseSections to not implement Writeable

3749a8c

javanna force-pushed the enhancement/parsed_aggs_search_response branch from a48dbb7 to 3749a8c Compare May 3, 2017 08:24

javanna commented May 3, 2017

View reviewed changes

tlrx approved these changes May 3, 2017

View reviewed changes

javanna merged commit fbd793d into elastic:feature/client_aggs_parsing May 3, 2017

tlrx mentioned this pull request May 3, 2017

Add parsing methods for InternalDateHistogram and InternalHistogram #24213

Merged

javanna mentioned this pull request May 22, 2017

Add aggs parsers for high level REST Client #24824

Merged

javanna mentioned this pull request May 23, 2017

Backport aggs parsers for high level REST Client #24844

Merged

Introduce SearchResponseSections base class #24442

Introduce SearchResponseSections base class #24442

Uh oh!

Conversation

javanna commented May 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tlrx 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

javanna commented May 3, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tlrx left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

javanna commented May 2, 2017 •

edited

Loading