Save memory in on aggs in async search #55083
Conversation
This replaces a reference to the result of partially reducing aggregations that async search keeps with a reference to the serialized form of the result of the partial reduction which we need to keep anyway.
nik9000
added
>enhancement
blocker
:Analytics/Aggregations
|
Pinging @elastic/es-analytics-geo (:Analytics/Aggregations) |
|
I've marked this a blocker for 7.8.0 because we'd really like to avoid releasing with the aggs referenced twice in async search. We have plenty of time to get it or something like it merged. |
|
Follow up to #54758 (review) |
|
Test failures are real. Looks like I should have run more of the tests before opening! |
|
I could have made aggs from remote clusters delayed but that'd have effected the transport client and, maybe, other things. I'll investigate it, but I don't want to "sneak" that in. |
jimczi
left a comment
jimczi
left a comment
There was a problem hiding this comment.
The change makes sense to me. I left one comment regarding the usage of InternalSearchResponse#aggregations that should be addressed.
| if (aggregations == null) { | ||
| throw new IllegalStateException("aggregations already consumed"); | ||
| } | ||
| return aggregations.get(); |
There was a problem hiding this comment.
I am worried about the hidden cost of calling this on serialized aggs. Can we at least ensure that we don't get aggregations twice like here ?
|
I took another look at this one with a week's distance and I hate it. I'm going to give it another shot in another way. This morning I'll be experimenting with async search locally to see how it all works..... |
|
Replaced by #55683. |
3 participants
This replaces a reference to the result of partially reducing
aggregations that async search keeps with a reference to the serialized
form of the result of the partial reduction which we need to keep
anyway.