-
Notifications
You must be signed in to change notification settings - Fork 24.7k
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
Save memory in on aggs in async search #55683
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.
Pinging @elastic/es-analytics-geo (:Analytics/Aggregations) |
And it doesn't have any of the sneaky hidden costs. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I left one comment, I don't think we should apply the final reduce eagerly.
x-pack/plugin/async-search/src/main/java/org/elasticsearch/xpack/search/AsyncSearchTask.java
Outdated
Show resolved
Hide resolved
.../plugin/async-search/src/main/java/org/elasticsearch/xpack/search/MutableSearchResponse.java
Show resolved
Hide resolved
.../plugin/async-search/src/main/java/org/elasticsearch/xpack/search/MutableSearchResponse.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I discussed with Nik offline, he explained the logic and I understand better now. I left one comment for readability but the change looks good to me. Nice simplification @nik9000 .
run elasticsearch-ci/2 |
run elasticsearch-ci/bwc |
run elasticsearch-ci/2 |
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.
Backport done! This was a blocker because we really didn't want to release 7.8 with the aggs referenced both in serialized and unserialized form. It just seemed like a big waste! |
This commit fixes the initialization of total hits in the async search response. Relates elastic#55683 Closes elastic#55920
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.