Pass split shard count summary through acquireSearcher #138259
Conversation
This will allow us to install search filters on shards being split according to whether the coordinating node is including new search shards in its requests or not.
bcully
added
>non-issue
:Distributed Indexing/Distributed
elasticsearchmachine
added
the
Team:Distributed Indexing
|
Pinging @elastic/es-distributed-indexing (Team:Distributed Indexing) |
|
|
||
| // Allows subclasses to wrap the DirectoryReader before it is used to create Searchers | ||
| protected DirectoryReader wrapDirectoryReader(DirectoryReader reader) throws IOException { | ||
| protected DirectoryReader wrapDirectoryReader(DirectoryReader reader, SplitShardCountSummary ignored) throws IOException { |
There was a problem hiding this comment.
this is not a good abstraction. Open to better ideas!
There was a problem hiding this comment.
This is always called from acquireSearcherSupplier so maybe we can introduce AcquireSearcherRequest and pass it down here? Looking at naming in SearcherScope, something like having two variant AcquireSearcherRequest.Internal and AcquireSearcherRequest.External may work. I am not sure if summary is needed for internal searchers, it feels like it won't be?
There was a problem hiding this comment.
We had a chat about this on slack, and I ended up refactoring this a little to make the wrapper only apply to external searches, but I think it's going to be easier to decide on the most generic way to apply the summary to the wrapper once we have all the calling paths figured out so we don't have all the default SplitShardCountSummary.UNSET paths.
…erSupplier method This is the one that is called by all the others. Should fix the test cases I looked at.
…lly/elasticsearch into reshard-install-source-search-filter
After reviewing the users of SearcherScope.INTERNAL none of them appear to want filtering, so it is simpler and safer not to provide an interface for them to do so.
…install-source-search-filter
elasticsearchmachine
added
the
serverless-linked
| return acquireSearcherSupplier(Engine.SearcherScope.EXTERNAL); | ||
| } | ||
|
|
||
| public Engine.SearcherSupplier acquireSearcherSupplier(SplitShardCountSummary splitShardCountSummary) { |
There was a problem hiding this comment.
Can we reflect that this is for external scopes only in the name of the method?
There was a problem hiding this comment.
We could. I wasn't sure about it because the signature for the overload below acquireSearcherSupplier(Engine.SearcherScope scope) is public, but I guess we're going to be slowly trying to remove callers of it and maybe mark it deprecated due to the fact that it doesn't have a shard count summary in the external case, in favor of calling internal and external acquirers.
There was a problem hiding this comment.
Yes, that's my expectation as well.
…install-source-search-filter
Call the variant that takes a splitShardCountSummary acquireExternalSearcherSupplier.
This will allow us to install search filters on shards being split according to whether the coordinating node is including new search shards in its requests or not. Move wrapDirectoryReader to wrapExternalDirectoryReader. After reviewing the users of SearcherScope.INTERNAL none of them appear to want filtering, so it is simpler and safer not to provide an interface for them to do so.
This will allow us to install search filters on shards being split according to whether the coordinating node is including new search shards in its requests or not. Move wrapDirectoryReader to wrapExternalDirectoryReader. After reviewing the users of SearcherScope.INTERNAL none of them appear to want filtering, so it is simpler and safer not to provide an interface for them to do so.
3 participants
This will allow us to install search filters on shards being split according to whether the coordinating node is including new search shards in its requests or not.