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

Acquire seacher on closing engine should throw AlreadyClosedException #33331

Merged
merged 4 commits into from
Sep 5, 2018
Merged

Acquire seacher on closing engine should throw AlreadyClosedException #33331

merged 4 commits into from
Sep 5, 2018

Conversation

dnhatn
Copy link
Member

@dnhatn dnhatn commented Sep 1, 2018

Closes #33330

@dnhatn dnhatn added >bug review v7.0.0 :Distributed Indexing/Engine Anything around managing Lucene and the Translog in an open shard. v6.5.0 labels Sep 1, 2018
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-distributed

@dnhatn dnhatn requested review from s1monw and bleskes September 2, 2018 19:48
s1monw
s1monw approved these changes Sep 3, 2018
View reviewed changes
Copy link
Contributor

@s1monw s1monw left a comment

Choose a reason for hiding this comment

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

LGTM left one comment.

server/src/main/java/org/elasticsearch/index/engine/InternalEngine.java Outdated
* the searcher is acquired. */
store.incRef();
Releasable releasable = store::decRef;
ensureOpen();
Copy link
Contributor

Choose a reason for hiding this comment

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

are we doing this just for cosmetic reasons? because if we can't inc the store we will fail here with IllegalStateException if we don't want to do this we should maybe use if (store.tryIncRef()) and if we can't throw an already closed Exception? I jsut wanna make sure this is known that this is best effort.

Copy link
Member Author

Choose a reason for hiding this comment

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

Nice suggestion. I replaced it with tryIncRef.

@dnhatn
Copy link
Member Author

dnhatn commented Sep 5, 2018

Thanks @s1monw.

@dnhatn dnhatn merged commit 41839cf into elastic:master Sep 5, 2018
@dnhatn dnhatn deleted the throw-ex-closing-engine branch September 5, 2018 23:03
dnhatn added a commit that referenced this pull request Sep 6, 2018
@dnhatn
Acquire seacher on closing engine should throw ACE (#33331)
ac3d321 
Closes #33330
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@s1monw s1monw s1monw approved these changes

@bleskes bleskes Awaiting requested review from bleskes

Assignees
No one assigned
Labels
>bug :Distributed Indexing/Engine Anything around managing Lucene and the Translog in an open shard. v6.5.0 v7.0.0-beta1
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@dnhatn @elasticmachine @s1monw @colings86