Lucene - Core
  1. Lucene - Core
  2. LUCENE-3520

If the NRT reader hasn't changed then IndexReader.openIfChanged should return null

    Details

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

      Description

      I hit a failure in TestSearcherManager (NOTE: doesn't always fail):

        ant test -Dtestcase=TestSearcherManager -Dtestmethod=testSearcherManager -Dtests.seed=459ac99a4256789c:-29b8a7f52497c3b4:145ae632ae9e1ecf
      

      It was tripping the assert inside SearcherLifetimeManager.record,
      because two different IndexSearcher instances had different IR
      instances sharing the same version. This was happening because
      IW.getReader always returns a new reader even when there are no
      changes. I think we should fix that...

      Separately I found a deadlock in
      TestSearcherManager.testIntermediateClose, if the test gets
      SerialMergeScheduler and needs to merge during the second commit.

      1. LUCENE-3520.patch
        20 kB
        Michael McCandless
      2. LUCENE-3520.patch
        11 kB
        Michael McCandless

        Activity

        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
        Hide
        Simon Willnauer added a comment -

        New patch; I was able to simplify SearcherManager since both cases (open-from-writer (= NRT case) and open-from-dir (= non-NRT case)) now call the same IR.openIfChanged(oldReader).

        yeah nice! looks good mike!

        Show
        Simon Willnauer added a comment - New patch; I was able to simplify SearcherManager since both cases (open-from-writer (= NRT case) and open-from-dir (= non-NRT case)) now call the same IR.openIfChanged(oldReader). yeah nice! looks good mike!
        Hide
        Michael McCandless added a comment -

        New patch; I was able to simplify SearcherManager since both cases (open-from-writer (= NRT case) and open-from-dir (= non-NRT case)) now call the same IR.openIfChanged(oldReader).

        Show
        Michael McCandless added a comment - New patch; I was able to simplify SearcherManager since both cases (open-from-writer (= NRT case) and open-from-dir (= non-NRT case)) now call the same IR.openIfChanged(oldReader).
        Hide
        Michael McCandless added a comment -

        hmm, however the signature of openIfChanged(IR, boolean) actually referes to openIfChanged(IndexReader oldReader, boolean readonly) which seems confusing when you pass applyDeletes to it, no?

        Ugh, you're right!

        In fact we don't need to pass applyDeletes either; this too is inherited from the prior reader. So I'll just reduce it to IR.openIfChanged(oldReader). Hmm then we can simplify SearcherManager some. I'll work out a new patch.

        Show
        Michael McCandless added a comment - hmm, however the signature of openIfChanged(IR, boolean) actually referes to openIfChanged(IndexReader oldReader, boolean readonly) which seems confusing when you pass applyDeletes to it, no? Ugh, you're right! In fact we don't need to pass applyDeletes either; this too is inherited from the prior reader. So I'll just reduce it to IR.openIfChanged(oldReader). Hmm then we can simplify SearcherManager some. I'll work out a new patch.
        Hide
        Michael McCandless added a comment -

        I noticed "SearchManager" in one of the traces. Maybe this issue fixes?

        Indeed I think this fixes it; I just committed that test-only fix.

        Show
        Michael McCandless added a comment - I noticed "SearchManager" in one of the traces. Maybe this issue fixes? Indeed I think this fixes it; I just committed that test-only fix.
        Hide
        Simon Willnauer added a comment -

        It should still be opening an NRT reader: if you have an NRT reader (which we do here) and pass that to IR.openIfChanged, you'll always get back a new NRT reader (this is the contract of IR.openIfChanged).

        hmm, however the signature of openIfChanged(IR, boolean) actually referes to openIfChanged(IndexReader oldReader, boolean readonly) which seems confusing when you pass applyDeletes to it, no?

        Show
        Simon Willnauer added a comment - It should still be opening an NRT reader: if you have an NRT reader (which we do here) and pass that to IR.openIfChanged, you'll always get back a new NRT reader (this is the contract of IR.openIfChanged). hmm, however the signature of openIfChanged(IR, boolean) actually referes to openIfChanged(IndexReader oldReader, boolean readonly) which seems confusing when you pass applyDeletes to it, no?
        Hide
        Yonik Seeley added a comment -

        Steve just took a stack trace (and aborted the build) of a test run started yesterday.
        I noticed "SearchManager" in one of the traces. Maybe this issue fixes?

        https://builds.apache.org/job/Lucene-Solr-tests-only-trunk/10840/console

        Show
        Yonik Seeley added a comment - Steve just took a stack trace (and aborted the build) of a test run started yesterday. I noticed "SearchManager" in one of the traces. Maybe this issue fixes? https://builds.apache.org/job/Lucene-Solr-tests-only-trunk/10840/console
        Hide
        Michael McCandless added a comment -

        Thanks Simon.

        It should still be opening an NRT reader: if you have an NRT reader (which we do here) and pass that to IR.openIfChanged, you'll always get back a new NRT reader (this is the contract of IR.openIfChanged).

        So I believe passing writer here wasn't necessary?

        Show
        Michael McCandless added a comment - Thanks Simon. It should still be opening an NRT reader: if you have an NRT reader (which we do here) and pass that to IR.openIfChanged, you'll always get back a new NRT reader (this is the contract of IR.openIfChanged). So I believe passing writer here wasn't necessary?
        Hide
        Simon Willnauer added a comment -

        mike, I just had a quick look but is this intentionally?

        -      return IndexReader.openIfChanged(oldReader, writer, applyDeletes);
        +      return IndexReader.openIfChanged(oldReader, applyDeletes);
        

        seems like you are not opening a NRT reader there anymore?

        Show
        Simon Willnauer added a comment - mike, I just had a quick look but is this intentionally? - return IndexReader.openIfChanged(oldReader, writer, applyDeletes); + return IndexReader.openIfChanged(oldReader, applyDeletes); seems like you are not opening a NRT reader there anymore?
        Hide
        Michael McCandless added a comment -

        Patch.

        Show
        Michael McCandless added a comment - Patch.

          People

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

            Dates

            • Created:
              Updated:
              Resolved:

              Development