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.

Sign up for GitHub

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

Jump to bottom

Fix memory leak when percolator uses bitset or field data cache #24115

Merged
merged 1 commit into from
Apr 26, 2017
Merged

Fix memory leak when percolator uses bitset or field data cache #24115

merged 1 commit into from
Apr 26, 2017

Conversation

martijnvg
Copy link
Member

The percolator doesn't close the IndexReader of the memory index any more.
Prior to 2.x the percolator had its own SearchContext (PercolatorContext) that did this,
but that was removed when the percolator was refactored as part of the 5.0 release.

I think an alternative way to fix this is to let percolator not use the bitset and fielddata caches,
that way we prevent the memory leak.

Adding a WIP label to this as I'm not happy with the current test. It is not easy to test that we don't use the bitset or fielddata cache for the percolator, because non percolator operations may use these caches, which is valid.

PR for #24108

@martijnvg martijnvg added :Search Relevance/Percolator Reverse search: find queries that match a document >bug WIP labels Apr 14, 2017
martijnvg
martijnvg commented Apr 14, 2017
View reviewed changes
modules/percolator/src/main/java/org/elasticsearch/percolator/PercolateQueryBuilder.java
@Override
@SuppressWarnings("unchecked")
public <IFD extends IndexFieldData<?>> IFD getForField(MappedFieldType fieldType) {
IndexFieldData.Builder builder = fieldType.fielddataBuilder();
Copy link
Member Author

Choose a reason for hiding this comment

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

Still need to think how to add a test for this too...

@jpountz
Copy link
Contributor

jpountz commented Apr 19, 2017

Ideally we would use the SearchContext.addReleasable API in order to tell ES to close the index reader once the request is processed. However it is not easy today to find the right place to get access to the search context...

@martijnvg
Copy link
Member Author

@jpountz What do you think about adding a QueryShardContext#addReleasable(...) method that delegates to SearchContext#addReleasable(...)? Then we can close the index readers the percolate query builder creates.

@jpountz
Copy link
Contributor

jpountz commented Apr 19, 2017

If you can find a way to do that, that would be great!

@martijnvg martijnvg force-pushed the percolator_do_not_use_bitset_fielddata_cache branch from ef6fe92 to 461767b Compare April 19, 2017 19:29
@martijnvg
Copy link
Member Author

@jpountz what do you think of the current approach in this pr?

jpountz
jpountz reviewed Apr 24, 2017
View reviewed changes
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.

Thanks for looking into it, I think what you are suggesting would work, we just need to make the wiring a bit nicer?

core/src/main/java/org/elasticsearch/index/query/QueryShardContext.java Outdated

public void setDelegate(Consumer<Releasable> delegate) {
this.delegate = delegate;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

should it be a constructor arg?

Copy link
Member Author

Choose a reason for hiding this comment

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

done - it is now a final field

core/src/main/java/org/elasticsearch/index/query/QueryShardContext.java Outdated
@@ -98,6 +100,10 @@ public void setTypes(String... types) {
private NestedScope nestedScope;
private boolean isFilter;

private Consumer<Releasable> delegate = (releasable) -> {
Copy link
Contributor

Choose a reason for hiding this comment

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

let's try to make it final and give it a better name?

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't have suggestions though...

Copy link
Member Author

Choose a reason for hiding this comment

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

Me neither... I've named it now releaseDelegator

@martijnvg martijnvg force-pushed the percolator_do_not_use_bitset_fielddata_cache branch from 461767b to ce08211 Compare April 24, 2017 11:48
@martijnvg martijnvg added review and removed WIP labels Apr 24, 2017
jpountz
jpountz reviewed Apr 24, 2017
View reviewed changes
core/src/main/java/org/elasticsearch/index/IndexService.java Outdated
@@ -472,11 +473,19 @@ public IndexSettings getIndexSettings() {
* {@link IndexReader}-specific optimizations, such as rewriting containing range queries.
*/
public QueryShardContext newQueryShardContext(int shardId, IndexReader indexReader, LongSupplier nowInMillis) {
Copy link
Contributor

Choose a reason for hiding this comment

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

should we just remove this one completely?

Copy link
Member Author

Choose a reason for hiding this comment

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

That method is still being used in other places. For example in MetaDataCreateIndexService and MetaDataIndexAliasesService. In these places we don't have access to a search context and we test there if alias filters parse correctly, which means we will not be supporting percolate queries there, which I think is ok.

Copy link
Member Author

Choose a reason for hiding this comment

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

I removed this method, wdyt?

@martijnvg martijnvg force-pushed the percolator_do_not_use_bitset_fielddata_cache branch 2 times, most recently from c67a66a to 161661e Compare April 26, 2017 07:57
@martijnvg
Copy link
Member Author

I've reverted this PR back to the initial approach (replacing field data and bitset cache in a custom QueryShardContext implementation) that was used to fix the problem. The change that added infrastructure for QueryShardContext#addReleasable(...) that allowed closing in-memory index is invasive and considering that this had to be backported it is better to use the initial fix as that change is mostly in one place. Closing the in-memory index is the right approach, but it should be explored in a followup change, in the meantime this change will fix the reported bug.

@martijnvg
[percolator] Fix memory leak when percolator uses bitset or field dat…
c17de49 
…a cache.

The percolator doesn't close the IndexReader of the memory index any more.
Prior to 2.x the percolator had its own SearchContext (PercolatorContext) that did this,
but that was removed when the percolator was refactored as part of the 5.0 release.

I think an alternative way to fix this is to let percolator not use the bitset and fielddata caches,
that way we prevent the memory leak.

Closes elastic#24108
@martijnvg martijnvg force-pushed the percolator_do_not_use_bitset_fielddata_cache branch from 161661e to c17de49 Compare April 26, 2017 09:10
@martijnvg martijnvg merged commit c17de49 into elastic:master Apr 26, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jpountz jpountz jpountz approved these changes

Assignees
No one assigned
Labels
>bug :Search Relevance/Percolator Reverse search: find queries that match a document v5.4.0 v5.5.0 v6.0.0-alpha1
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@martijnvg @jpountz @clintongormley