Details

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

      Description

      NRTManager is hairy now, because the applyDeletes is separately passed
      to ctor, passed to maybeReopen, passed to getSearcherManager, etc.

      I think, instead, you should pass it only to the ctor, and if you have
      some cases needing deletes and others not then you can make two
      NRTManagers. This should be no less efficient than we have today,
      just simpler.

      I think it will also enable NRTManager to subclass ThingyManager
      (LUCENE-3761).

      1. LUCENE-3769.patch
        36 kB
        Michael McCandless
      2. LUCENE-3769.patch
        36 kB
        Michael McCandless

        Activity

        Hide
        Michael McCandless added a comment -

        Patch; this is a great simplification of the NRTMgr API, and I think sets it up for ThingyManager.

        But: I had to introduce TrackingIndexWriter, which just wraps an IndexWriter and tracks the changes made through it (as a generation). Previously NRTManager did this all itself, but now you may need to share this tracking across 2 NRTManagers so I had to pull it out...

        Show
        Michael McCandless added a comment - Patch; this is a great simplification of the NRTMgr API, and I think sets it up for ThingyManager. But: I had to introduce TrackingIndexWriter, which just wraps an IndexWriter and tracks the changes made through it (as a generation). Previously NRTManager did this all itself, but now you may need to share this tracking across 2 NRTManagers so I had to pull it out...
        Hide
        Simon Willnauer added a comment -

        mike, I like the simplification. I think we talked about this last time we touched this code already. I think this is the most common use-case... Yet, I don't think NRTManager should expose IndexSearcher directly. Think of an app that either uses NRT or commit / reopen ie. uses a NRTManager and a PlainOldIndexManager. From a user perspective SearcherManager is the main interface to search the index no matter how you commit / flush / NRT etc. I'd rather keep it exposing the SM instead.

        about TrackingIndexWriter - I think we should fix IW to use seq ids eventually but I think for this purpose its fine.

        Show
        Simon Willnauer added a comment - mike, I like the simplification. I think we talked about this last time we touched this code already. I think this is the most common use-case... Yet, I don't think NRTManager should expose IndexSearcher directly. Think of an app that either uses NRT or commit / reopen ie. uses a NRTManager and a PlainOldIndexManager. From a user perspective SearcherManager is the main interface to search the index no matter how you commit / flush / NRT etc. I'd rather keep it exposing the SM instead. about TrackingIndexWriter - I think we should fix IW to use seq ids eventually but I think for this purpose its fine.
        Hide
        Michael McCandless added a comment -

        Yet, I don't think NRTManager should expose IndexSearcher directly.

        Hmm, but the example you raised will be addressed by LUCENE-3761, right? Ie at that point, both SM and NRTM are ThingyManager<IndexSearcher>?

        I don't like the extra indirection (you get an SM from NRTM) because now you have two linked instances. For example, it opens up the risk that the app will (incorrectly) call SM.maybeReopen instead of NRTM.maybeReopen. I think having the one manager to work with is less dangerous...

        Show
        Michael McCandless added a comment - Yet, I don't think NRTManager should expose IndexSearcher directly. Hmm, but the example you raised will be addressed by LUCENE-3761 , right? Ie at that point, both SM and NRTM are ThingyManager<IndexSearcher>? I don't like the extra indirection (you get an SM from NRTM) because now you have two linked instances. For example, it opens up the risk that the app will (incorrectly) call SM.maybeReopen instead of NRTM.maybeReopen. I think having the one manager to work with is less dangerous...
        Hide
        Simon Willnauer added a comment -

        Hmm, but the example you raised will be addressed by LUCENE-3761, right? Ie at that point, both SM and NRTM are ThingyManager<IndexSearcher>?

        I think NRTM should delegate rather than subclass. It serves two different purposes and we shouldn't try to couple tight if not needed.

        Show
        Simon Willnauer added a comment - Hmm, but the example you raised will be addressed by LUCENE-3761 , right? Ie at that point, both SM and NRTM are ThingyManager<IndexSearcher>? I think NRTM should delegate rather than subclass. It serves two different purposes and we shouldn't try to couple tight if not needed.
        Hide
        Michael McCandless added a comment -

        New patch, exposing the private SearcherManager via
        getSearcherManager.

        I really don't like the trappiness, ie that an app will mistakenly
        call SM.maybeReopen instead of NRTM.maybeReopen, thus silently causing
        threads to hang.... But I think once LUCENE-3761 is in we can fix
        this... I'll open a separate issue to address this....

        I think this is ready!

        Show
        Michael McCandless added a comment - New patch, exposing the private SearcherManager via getSearcherManager. I really don't like the trappiness, ie that an app will mistakenly call SM.maybeReopen instead of NRTM.maybeReopen, thus silently causing threads to hang.... But I think once LUCENE-3761 is in we can fix this... I'll open a separate issue to address this.... I think this is ready!
        Hide
        Simon Willnauer added a comment -

        I think this is ready!

        +1 I think this gets further improved once LUCENE-3761 is in. thanks mike

        Show
        Simon Willnauer added a comment - I think this is ready! +1 I think this gets further improved once LUCENE-3761 is in. thanks mike
        Hide
        Simon Willnauer added a comment -

        one question, who is "MJB" in the changes.txt?

        Show
        Simon Willnauer added a comment - one question, who is "MJB" in the changes.txt?
        Hide
        Michael McCandless added a comment -

        one question, who is "MJB" in the changes.txt?

        Oh, it's someone who found the applyDeletes logic confusing... s/he was asking about it in the comments near the end, here:

        http://blog.mikemccandless.com/2011/06/lucenes-near-real-time-search-is-fast.html

        Show
        Michael McCandless added a comment - one question, who is "MJB" in the changes.txt? Oh, it's someone who found the applyDeletes logic confusing... s/he was asking about it in the comments near the end, here: http://blog.mikemccandless.com/2011/06/lucenes-near-real-time-search-is-fast.html
        Hide
        Michael McCandless added a comment -

        I'll open follow-on issue for the nasty trap...

        Show
        Michael McCandless added a comment - I'll open follow-on issue for the nasty trap...

          People

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

            Dates

            • Created:
              Updated:
              Resolved:

              Development