Skip to content
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

Merged
merged 8 commits into from
Apr 28, 2020

Conversation

nik9000
Copy link
Member

@nik9000 nik9000 commented Apr 23, 2020

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.

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.
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-analytics-geo (:Analytics/Aggregations)

@nik9000
Copy link
Member Author

nik9000 commented Apr 23, 2020

@jimczi, I think this is much simpler than #55083 and amounts to the same change.

@nik9000
Copy link
Member Author

nik9000 commented Apr 23, 2020

And it doesn't have any of the sneaky hidden costs.

@nik9000 nik9000 requested review from jimczi and javanna April 23, 2020 17:57
Copy link
Contributor

@jimczi jimczi left a 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.

Copy link
Contributor

@jimczi jimczi left a 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 .

@nik9000
Copy link
Member Author

nik9000 commented Apr 28, 2020

run elasticsearch-ci/2

@nik9000
Copy link
Member Author

nik9000 commented Apr 28, 2020

run elasticsearch-ci/bwc

@nik9000
Copy link
Member Author

nik9000 commented Apr 28, 2020

run elasticsearch-ci/2

@nik9000 nik9000 merged commit 55874c9 into elastic:master Apr 28, 2020
nik9000 added a commit to nik9000/elasticsearch that referenced this pull request Apr 28, 2020
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 a commit that referenced this pull request Apr 28, 2020
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
Copy link
Member Author

nik9000 commented Apr 28, 2020

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!

jimczi added a commit to jimczi/elasticsearch that referenced this pull request Apr 29, 2020
This commit fixes the initialization of total hits
in the async search response.

Relates elastic#55683
Closes elastic#55920
jimczi added a commit that referenced this pull request Apr 29, 2020
This commit fixes the initialization of total hits
in the async search response.

Relates #55683
Closes #55920
jimczi added a commit that referenced this pull request Apr 29, 2020
This commit fixes the initialization of total hits
in the async search response.

Relates #55683
Closes #55920
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants