Lucene - Core
  1. Lucene - Core
  2. LUCENE-3872

Index changes are lost if you call prepareCommit() then close()

    Details

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

      Description

      You are supposed to call commit() after calling prepareCommit(), but... if you forget, and call close() after prepareCommit() without calling commit(), then any changes done after the prepareCommit() are silently lost (including adding/deleting docs, but also any completed merges).

      Spinoff from java-user thread "lots of .cfs (compound files) in the index directory" from Tim Bogaert.

      I think to fix this, IW.close should throw an IllegalStateException if prepareCommit() was called with no matching call to commit().

      1. LUCENE-3872.patch
        4 kB
        Michael McCandless
      2. LUCENE-3872.patch
        2 kB
        Michael McCandless

        Activity

        Hide
        Michael McCandless added a comment -

        Patch w/ failing test showing how we silently lose indexed docs...

        Show
        Michael McCandless added a comment - Patch w/ failing test showing how we silently lose indexed docs...
        Hide
        Michael McCandless added a comment -

        Patch, I think it's ready.

        One test was failing to call the commit() matching its prepareCommit()... I fixed it.

        Show
        Michael McCandless added a comment - Patch, I think it's ready. One test was failing to call the commit() matching its prepareCommit()... I fixed it.
        Hide
        Robert Muir added a comment -

        I don't have a better fix, but at the same time i feel you should be able to close() at any time,
        (such as when handling exceptions in your app), since we are a real closeable here.

        Show
        Robert Muir added a comment - I don't have a better fix, but at the same time i feel you should be able to close() at any time, (such as when handling exceptions in your app), since we are a real closeable here.
        Hide
        Michael McCandless added a comment -

        Well, we could also easily allow skipping the call to commit... in this case IW.close would detect the missing call to commit, call commit, and call commit again to save any changes done after the prepareCommit and before close.

        Show
        Michael McCandless added a comment - Well, we could also easily allow skipping the call to commit... in this case IW.close would detect the missing call to commit, call commit, and call commit again to save any changes done after the prepareCommit and before close.
        Hide
        Robert Muir added a comment -

        in this case IW.close would detect the missing call to commit, call commit, and call commit again to save any changes done after the prepareCommit and before close.

        I think that would make it even more lenient and complicated and worse. I guess i feel close() should really be rollback(). But this is likely ridiculous to change.
        So on second thought I think patch is good... if someone is handling exceptional cases like this they should be thinking about using rollback() anyway,
        and they have this option still.

        I wasn't really against the patch anyway, just whining. its definitely an improvement on the current behavior, let's do it.

        Show
        Robert Muir added a comment - in this case IW.close would detect the missing call to commit, call commit, and call commit again to save any changes done after the prepareCommit and before close. I think that would make it even more lenient and complicated and worse. I guess i feel close() should really be rollback(). But this is likely ridiculous to change. So on second thought I think patch is good... if someone is handling exceptional cases like this they should be thinking about using rollback() anyway, and they have this option still. I wasn't really against the patch anyway, just whining. its definitely an improvement on the current behavior, let's do it.
        Hide
        Dawid Weiss added a comment -

        I guess i feel close() should really be rollback().

        Yeah... I think this feeling of unease is fairly common – see JDBC's Connection javadoc on close, for example: "It is strongly recommended that an application explicitly commits or rolls back an active transaction prior to calling the close method. If the close method is called and there is an active transaction, the results are implementation-defined."

        Show
        Dawid Weiss added a comment - I guess i feel close() should really be rollback(). Yeah... I think this feeling of unease is fairly common – see JDBC's Connection javadoc on close, for example: "It is strongly recommended that an application explicitly commits or rolls back an active transaction prior to calling the close method. If the close method is called and there is an active transaction, the results are implementation-defined."
        Hide
        Michael McCandless added a comment -

        Thanks Tim!

        Show
        Michael McCandless added a comment - Thanks Tim!

          People

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

            Dates

            • Created:
              Updated:
              Resolved:

              Development