Add the ability to wrap an index searcher.

[martijnvg]

No description provided.

[rjernst]

Why a Set? The wrapping order below would then be in an indeterminate order. But why support multiple at all? Can't someone who wants to add multiple layers of IndexSearcher wrappers do it with a single IndexSearcherWrapper?

[martijnvg]

Why a Set?

This gets injected via Guice and there is no list binder, just a set binder. We can add IndexSearcherWrapper#order() that returns an integer and change the incoming Set to a list and sort based on the order?

Can't someone who wants to add multiple layers of IndexSearcher wrappers do it with a single IndexSearcherWrapper?

They could, but if multiple plugins like to independently decorate the IndexSearcher then that wouldn't possible if only a single instance can be specified. I don't have a concrete example... but I like to be flexible around this.

[martijnvg]

I updated the PR and added IndexSearcherWrapper#order() method to have control over the order of which the IndexSearchWrapper instances do their wrapping.

I think as a follow up issue we should implement a IndexSearcherWrapper that applies the alias filters if there are any. I think this will be cleaner then how do this currently via the ContextIndexSearcher. Also this will unify logic between search and percolate api with regard to applying filtered aliases.

[jpountz]

Would EngineSearcherFactory be a better place to do the wrapping? Otherwise I'm concerned we might not call incRef() and decRef() on the same index readers.

[martijnvg]

unfortunately EngineSearcherFactory is only used once per refresh and not invoked each time a search is happening. The IndexSearcherWrapperService does delegate to the close method of the original Engine.Searcher do the ref counting should add up.

[jpountz]

One issue I have with this change is that the IndexSearcher class is really hard to wrap.

For instance, imagine you both need to wrap the underlying IndexReader being the searcher. Since IndexSearcher doesn't allow you to replace the wrapped reader, you need to create a new IndexSearcher around this wrapped reader. So if the original IndexSearcher was a sub-class of IndexSearcher that customized, say, how search() is implemented, then these changes will be lost. And you can't delegate to the original IndexSearcher since it is wrapping the wrong reader.

Maybe one way to work around this issue would be to wrap readers and searchers separately: first you would wpply all reader wrappers, then all searcher wrappers?

[martijnvg]

@jpountz good point. I changed the IndexSearcherWrapper interface to wrap directory readers and searcher independently. I think this resolves your concern.

[martijnvg]

After talking with @jpountz we decided that it is for now better to allow only one IndexSearcherWrapper to wrap the underlying IndexSearcher from the engine. The order in which IndexSearcher get wrapped is too lenient, which can lead to confusing issues between plugins. Also an custom filtered IndexSearcher implementation needs to be really careful what methods to override. For example if IndexSearcher#rewrite gets overwritten than also IndexSearcher#createNormalizedWeight needs to be overwritten.

[jpountz]

maybe add the set of wrappers to the message so taht it would be easier to debug

[martijnvg]

+1

[jpountz]

I think this can go in now. However can you add TODOs in the code that this stuff needs to be cleaned up and that we should make IndexSearcher more wrapping-friendly?

[jpountz]

LGTM