Lucene - Core
  1. Lucene - Core
  2. LUCENE-3116

pendingCommit in IndexWriter is not thoroughly tested

    Details

    • Type: Test Test
    • Status: Open
    • Priority: Major Major
    • Resolution: Unresolved
    • Affects Version/s: 3.2, 4.0-ALPHA
    • Fix Version/s: 4.9, 5.0
    • Component/s: core/index
    • Labels:
      None
    • Lucene Fields:
      New

      Description

      When working on LUCENE-3084, I had a copy-paste error in my patch (see revision 1124307 and corrected in 1124316), I replaced pendingCommit by segmentInfos in IndexWriter, corrected by the following patch:

      --- lucene/dev/trunk/lucene/src/java/org/apache/lucene/index/IndexWriter.java (original)
      +++ lucene/dev/trunk/lucene/src/java/org/apache/lucene/index/IndexWriter.java Wed May 18 16:16:29 2011
      @@ -2552,7 +2552,7 @@ public class IndexWriter implements Clos
               lastCommitChangeCount = pendingCommitChangeCount;
               segmentInfos.updateGeneration(pendingCommit);
               segmentInfos.setUserData(pendingCommit.getUserData());
      -        rollbackSegments = segmentInfos.createBackupSegmentInfos(true);
      +        rollbackSegments = pendingCommit.createBackupSegmentInfos(true);
               deleter.checkpoint(pendingCommit, true);
             } finally {
               // Matches the incRef done in startCommit:
      

      This did not cause any test failure.

      On IRC, Mike said:

      [19:21] mikemccand: ThetaPh1: hmm
      [19:21] mikemccand: well
      [19:22] mikemccand: pendingCommit and sis only differ while commit() is running
      [19:22] mikemccand: ie if a thread starts commit
      [19:22] mikemccand: but fsync is taking a long time
      [19:22] mikemccand: and another thread makes a change to sis
      [19:22] ThetaPh1: ok so hard to find that bug
      [19:22] mikemccand: we need our mock dir wrapper to sometimes take a long time syncing....

      Maybe we need such a test, I feel bad when such stupid changes don't make any test fail.

        Activity

        Hide
        Uwe Schindler added a comment -

        Move issue to Lucene 4.9.

        Show
        Uwe Schindler added a comment - Move issue to Lucene 4.9.
        Hide
        Steve Rowe added a comment -

        Bulk move 4.4 issues to 4.5 and 5.0

        Show
        Steve Rowe added a comment - Bulk move 4.4 issues to 4.5 and 5.0
        Hide
        Michael McCandless added a comment -

        I think we can push to 4.0...

        Show
        Michael McCandless added a comment - I think we can push to 4.0...
        Hide
        Robert Muir added a comment -

        its easy to add the sleep, but we dont even have good multithreaded tests with rollback() [except testing how exceptions are handled and not really asserting anything?]

        Can we push this out to 4.0?

        Show
        Robert Muir added a comment - its easy to add the sleep, but we dont even have good multithreaded tests with rollback() [except testing how exceptions are handled and not really asserting anything?] Can we push this out to 4.0?
        Hide
        Robert Muir added a comment -

        bulk move 3.2 -> 3.3

        Show
        Robert Muir added a comment - bulk move 3.2 -> 3.3
        Hide
        Michael McCandless added a comment -

        It's great you caught this on backport Uwe! And, yes, spooky no tests failed...

        It'll be challenging to have a test catch this. Fixing MockDirWrapper to sometimes take "unusually" long time to do the fsync is a great start. What this change would have caused is .rollback() would roll back to a wrong copy of the sis, ie not a commit point but rather a commit point plus some additional flushes.

        Show
        Michael McCandless added a comment - It's great you caught this on backport Uwe! And, yes, spooky no tests failed... It'll be challenging to have a test catch this. Fixing MockDirWrapper to sometimes take "unusually" long time to do the fsync is a great start. What this change would have caused is .rollback() would roll back to a wrong copy of the sis, ie not a commit point but rather a commit point plus some additional flushes.

          People

          • Assignee:
            Unassigned
            Reporter:
            Uwe Schindler
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:

              Development