Lucene - Core
  1. Lucene - Core
  2. LUCENE-3636

make it possible to use searchermanager with distributed stats

    Details

    • Type: Task Task
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 3.5, 4.0-ALPHA
    • Fix Version/s: 3.6, 4.0-ALPHA
    • Component/s: None
    • Labels:
      None
    • Lucene Fields:
      New

      Description

      LUCENE-3555 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.

      1. LUCENE-3636.patch
        15 kB
        Robert Muir
      2. LUCENE-3636.patch
        18 kB
        Robert Muir
      3. LUCENE-3636.patch
        19 kB
        Michael McCandless
      4. LUCENE-3636.patch
        23 kB
        Robert Muir

        Activity

        Hide
        Robert Muir added a comment -

        Thanks for the help Shai and Mike!

        Show
        Robert Muir added a comment - Thanks for the help Shai and Mike!
        Hide
        Michael McCandless added a comment -

        +1 looks nice!

        Show
        Michael McCandless added a comment - +1 looks nice!
        Hide
        Robert Muir added a comment -

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

        Show
        Robert Muir added a comment - updated patch with the factory top-level, javadocs and VERBOSE cleanup.
        Hide
        Robert Muir added a comment -

        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.

        Show
        Robert Muir added a comment - 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.
        Hide
        Shai Erera added a comment -

        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.

        Show
        Shai Erera added a comment - 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.
        Hide
        Robert Muir added a comment -

        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

        Show
        Robert Muir added a comment - 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
        Hide
        Robert Muir added a comment -

        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).

        Show
        Robert Muir added a comment - 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).
        Hide
        Michael McCandless added a comment -

        +1 this approach is nice!

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

        Show
        Michael McCandless added a comment - +1 this approach is nice! So now it's caller's job to install a merged segment warmer on IW.
        Hide
        Michael McCandless added a comment -

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

        Show
        Michael McCandless added a comment - I got TestSM.testIntermediateClose to work – had to modify the newSearcher method to only wait if reopen was tried.
        Hide
        Robert Muir added a comment -

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

        Show
        Robert Muir added a comment - trying Shai's idea... i like it, it simplifies the APIs.
        Hide
        Robert Muir added a comment -

        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.

        Show
        Robert Muir added a comment - 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.
        Hide
        Shai Erera added a comment -

        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.

        Show
        Shai Erera added a comment - 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.
        Hide
        Robert Muir added a comment -

        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.

        Show
        Robert Muir added a comment - 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.
        Hide
        Robert Muir added a comment -

        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)

        Show
        Robert Muir added a comment - 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)

          People

          • Assignee:
            Robert Muir
            Reporter:
            Robert Muir
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development