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

Replace NestedChildrenQuery with ParentChildrenBlockJoinQuery #24016

Conversation

martijnvg
Copy link
Member

@martijnvg martijnvg commented Apr 10, 2017

PR for #24009

Before this change if NestedChildrenQuery were to be cached it could lead to memory leak, because this query keeps a reference to the IndexReader. The chance that it would be cached is low, because this query is different for each search request and search hit it is trying to fetch inner hits for.

Copy link
Contributor

@jpountz jpountz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great!

@jpountz
Copy link
Contributor

jpountz commented Apr 10, 2017

Should it be merged to 5.3.1? cc @clintongormley

@jpountz
Copy link
Contributor

jpountz commented Apr 10, 2017

ParentChildrenBlockJoinQuery only exists in Lucene 6.5 but it should be easy to copy/paste if we wanted that change to be in Elasticsearch 5.3?

@martijnvg
Copy link
Member Author

ParentChildrenBlockJoinQuery only exists in Lucene 6.5 but it should be easy to copy/paste if we wanted that change to be in Elasticsearch 5.3?

If we are ok with backporting this to 5.3 branch, then I'll make it in a different pr.

@martijnvg martijnvg force-pushed the inner_hits_use_ParentChildrenBlockJoinQuery branch from 3eea8e4 to 887f3ed Compare April 10, 2017 15:37
@martijnvg martijnvg merged commit 887f3ed into elastic:master Apr 10, 2017
@jpountz
Copy link
Contributor

jpountz commented Apr 10, 2017

@martijnvg Can you update the description of this PR so that it better explains the bug it fixes?

If we are ok with backporting this to 5.3 branch, then I'll make it in a different pr.

+1

@martijnvg
Copy link
Member Author

@jpountz updated the description.

@clintongormley clintongormley added :Search/Search Search-related issues that do not fall into other categories and removed :Inner Hits labels Feb 14, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>bug :Search/Search Search-related issues that do not fall into other categories v5.4.0 v6.0.0-alpha1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants