Lucene - Core
  1. Lucene - Core
  2. LUCENE-3445

Add SearcherManager, to manage IndexSearcher usage across threads and reopens

    Details

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

      Description

      This is a simple helper class I wrote for Lucene in Action 2nd ed.
      I'd like to commit under Lucene (contrib/misc).

      It simplifies using & reopening an IndexSearcher across multiple
      threads, by using IndexReader's ref counts to know when it's safe
      to close the reader.

      In the process I also factored out a test base class for tests that
      want to make lots of simultaneous indexing and searching threads, and
      fixed TestNRTThreads (core), TestNRTManager (contrib/misc) and the new
      TestSearcherManager (contrib/misc) to use this base class.

      1. LUCENE-3445.patch
        94 kB
        Michael McCandless
      2. LUCENE-3445.patch
        105 kB
        Michael McCandless
      3. LUCENE-3445.patch
        7 kB
        Simon Willnauer

        Activity

        Hide
        Shai Erera added a comment -

        This is great Mike !

        I reviewed SearcherManager and have a comment about the TODO on whether or not to call warm in the ctor. If an extending class relies on some internal members to be initialized before warm() can safely be called, then this will lead to exceptions. I think that warm() should not be called in the ctor, or at least add a ctor which accepts a boolean doWarm, while the other ctors call it with 'true'.

        Calling warm() in the ctor is useful if one wants to warm the IndexSearcher instance before SearcherManager is ready for use. So perhaps an additional ctor with the boolean gives the most flexibility.

        Also, I remember there was a ctor which took IndexWriter, to allow for an NRT-SearcherManager. What happened to it?

        Show
        Shai Erera added a comment - This is great Mike ! I reviewed SearcherManager and have a comment about the TODO on whether or not to call warm in the ctor. If an extending class relies on some internal members to be initialized before warm() can safely be called, then this will lead to exceptions. I think that warm() should not be called in the ctor, or at least add a ctor which accepts a boolean doWarm, while the other ctors call it with 'true'. Calling warm() in the ctor is useful if one wants to warm the IndexSearcher instance before SearcherManager is ready for use. So perhaps an additional ctor with the boolean gives the most flexibility. Also, I remember there was a ctor which took IndexWriter, to allow for an NRT-SearcherManager. What happened to it?
        Hide
        Simon Willnauer added a comment -

        Mike, would it be possible to merge the NRT and SearcherManager into one class / manager or that maybe both implement the same interface?

        Show
        Simon Willnauer added a comment - Mike, would it be possible to merge the NRT and SearcherManager into one class / manager or that maybe both implement the same interface?
        Hide
        Michael McCandless added a comment -

        Maybe we just shouldn't ever call warm from the ctor? Caller can easily call warm themselves... we can just jdoc this.

        Also, I remember there was a ctor which took IndexWriter, to allow for an NRT-SearcherManager. What happened to it?

        Right, I removed it because we have NRTManager now.

        Mike, would it be possible to merge the NRT and SearcherManager into one class / manager or that maybe both implement the same interface?

        Right, they used to be merged.... we could consider merging them again?

        NRTManager is more feature-full, though, because it offers add/updateDocument(s) APIs that invoke the writer but return a [long] gen, which you can then turnaround and pass to .get() to ensure the returned searcher includes those changes. SearcherManager doesn't have notion... unless we somehow tie in the IndexReader.getVersion() somehow? I'm torn... they seem different enough that maybe they should remain separate. Plus, users usually know quite strongly if they use NRT or not and could pick the right manager accordingly...

        Show
        Michael McCandless added a comment - Maybe we just shouldn't ever call warm from the ctor? Caller can easily call warm themselves... we can just jdoc this. Also, I remember there was a ctor which took IndexWriter, to allow for an NRT-SearcherManager. What happened to it? Right, I removed it because we have NRTManager now. Mike, would it be possible to merge the NRT and SearcherManager into one class / manager or that maybe both implement the same interface? Right, they used to be merged.... we could consider merging them again? NRTManager is more feature-full, though, because it offers add/updateDocument(s) APIs that invoke the writer but return a [long] gen, which you can then turnaround and pass to .get() to ensure the returned searcher includes those changes. SearcherManager doesn't have notion... unless we somehow tie in the IndexReader.getVersion() somehow? I'm torn... they seem different enough that maybe they should remain separate. Plus, users usually know quite strongly if they use NRT or not and could pick the right manager accordingly...
        Hide
        Shai Erera added a comment -

        Caller can easily call warm themselves

        warm() is protected. If you don't want to extend SearcherManager, how would you ensure it's warmed-up after creation?

        I removed it because we have NRTManager now

        Ok, I wasn't aware of that.

        Show
        Shai Erera added a comment - Caller can easily call warm themselves warm() is protected. If you don't want to extend SearcherManager, how would you ensure it's warmed-up after creation? I removed it because we have NRTManager now Ok, I wasn't aware of that.
        Hide
        Michael McCandless added a comment -

        warm() is protected. If you don't want to extend SearcherManager, how would you ensure it's warmed-up after creation?

        Well, warm() is a no-op if you don't extend SearcherManager, so, if you extend, you'd impl warm, and then in your ctor you should call warm on the initial searcher if need be. Definitely trappy so we should jdoc that subclass's ctor must handle warming initial searcher....

        Show
        Michael McCandless added a comment - warm() is protected. If you don't want to extend SearcherManager, how would you ensure it's warmed-up after creation? Well, warm() is a no-op if you don't extend SearcherManager, so, if you extend, you'd impl warm, and then in your ctor you should call warm on the initial searcher if need be. Definitely trappy so we should jdoc that subclass's ctor must handle warming initial searcher....
        Hide
        Simon Willnauer added a comment -

        Well, warm() is a no-op if you don't extend SearcherManager, so, if you extend, you'd impl warm, and then in your ctor you should call warm on the initial searcher if need be. Definitely trappy so we should jdoc that subclass's ctor must handle warming initial searcher....

        it seems pretty trappy to have this in the ctor though. Since this is a noop by default and it requires subclassing to do something with it it might be worth introducing a SearchWarmer interface and simply pass this interface into the ctor ie. have two ctors where by default we pass a noop warmer?

        Show
        Simon Willnauer added a comment - Well, warm() is a no-op if you don't extend SearcherManager, so, if you extend, you'd impl warm, and then in your ctor you should call warm on the initial searcher if need be. Definitely trappy so we should jdoc that subclass's ctor must handle warming initial searcher.... it seems pretty trappy to have this in the ctor though. Since this is a noop by default and it requires subclassing to do something with it it might be worth introducing a SearchWarmer interface and simply pass this interface into the ctor ie. have two ctors where by default we pass a noop warmer?
        Hide
        Michael McCandless added a comment -

        I like that approach; we can then use that same interface for passing to NRTManager. I'll do that.

        Show
        Michael McCandless added a comment - I like that approach; we can then use that same interface for passing to NRTManager. I'll do that.
        Hide
        Michael McCandless added a comment -

        New patch, adds a SearcherWarmer interface (also in contrib/misc), and switches Searcher/NRTManager to accepting a warmer to their ctors instead of subclassing.

        I think it's ready for commit!

        Show
        Michael McCandless added a comment - New patch, adds a SearcherWarmer interface (also in contrib/misc), and switches Searcher/NRTManager to accepting a warmer to their ctors instead of subclassing. I think it's ready for commit!
        Hide
        Greg Steffensen added a comment -

        What are the different use cases for this and NRTManager?

        Show
        Greg Steffensen added a comment - What are the different use cases for this and NRTManager?
        Hide
        Simon Willnauer added a comment -

        I have a couple of smallish improvements to SearcherManager making the hottest method in SM non-blocking. I added a tryIncRef to index reader that makes is easier to work with IR refcounts in a non-blocking fashion. I also renamed SM#get to SM#acquire which seems to be more clear about what happens here (also kind of stronger than get - so you'd better release it).

        Show
        Simon Willnauer added a comment - I have a couple of smallish improvements to SearcherManager making the hottest method in SM non-blocking. I added a tryIncRef to index reader that makes is easier to work with IR refcounts in a non-blocking fashion. I also renamed SM#get to SM#acquire which seems to be more clear about what happens here (also kind of stronger than get - so you'd better release it).
        Hide
        Michael McCandless added a comment -

        +1 – changes look great! Nice to have .acquire be lock free.

        Show
        Michael McCandless added a comment - +1 – changes look great! Nice to have .acquire be lock free.
        Hide
        Simon Willnauer added a comment -

        Committed the last patch in revision 1176772. Thanks for review mike.

        Show
        Simon Willnauer added a comment - Committed the last patch in revision 1176772. Thanks for review mike.
        Hide
        Simon Willnauer added a comment -

        backported to 3.x in rev. 1176933

        Show
        Simon Willnauer added a comment - backported to 3.x in rev. 1176933
        Hide
        Michael McCandless added a comment -

        Thanks Simon!

        Show
        Michael McCandless added a comment - Thanks Simon!
        Hide
        Uwe Schindler added a comment -

        Bulk close after release of 3.5

        Show
        Uwe Schindler added a comment - Bulk close after release of 3.5

          People

          • Assignee:
            Michael McCandless
            Reporter:
            Michael McCandless
          • Votes:
            0 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development