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

Fix leaking searcher when shards are removed or relocated #52099

Merged
merged 4 commits into from
Feb 10, 2020

Conversation

dnhatn
Copy link
Member

@dnhatn dnhatn commented Feb 8, 2020

We might leak a searcher if the target shard is removed (i.e., its index is deleted) or relocated while we are creating a SearchContext from a SearchRewriteContext.

Relates #51708
Closes #52021

I labelled this non-issue for an unreleased bug introduced in #51708.

@dnhatn dnhatn added >non-issue :Search/Search Search-related issues that do not fall into other categories v8.0.0 v7.7.0 labels Feb 8, 2020
@dnhatn dnhatn requested a review from jpountz February 8, 2020 21:25
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-search (:Search/Search)

@mark-vieira
Copy link
Contributor

mark-vieira commented Feb 8, 2020

Since this addresses the failures mentioned in #52021 then we should probably revert the mute I did as part of that in 16afbf9 in this PR while we're at it.

@dnhatn
Copy link
Member Author

dnhatn commented Feb 8, 2020

@mark-vieira Thanks for the head-up. Since you muted this suite on 7.x only, I will unmute it when backporting this PR.

@dnhatn dnhatn changed the title Fix leaking searcher when index got deleted Fix leaking searcher when shard are removed or relocated Feb 9, 2020
@dnhatn dnhatn changed the title Fix leaking searcher when shard are removed or relocated Fix leaking searcher when shards are removed or relocated Feb 9, 2020
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.

LGTM, maybe also update the comment in the finally block so that it doesn't look like the DefaultSearchContext constructor is the only call that may fail?

@dnhatn
Copy link
Member Author

dnhatn commented Feb 10, 2020

maybe also update the comment in the finally block so that it doesn't look like the DefaultSearchContext constructor is the only call that may fail

++. I've adjusted the comment in ad5fe76. Thank you for reviewing, Adrien.

@dnhatn dnhatn merged commit e277034 into elastic:master Feb 10, 2020
@dnhatn dnhatn deleted the leak-context branch February 10, 2020 03:04
dnhatn added a commit that referenced this pull request Feb 10, 2020
We might leak a searcher if the target shard is removed (i.e., its index
is deleted) or relocated while we are creating a SearchContext from a
SearchRewriteContext.

Relates #51708
Closes #52021

I labelled this non-issue for an unreleased bug introduced in #51708.
dnhatn added a commit that referenced this pull request Feb 10, 2020
This reverts commit 16afbf9.

The issue was fixed in #52099
@mark-vieira
Copy link
Contributor

@dnhatn I think this needs to be backported to 7.6 as well. I just encountered this failure:

https://gradle-enterprise.elastic.co/s/zhx3wtz3etvxe/tests/kyv2y2z3r4v7m-ncgrhme3gg7g4

@dnhatn
Copy link
Member Author

dnhatn commented Feb 12, 2020

@mark-vieira This PR fixed a bug introduced in #51708, which got into only 8.0 and 7.7. Anyway, I will take a look at the failure that you linked. Thank you for the ping.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>non-issue :Search/Search Search-related issues that do not fall into other categories v7.7.0 v8.0.0-alpha1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[CI] Multiple tests failing with "some shards are still open" error
5 participants