Streamline top level reader close listeners and forbid general usage #14084
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| @@ -76,4 +74,38 @@ public LeafReader wrap(LeafReader reader) { | |||
| } | |||
| } | |||
|
|
|||
| @SuppressForbidden(reason = "This is the only sane way to add a ReaderClosedListener") | |||
| public static void addReaderCloseListener(IndexReader reader, IndexReader.ReaderClosedListener listener) { | |||
There was a problem hiding this comment.
It would be good in the future if we could refine this to just DirectoryReader? Really IMO nobody should be installing a readerCloseListener on a LeafReader... caching etc should always be managed via coreCloseListener
|
I like the changes, thanks for improving the type safety. when merging master you can drop changes in Versions.java as it now uses a core-closed listener. |
IndexReader#addReaderCloseListener is very error prone when it comes to caching and reader wrapping. The listeners are not delegated to the sub readers nor can it's implementation change since it's final in the base class. This commit only allows installing close listeners on the top level ElasticsearchDirecotryReader which is known to work an has a defined lifetime which corresponds to its subreader. This ensure that cachesa re cleared once the reader goes out of scope.
this commit also fixes a bug where we wramed a leaf reader in a top level context which caused atomic segment readers to be used in our top level caches.
s1monw
force-pushed
the
close_the_wrapper_only
branch
from
8340fa7 to
e3f00e3
Compare
| if (reader() instanceof DirectoryReader) { | ||
| return (DirectoryReader) reader(); | ||
| } | ||
| throw new IllegalStateException("Can't use " + reader().getClass() + " as an directory reader"); |
| } | ||
| } | ||
|
|
||
| final class NonClosingReaderWrapper extends FilterDirectoryReader { |
|
LGTM, just left very minor comments |
|
looks good here too. i have nothing to add to adrien's suggestions. this is a good cleanup! |
s1monw
added a commit
that referenced
this pull request
Oct 14, 2015
Streamline top level reader close listeners and forbid general usage
s1monw
added a commit
that referenced
this pull request
Oct 14, 2015
Streamline top level reader close listeners and forbid general usage
s1monw
added a commit
to s1monw/elasticsearch
that referenced
this pull request
Oct 14, 2015
Streamline top level reader close listeners and forbid general usage
s1monw
added a commit
to s1monw/elasticsearch
that referenced
this pull request
Oct 14, 2015
Streamline top level reader close listeners and forbid general usage
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
IndexReader#addReaderCloseListener is very error prone when it comes to
caching and reader wrapping. The listeners are not delegated to the sub readers
nor can it's implementation change since it's final in the base class. This commit
only allows installing close listeners on the top level ElasticsearchDirecotryReader
which is known to work an has a defined lifetime which corresponds to its subreader.
This ensure that caches are cleared once the reader goes out of scope.