apache / lucene

Apache Lucene open-source search software
https://lucene.apache.org/
Apache License 2.0
2.63k stars 1.02k forks source link

make it possible to use searchermanager with distributed stats [LUCENE-3636] #4710

Closed asfimport closed 12 years ago

asfimport commented 12 years ago

4629 added explicit stats methods to indexsearcher, but you must

subclass to override this (e.g. populate with distributed stats).

Its also impossible to then do this with SearcherManager.

One idea is make this a factory method (or similar) on IndexSearcher instead, so you don't need to subclass it to override.

Then you can initialize this in a SearcherWarmer, except there is currently a lot of hair in what this warming should be. This is a prime example where Searcher has different meaning from Reader, we should clean this up.

Otherwise, lets make NRT/SearcherManager subclassable in such a way that you can return a custom indexsearcher.


Migrated from LUCENE-3636 by Robert Muir (@rmuir), resolved Dec 11 2011 Attachments: LUCENE-3636.patch (versions: 4)

asfimport commented 12 years ago

Robert Muir (@rmuir) (migrated from JIRA)

attached is a patch, but I'm not sure I like the API on indexsearcher, i think it looks confusing.

I disabled testIntermediateClose in TestSearcherManager, the concurrency is un-understandable and the test seems to rely upon how we warm today (which I changed in the patch)

asfimport commented 12 years ago

Robert Muir (@rmuir) (migrated from JIRA)

I want to try another patch where we make SearcherManager non-final, and have newSearcher() or similar. this way we can compare, doesn't seem like these classes really need to be final.

asfimport commented 12 years ago

Shai Erera (@shaie) (migrated from JIRA)

We can make these classes sub-classable, by making most of their methods final, except for newSearcher() and the like.

Another option is to pass an IndexSearcherFactory which returns an IS given an IR.

asfimport commented 12 years ago

Robert Muir (@rmuir) (migrated from JIRA)

Another option is to pass an IndexSearcherFactory which returns an IS given an IR.

That might be the only way to go... so we can safely handle the first searcher in the ctor this way too.

asfimport commented 12 years ago

Robert Muir (@rmuir) (migrated from JIRA)

trying Shai's idea... i like it, it simplifies the APIs.

asfimport commented 12 years ago

Michael McCandless (@mikemccand) (migrated from JIRA)

I got TestSM.testIntermediateClose to work – had to modify the newSearcher method to only wait if reopen was tried.

asfimport commented 12 years ago

Michael McCandless (@mikemccand) (migrated from JIRA)

+1 this approach is nice!

So now it's caller's job to install a merged segment warmer on IW.

asfimport commented 12 years ago

Robert Muir (@rmuir) (migrated from JIRA)

Thanks for fixing the test!

So now it's caller's job to install a merged segment warmer on IW.

Yes, i should fix the javadocs to recommend this too. In general i think the javadocs need cleanup (I didnt test that there were no warnings yet).

asfimport commented 12 years ago

Robert Muir (@rmuir) (migrated from JIRA)

Setting 3.5 and 4.0... really we should be able to backport this to 3.6 too, we just have to make decisions about whether we should break or add lots of deprecated ctors :)

asfimport commented 12 years ago

Shai Erera (@shaie) (migrated from JIRA)

Phew, that does clean up a lot of code ! :)

Can we make SearcherFactory its own class? Since SearcherManager and NRTManager both depend on it (and I can already see how I will depend on it as well !), I think it can be its own top-level class.

really we should be able to backport this to 3.6 too, we just have to make decisions about whether we should break or add lots of deprecated ctors

+1 for breaking. This API is new, experimental. We ought to be able to break it, and it's not like a very hard break.

asfimport commented 12 years ago

Robert Muir (@rmuir) (migrated from JIRA)

Can we make SearcherFactory its own class? Since SearcherManager and NRTManager both depend on it (and I can already see how I will depend on it as well !), I think it can be its own top-level class.

Yes, we don't have to put it inside SearcherManager.

asfimport commented 12 years ago

Robert Muir (@rmuir) (migrated from JIRA)

updated patch with the factory top-level, javadocs and VERBOSE cleanup.

asfimport commented 12 years ago

Michael McCandless (@mikemccand) (migrated from JIRA)

+1 looks nice!

asfimport commented 12 years ago

Robert Muir (@rmuir) (migrated from JIRA)

Thanks for the help Shai and Mike!