Lucene - Core
  1. Lucene - Core
  2. LUCENE-5981

CheckIndex modifies index without write.lock

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 5.0, 6.0
    • Component/s: None
    • Labels:
      None
    • Lucene Fields:
      New

      Description

      Instead it asks you nicely to "not do that".

      Due to the way this is implemented, if you choose to drop corrupt segments, it should obtain the lock before actually doing any reads too, or it might lose more than you think or do other strange stuff.

      1. LUCENE-5981.patch
        17 kB
        Robert Muir
      2. LUCENE-5981.patch
        14 kB
        Robert Muir

        Activity

        Hide
        Robert Muir added a comment -

        Here's one patch:

        • makes CheckIndex take boolean readOnly (disallows modifications if this is true, otherwise obtains write.lock).
        • makes CheckIndex Closeable to release any lock.
        • fixed CheckIndex main() to actually call close() on the Directory always.
        • and moves main() logic to doMain() so its easier to test without it shutting down the JVM.
        • adds a simple test.

        Its a little complicated (yeah the stupid readOnly param) because I thought it was overkill to require it to obtain write.lock in the "typical" case where you are not going to let it drop segments. But when you are, its important to make sure nothing is changing stuff out from under you.

        Show
        Robert Muir added a comment - Here's one patch: makes CheckIndex take boolean readOnly (disallows modifications if this is true, otherwise obtains write.lock). makes CheckIndex Closeable to release any lock. fixed CheckIndex main() to actually call close() on the Directory always. and moves main() logic to doMain() so its easier to test without it shutting down the JVM. adds a simple test. Its a little complicated (yeah the stupid readOnly param) because I thought it was overkill to require it to obtain write.lock in the "typical" case where you are not going to let it drop segments. But when you are, its important to make sure nothing is changing stuff out from under you.
        Hide
        Michael McCandless added a comment -

        I think we should obtain the lock even if you will not exorcise?

        Otherwise you can get false corruption reports?

        Show
        Michael McCandless added a comment - I think we should obtain the lock even if you will not exorcise? Otherwise you can get false corruption reports?
        Hide
        Robert Muir added a comment -

        How is it any worse than a normal reader with lockless commits?

        Show
        Robert Muir added a comment - How is it any worse than a normal reader with lockless commits?
        Hide
        Robert Muir added a comment -

        OK i see, this guy doesn't actually open readers in the loop. I don't think it should either. I agree, lets just obtain a lock.

        Show
        Robert Muir added a comment - OK i see, this guy doesn't actually open readers in the loop. I don't think it should either. I agree, lets just obtain a lock.
        Hide
        Robert Muir added a comment -

        My issue with that approach is that it will make a lot of tests harder to debug. E.g. exception handling tests are calling this directly after getting exception (without closing writer and potentially getting more exceptions).

        Show
        Robert Muir added a comment - My issue with that approach is that it will make a lot of tests harder to debug. E.g. exception handling tests are calling this directly after getting exception (without closing writer and potentially getting more exceptions).
        Hide
        Robert Muir added a comment -

        Updated patch as a simple step.

        Basically our tests still continue to cheat.

        Show
        Robert Muir added a comment - Updated patch as a simple step. Basically our tests still continue to cheat.
        Hide
        Michael McCandless added a comment -

        +1, thanks Rob!

        Show
        Michael McCandless added a comment - +1, thanks Rob!
        Hide
        ASF subversion and git services added a comment -

        Commit 1628675 from Robert Muir in branch 'dev/trunk'
        [ https://svn.apache.org/r1628675 ]

        LUCENE-5981: fix CheckIndex to obtain write.lock

        Show
        ASF subversion and git services added a comment - Commit 1628675 from Robert Muir in branch 'dev/trunk' [ https://svn.apache.org/r1628675 ] LUCENE-5981 : fix CheckIndex to obtain write.lock
        Hide
        ASF subversion and git services added a comment -

        Commit 1628678 from Robert Muir in branch 'dev/branches/branch_5x'
        [ https://svn.apache.org/r1628678 ]

        LUCENE-5981: fix CheckIndex to obtain write.lock

        Show
        ASF subversion and git services added a comment - Commit 1628678 from Robert Muir in branch 'dev/branches/branch_5x' [ https://svn.apache.org/r1628678 ] LUCENE-5981 : fix CheckIndex to obtain write.lock
        Hide
        Anshum Gupta added a comment -

        Bulk close after 5.0 release.

        Show
        Anshum Gupta added a comment - Bulk close after 5.0 release.

          People

          • Assignee:
            Unassigned
            Reporter:
            Robert Muir
          • Votes:
            0 Vote for this issue
            Watchers:
            4 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development