Remove InternalSearchResponse as well as most usage of SearchResponseSections #103583
Conversation
…Sections This indirection isn't really necessary now that its use in the high level rest client went away. -> flattened the `SearchResponse` class to make ref counting easier -> removed `InternalSearchResponse` whose functionality was completely redundant now -> removed serialization and most of the usage of `SearchResponseSections`, only keeping this for the few search execution spots that currently use it and where removing it won't get us anything and will be a huge changeset.
original-brownbear
added
:Search/Search
|
Pinging @elastic/es-search (Team:Search) |
| @@ -212,7 +212,9 @@ public final void start() { | |||
| // total hits is null in the response if the tracking of total hits is disabled | |||
| boolean withTotalHits = trackTotalHitsUpTo != SearchContext.TRACK_TOTAL_HITS_DISABLED; | |||
| sendSearchResponse( | |||
| withTotalHits ? InternalSearchResponse.EMPTY_WITH_TOTAL_HITS : InternalSearchResponse.EMPTY_WITHOUT_TOTAL_HITS, | |||
| withTotalHits | |||
| ? new SearchResponseSections(SearchHits.EMPTY_WITH_TOTAL_HITS, null, null, false, null, null, 1) | |||
There was a problem hiding this comment.
I inlined these, we don't really want constants for stuff that becomes ref-counted. We can make this nicer later one at some point IMO.
| private final Supplier<SearchPhase> nextPhase; | ||
|
|
||
| ExpandSearchPhase(SearchPhaseContext context, InternalSearchResponse searchResponse, Supplier<SearchPhase> nextPhase) { | ||
| ExpandSearchPhase(SearchPhaseContext context, SearchHits searchHits, Supplier<SearchPhase> nextPhase) { |
There was a problem hiding this comment.
We only use the hits, so lets only pass around the hits => only have to worry about ref-counting the hits :)
| @@ -250,7 +248,7 @@ protected final void sendResponse( | |||
| ActionListener.respondAndRelease( | |||
| listener, | |||
| new SearchResponse( | |||
| internalResponse, | |||
| SearchPhaseController.merge(true, queryPhase, fetchResults), | |||
There was a problem hiding this comment.
Inlining this stuff so follow-up ref counting becomes easier.
| import java.io.IOException; | ||
|
|
||
| /** | ||
| * {@link SearchResponseSections} subclass that can be serialized over the wire. |
There was a problem hiding this comment.
No need for that any longer
| * The reason why this class exists is that the high level REST client uses its own classes | ||
| * to parse aggregations into, which are not serializable. This is the common part that can be | ||
| * shared between core and client. | ||
| * Holds some sections that a search response is composed of (hits, aggs, suggestions etc.) during some steps of the search response |
There was a problem hiding this comment.
Does it make sense to mark this class as deprecated?
There was a problem hiding this comment.
Maybe not, I was trying to remove it altogether but the few use cases where this is used response merging logic actually look nicer having a record style object like this rather than a bunch of parameters I think. I was gonna double check that point though, maybe not all of these fields are needed and that becomes not true anymore :) lets see 🤞
|
Thanks Carlos! |
original-brownbear
added
the
auto-merge
|
FWIW, SearchResponseSections was introduced when we added response parsing for the high level REST client. Given that HLRC no longer exists, it kind of make sense to clean that up. |
…Sections (elastic#103583) This indirection isn't really necessary now that its use in the high level rest client went away. -> flattened the `SearchResponse` class to make ref counting easier -> removed `InternalSearchResponse` whose functionality was completely redundant now -> removed serialization and most of the usage of `SearchResponseSections`, only keeping this for the few search execution spots that currently use it and where removing it won't get us anything and will be a huge changeset. enables the last steps in elastic#102030
This indirection isn't really necessary now that its use in the high level rest client went away. -> flattened the
SearchResponseclass to make ref counting easier -> removedInternalSearchResponsewhose functionality was completely redundant now -> removed serialization and most of the usage ofSearchResponseSections, only keeping this for the few search execution spots that currently use it and where removing it won't get us anything and will be a huge changeset.enables the last steps in #102030