Skip to content

Fix leaking searcher when shards are removed or relocated#52099

Merged
dnhatn merged 4 commits intoelastic:masterfrom
dnhatn:leak-context
Feb 10, 2020
Merged

Fix leaking searcher when shards are removed or relocated#52099
dnhatn merged 4 commits intoelastic:masterfrom
dnhatn:leak-context

Conversation

@dnhatn
Copy link
Copy Markdown
Member

@dnhatn dnhatn commented Feb 8, 2020 •
edited
Loading

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
Copy Markdown
Collaborator

elasticmachine commented Feb 8, 2020

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

@dnhatn dnhatn mentioned this pull request Feb 8, 2020
Closed
@mark-vieira
Copy link
Copy Markdown
Contributor

mark-vieira commented Feb 8, 2020 •
edited
Loading

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
Copy Markdown
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
jpountz
jpountz approved these changes Feb 9, 2020
View reviewed changes
Copy link
Copy Markdown
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
Copy Markdown
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
@dnhatn
Fix leaking searcher when shards are removed or relocated (#52099)
80a9a08 
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
@dnhatn
Revert "Mute ReindexFailureTests test"
6d0a0e1 
This reverts commit 16afbf9.

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

mark-vieira commented Feb 11, 2020

@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
Copy Markdown
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

Reviewers

1 more reviewer

@jpountz jpountz jpountz approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

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

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

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

5 participants

@dnhatn @elasticmachine @mark-vieira @jpountz @jakelandis