Lucene - Core
  1. Lucene - Core
  2. LUCENE-2536

Rollback doesn't preserve integrity of original index

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 3.0.2
    • Fix Version/s: 2.9.4, 3.0.3, 3.1, 4.0-ALPHA
    • Component/s: core/index
    • Labels:
      None
    • Environment:

      Windows XP pro

    • Lucene Fields:
      New

      Description

      After several "updateDocuments" calls a rollback call does not return the index to the prior state.
      This seems to occur if the number of updates exceeds the RAM buffer size i.e. when some flushing of updates occurs.

      Test fails in Lucene 2.4, 2.9, 3.0.1 and 3.0.2

      JUnit to follow.

      1. LUCENE-2536.patch
        4 kB
        Michael McCandless
      2. LUCENE-2536.patch
        5 kB
        Michael McCandless
      3. TestRollback.java
        3 kB
        Shai Erera
      4. TestRollback.java
        3 kB
        Mark Harwood

        Activity

        Hide
        Mark Harwood added a comment -

        Example JUnit

        Show
        Mark Harwood added a comment - Example JUnit
        Hide
        Mark Harwood added a comment -

        Looks like on the "close" call IndexWriter is flushing a superfluous ".del" file of buffered deletes.

        Show
        Mark Harwood added a comment - Looks like on the "close" call IndexWriter is flushing a superfluous ".del" file of buffered deletes.
        Hide
        Shai Erera added a comment -

        It did not fail for me on 3.1, however did fail on 3.0.1.

        I'm attaching a version of this test, which creates far less docs (5, instead of 10,000) and uses a RAMDirectory). The test still fails, which is expected. I assume if we want to make that an official unit test, the test case which passes should go away? (or not)

        Show
        Shai Erera added a comment - It did not fail for me on 3.1, however did fail on 3.0.1. I'm attaching a version of this test, which creates far less docs (5, instead of 10,000) and uses a RAMDirectory). The test still fails, which is expected. I assume if we want to make that an official unit test, the test case which passes should go away? (or not)
        Hide
        Michael McCandless added a comment -

        Duh – in DocumentsWriter.abort, right after deletesInRAM.clear() we must add deletesFlush.clear(). So, indeed, any deletions pending (either due to .updateDoc or .deleteDocs) when segments were flushed, fail to be cleared and are then applied on close.

        How awful!

        Show
        Michael McCandless added a comment - Duh – in DocumentsWriter.abort, right after deletesInRAM.clear() we must add deletesFlush.clear(). So, indeed, any deletions pending (either due to .updateDoc or .deleteDocs) when segments were flushed, fail to be cleared and are then applied on close. How awful!
        Hide
        Michael McCandless added a comment -

        Attached patch – started from Shai's patch, tweaked it a bit, added the one line fix & CHANGES entry.

        I see the test in fact failing on 3.x.

        Show
        Michael McCandless added a comment - Attached patch – started from Shai's patch, tweaked it a bit, added the one line fix & CHANGES entry. I see the test in fact failing on 3.x.
        Hide
        Shai Erera added a comment -

        Looks good to me. I think now that the test class contains just this method, we can consolidate all the helper ones into 1? It was required before 'cause there were two test cases. And we can remove the first assert in the test, since we're not checking here that the index contains the number of added docs after it was committed - there are plenty of other tests that do that.

        Show
        Shai Erera added a comment - Looks good to me. I think now that the test class contains just this method, we can consolidate all the helper ones into 1? It was required before 'cause there were two test cases. And we can remove the first assert in the test, since we're not checking here that the index contains the number of added docs after it was committed - there are plenty of other tests that do that.
        Hide
        Michael McCandless added a comment -

        Thanks Shai – new patch.

        Show
        Michael McCandless added a comment - Thanks Shai – new patch.
        Hide
        Mark Harwood added a comment -

        Thanks for the rapid response Mike/Shai.

        I'll need to patch a couple of production systems using 2.4.0 and 2.9.1 with this.
        For the record - with this same change applied, the test passes OK on these older versions too.

        Show
        Mark Harwood added a comment - Thanks for the rapid response Mike/Shai. I'll need to patch a couple of production systems using 2.4.0 and 2.9.1 with this. For the record - with this same change applied, the test passes OK on these older versions too.
        Hide
        Michael McCandless added a comment -

        Thanks Mark – what a doozie!

        Show
        Michael McCandless added a comment - Thanks Mark – what a doozie!

          People

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

            Dates

            • Created:
              Updated:
              Resolved:

              Development