Refactor index engines to manage readers instead of searchers #43860
Conversation
This commit changes the way we manage refreshes in the index engines. Instead of relying on a SearcherManager, this change uses a ReaderManager that creates ElasticsearchDirectoryReader when needed. Searchers are now created on-demand (when acquireSearcher is called) from the current ElasticsearchDirectoryReader. It also slightly changes the Engine.Searcher to extend IndexSearcher in order to simplify the usage in the consumer.
jimczi
added
>enhancement
:Search/Search
|
Pinging @elastic/es-distributed |
|
Pinging @elastic/es-search |
s1monw
left a comment
s1monw
left a comment
There was a problem hiding this comment.
Looks good, left some minors
| private final Closeable onClose; | ||
|
|
||
| public Searcher(String source, IndexSearcher searcher, Closeable onClose) { | ||
| public Searcher(String source, IndexReader reader, |
There was a problem hiding this comment.
any chance we can make this ElasticsearchDirectoryReader instead of IndexReader?
There was a problem hiding this comment.
The ElasticsearchDirectoryReader can be wrapped by plugins so we'd need to move/recreate one when we create the Searcher. I agree that it would be simpler to always force the ElasticsearchDirectoryReader at the top though so I'll give it a try.
There was a problem hiding this comment.
I forgot the TranslogLeafReader that is used for get requests so this would require a bigger change and more refactoring. Maybe better to do this in a follow up ?
There was a problem hiding this comment.
sure ++ no rush I didn't anticipate this one either
| return searcher.getIndexReader(); | ||
| } | ||
|
|
||
| public DirectoryReader getDirectoryReader() { |
There was a problem hiding this comment.
if we can move to a concrete reader then we can make this a hard cast to ElasticsearchDirectoryReader
| } | ||
| } | ||
|
|
||
| private void notifyListener(ElasticsearchDirectoryReader reader, ElasticsearchDirectoryReader previousReader) throws IOException { |
There was a problem hiding this comment.
can we move the reference stealing into this method too?
There was a problem hiding this comment.
then we can also do this return incrementAndNotify(acquire, referenceToRefresh)
|
|
||
| private void notifyListener(ElasticsearchDirectoryReader reader, ElasticsearchDirectoryReader previousReader) throws IOException { | ||
| boolean success = false; | ||
| try { |
There was a problem hiding this comment.
you can maybe do this:
reader.incRef(); // steal the reference
try (Closeable c = reader::decRef) {
refreshListener.accept(reader, previousReader);
reader.incRef(); // double inc-ref if we were successful
return reader;
}| if (renewed) { | ||
| // refresh outside of the write lock | ||
| // we have to refresh internal searcher here to ensure we release unreferenced segments. | ||
| // we have to refresh internal readr here to ensure we release unreferenced segments. |
|
@s1monw , the |
|
Thanks @s1monw |
This commit changes the way we manage refreshes in the index engines. Instead of relying on a SearcherManager, this change uses a ReaderManager that creates ElasticsearchDirectoryReader when needed. Searchers are now created on-demand (when acquireSearcher is called) from the current ElasticsearchDirectoryReader. It also slightly changes the Engine.Searcher to extend IndexSearcher in order to simplify the usage in the consumer.
colings86
removed
the
:Search/Search
6 participants
This commit changes the way we manage refreshes in the index engines.
Instead of relying on a SearcherManager, this change uses a ReaderManager that
creates ElasticsearchDirectoryReader when needed. Searchers are now created on-demand
(when acquireSearcher is called) from the current ElasticsearchDirectoryReader.
It also slightly changes the Engine.Searcher to extend IndexSearcher in order
to simplify the usage in the consumer.