Lucene - Core
  1. Lucene - Core
  2. LUCENE-5871

Simplify or remove use of Version in IndexWriterConfig

    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

      IndexWriter currently uses Version from IndexWriterConfig to determine the semantics of close(). This is a trapdoor for users, as they often default to just sending Version.LUCENE_CURRENT since they don't understand what it will be used for. Instead, we should make the semantics of close a direction option in IWC.

      1. LUCENE-5871.iwclose.4x.patch
        20 kB
        Michael McCandless
      2. LUCENE-5871.iwclose.trunk.patch
        8 kB
        Michael McCandless
      3. LUCENE-5871.patch
        820 kB
        Ryan Ernst
      4. LUCENE-5871.patch
        819 kB
        Ryan Ernst
      5. LUCENE-5871.patch
        709 kB
        Michael McCandless
      6. LUCENE-5871.patch
        696 kB
        Ryan Ernst
      7. LUCENE-5871.patch
        181 kB
        Ryan Ernst

        Activity

        Hide
        Shai Erera added a comment -

        Version is currently used to decide whether to throw an exception in case IW has uncommitted or in-flight merges when you call close(), or silently discard them. I think that users who go to 5.0 will need to read the CHANGES carefully anyway, and adapt to the new way (close() only closes, not commit). I think we added it as convenience for users who upgrade and don't read CHANGES. But as you say, those users probably also pass Version.CURRENT, which means they silently lose those changes, so I now wonder if we should have this "convenience" in the code at all. And if we do decide to keep it, I suggest deprecating it in trunk, so it can be removed in e.g. 6.0.

        Show
        Shai Erera added a comment - Version is currently used to decide whether to throw an exception in case IW has uncommitted or in-flight merges when you call close(), or silently discard them. I think that users who go to 5.0 will need to read the CHANGES carefully anyway, and adapt to the new way (close() only closes, not commit). I think we added it as convenience for users who upgrade and don't read CHANGES. But as you say, those users probably also pass Version.CURRENT, which means they silently lose those changes, so I now wonder if we should have this "convenience" in the code at all. And if we do decide to keep it, I suggest deprecating it in trunk, so it can be removed in e.g. 6.0.
        Hide
        Ryan Ernst added a comment -

        I don't think we should deprecate something that is only in trunk. We should simply change trunk, as 5.0 is not released. I am suggesting replacing the logic depending on version with an explicit option in IWC. It will default to current behavior (as if we saw LUCENE_5_0 currently), but can be set to use the semantics of 4x (commit on close).

        Show
        Ryan Ernst added a comment - I don't think we should deprecate something that is only in trunk. We should simply change trunk, as 5.0 is not released. I am suggesting replacing the logic depending on version with an explicit option in IWC. It will default to current behavior (as if we saw LUCENE_5_0 currently), but can be set to use the semantics of 4x (commit on close).
        Hide
        Ryan Ernst added a comment -

        Here is a patch with the plan as I described above. All tests pass.

        Show
        Ryan Ernst added a comment - Here is a patch with the plan as I described above. All tests pass.
        Hide
        Shai Erera added a comment -

        I don't think this patch is correct. IndexWriter uses Version not in order to commitOnClose, but to throw an exception if you have uncommitted changes. You see, IndexWriter.close() no longer commits in trunk, it always discards uncommitted changes. The only difference is whether they are discarded silently or loudly.

        That's why I said that this code should be either removed or deprecated in 5.0 and removed in 6.0. The only reason this code exists is that we want people who migrate from 4.x to 5.0 will not silently break their app if they didn't call commit() prior to close() (or called shutdown()). But I'm not sure these users will know to pass Version < 5.0 in order to set that behavior, unless they already do.

        Anyway, if we want to make this patch valid, we should name the setter something like setFailIfUncommittedChangesOnClose ...

        Show
        Shai Erera added a comment - I don't think this patch is correct. IndexWriter uses Version not in order to commitOnClose, but to throw an exception if you have uncommitted changes. You see, IndexWriter.close() no longer commits in trunk, it always discards uncommitted changes. The only difference is whether they are discarded silently or loudly. That's why I said that this code should be either removed or deprecated in 5.0 and removed in 6.0. The only reason this code exists is that we want people who migrate from 4.x to 5.0 will not silently break their app if they didn't call commit() prior to close() (or called shutdown()). But I'm not sure these users will know to pass Version < 5.0 in order to set that behavior, unless they already do. Anyway, if we want to make this patch valid, we should name the setter something like setFailIfUncommittedChangesOnClose ...
        Hide
        Michael McCandless added a comment -

        Actually, I rather like the change in semantics (from the current (on trunk) weird "throw exception or not in close telling you if you lost changes", to "commit on close").

        And ... the default makes me nervous: can we go back to 4.x's default? Ie, by default close will wait for merges / commit, but you can disable this to make close == rollback by calling IWC.setCommitOnClose(false).

        If we do this, I think we need to fix close to call shutdown(true) when commitOnClose is true, else rollback, and it no longer throws any exceptions about changes being lost.

        Show
        Michael McCandless added a comment - Actually, I rather like the change in semantics (from the current (on trunk) weird "throw exception or not in close telling you if you lost changes", to "commit on close"). And ... the default makes me nervous: can we go back to 4.x's default? Ie, by default close will wait for merges / commit, but you can disable this to make close == rollback by calling IWC.setCommitOnClose(false). If we do this, I think we need to fix close to call shutdown(true) when commitOnClose is true, else rollback, and it no longer throws any exceptions about changes being lost.
        Hide
        Shai Erera added a comment -

        We could change the semantics, but I pointed out that this patch doesn't in fact do that, except maybe it intends to do it .

        But won't changing the semantics to commit + waitMerges by default sort of rollback the change we did on LUCENE-4246? And if we do that, why do we need shutdown()? I mean, if I can set IWC.commitOnClose(true), should I also call shutdown?

        Show
        Shai Erera added a comment - We could change the semantics, but I pointed out that this patch doesn't in fact do that, except maybe it intends to do it . But won't changing the semantics to commit + waitMerges by default sort of rollback the change we did on LUCENE-4246 ? And if we do that, why do we need shutdown()? I mean, if I can set IWC.commitOnClose(true), should I also call shutdown?
        Hide
        Shai Erera added a comment -

        I guess what I'm trying to say is, if we make this IWC setter, maybe we could remove shutdown and stick w/ only close(). That will at least remove a slew of warnings in eclipse that IndexWriter is an unclosed resource .

        Show
        Shai Erera added a comment - I guess what I'm trying to say is, if we make this IWC setter, maybe we could remove shutdown and stick w/ only close(). That will at least remove a slew of warnings in eclipse that IndexWriter is an unclosed resource .
        Hide
        Robert Muir added a comment -

        Actually, I rather like the change in semantics (from the current (on trunk) weird "throw exception or not in close telling you if you lost changes", to "commit on close").

        And ... the default makes me nervous: can we go back to 4.x's default? Ie, by default close will wait for merges / commit, but you can disable this to make close == rollback by calling IWC.setCommitOnClose(false).

        If we do this, I think we need to fix close to call shutdown(true) when commitOnClose is true, else rollback, and it no longer throws any exceptions about changes being lost

        +1

        This gives the option for advanced users but prevents any scary mailing lists messages about people losing documents because they passed VERSION_CURRENT.

        Show
        Robert Muir added a comment - Actually, I rather like the change in semantics (from the current (on trunk) weird "throw exception or not in close telling you if you lost changes", to "commit on close"). And ... the default makes me nervous: can we go back to 4.x's default? Ie, by default close will wait for merges / commit, but you can disable this to make close == rollback by calling IWC.setCommitOnClose(false). If we do this, I think we need to fix close to call shutdown(true) when commitOnClose is true, else rollback, and it no longer throws any exceptions about changes being lost +1 This gives the option for advanced users but prevents any scary mailing lists messages about people losing documents because they passed VERSION_CURRENT.
        Hide
        Michael McCandless added a comment -

        I guess what I'm trying to say is, if we make this IWC setter, maybe we could remove shutdown and stick w/ only close().

        +1

        Show
        Michael McCandless added a comment - I guess what I'm trying to say is, if we make this IWC setter, maybe we could remove shutdown and stick w/ only close(). +1
        Hide
        Ryan Ernst added a comment -

        Thanks for all the ideas! This new patch removes shutdown() (as a public method), and makes close() act based on commitOnClose. There are a couple things still to fix:

        • I'm unsure if TestIndexWriter.testCloseWhileMergeIsRunning is really doing anything (nothing waits on mergeStarted? and the exc check of the old behavior was the only assertion?)
        • TestIndexWriterMerging.testNoWaitClose hangs (I haven't looked at this at all yet)
        • I haven't checked the latest changes against Solr yet.
        Show
        Ryan Ernst added a comment - Thanks for all the ideas! This new patch removes shutdown() (as a public method), and makes close() act based on commitOnClose . There are a couple things still to fix: I'm unsure if TestIndexWriter.testCloseWhileMergeIsRunning is really doing anything (nothing waits on mergeStarted? and the exc check of the old behavior was the only assertion?) TestIndexWriterMerging.testNoWaitClose hangs (I haven't looked at this at all yet) I haven't checked the latest changes against Solr yet.
        Hide
        Michael McCandless added a comment -

        Hmm, patch doesn't quite cleanly apply on trunk?

        Show
        Michael McCandless added a comment - Hmm, patch doesn't quite cleanly apply on trunk?
        Hide
        Michael McCandless added a comment -

        Oh, nevermind, I see: the 2nd patch isn't cumulative ... once I applied 1st patch, 2nd patch applied fine.

        Show
        Michael McCandless added a comment - Oh, nevermind, I see: the 2nd patch isn't cumulative ... once I applied 1st patch, 2nd patch applied fine.
        Hide
        Michael McCandless added a comment -

        Here's a new patch w/ just a couple changes over the first 2:

        • Default commitOnClose to true
        • Fixed test failures in TestIndexWriterWithThreads

        Lucene tests seem to pass with this patch ...

        Show
        Michael McCandless added a comment - Here's a new patch w/ just a couple changes over the first 2: Default commitOnClose to true Fixed test failures in TestIndexWriterWithThreads Lucene tests seem to pass with this patch ...
        Hide
        Michael McCandless added a comment -

        Woops, I forgot one more change:

        • I fixed IW.close, in the commitOnClose=true case, to not call shutdown if the IW is already closed.
        Show
        Michael McCandless added a comment - Woops, I forgot one more change: I fixed IW.close, in the commitOnClose=true case, to not call shutdown if the IW is already closed.
        Hide
        Ryan Ernst added a comment -

        Here's another patch that just fixes one test (I don't know why it didn't show up as a compile error before, it may have been a result of a recent sync up with trunk).

        I am still getting a hang with TestIndexWriterMerging.testNoWaitClose, but only when I run ant test within lucene/core, not when I run that test individually (or the suite).

        Show
        Ryan Ernst added a comment - Here's another patch that just fixes one test (I don't know why it didn't show up as a compile error before, it may have been a result of a recent sync up with trunk). I am still getting a hang with TestIndexWriterMerging.testNoWaitClose, but only when I run ant test within lucene/core, not when I run that test individually (or the suite).
        Hide
        Ryan Ernst added a comment -

        Ok, another patch with the sometimes failing tests now passing (Mike beasted many iterations). Solr tests also pass.

        I'll commit Friday afternoon PST if there are no objections.

        Show
        Ryan Ernst added a comment - Ok, another patch with the sometimes failing tests now passing (Mike beasted many iterations). Solr tests also pass. I'll commit Friday afternoon PST if there are no objections.
        Hide
        ASF subversion and git services added a comment -

        Commit 1617004 from Ryan Ernst in branch 'dev/trunk'
        [ https://svn.apache.org/r1617004 ]

        LUCENE-5871: Remove Version from IndexWriterConfig

        Show
        ASF subversion and git services added a comment - Commit 1617004 from Ryan Ernst in branch 'dev/trunk' [ https://svn.apache.org/r1617004 ] LUCENE-5871 : Remove Version from IndexWriterConfig
        Hide
        Michael McCandless added a comment -

        I'd like to backport the improvement to IW.close to 4.x, even though the rest of this is 5.0 ... that improvement makes IW.close's behavior on exception sane (it's actually closed)...

        Show
        Michael McCandless added a comment - I'd like to backport the improvement to IW.close to 4.x, even though the rest of this is 5.0 ... that improvement makes IW.close's behavior on exception sane (it's actually closed)...
        Hide
        Michael McCandless added a comment -

        Here's a patch for fixing IW.close on 4.x ... I also fixed trunk's close a bit (put back the shouldClose() call), and added a couple test cases.

        For 4.x, I also back-ported a bug fix + test that snuck in with LUCENE-4246.

        I think it's ready...

        Show
        Michael McCandless added a comment - Here's a patch for fixing IW.close on 4.x ... I also fixed trunk's close a bit (put back the shouldClose() call), and added a couple test cases. For 4.x, I also back-ported a bug fix + test that snuck in with LUCENE-4246 . I think it's ready...
        Hide
        ASF subversion and git services added a comment -

        Commit 1617961 from Michael McCandless in branch 'dev/trunk'
        [ https://svn.apache.org/r1617961 ]

        LUCENE-5871: one one thread can IW.close at once

        Show
        ASF subversion and git services added a comment - Commit 1617961 from Michael McCandless in branch 'dev/trunk' [ https://svn.apache.org/r1617961 ] LUCENE-5871 : one one thread can IW.close at once
        Hide
        ASF subversion and git services added a comment -

        Commit 1617991 from Michael McCandless in branch 'dev/trunk'
        [ https://svn.apache.org/r1617991 ]

        LUCENE-5871: fix silly test bug

        Show
        ASF subversion and git services added a comment - Commit 1617991 from Michael McCandless in branch 'dev/trunk' [ https://svn.apache.org/r1617991 ] LUCENE-5871 : fix silly test bug
        Hide
        ASF subversion and git services added a comment -

        Commit 1618062 from Michael McCandless in branch 'dev/branches/branch_4x'
        [ https://svn.apache.org/r1618062 ]

        LUCENE-5871: IW.close really closes the IndexWriter, even on exception

        Show
        ASF subversion and git services added a comment - Commit 1618062 from Michael McCandless in branch 'dev/branches/branch_4x' [ https://svn.apache.org/r1618062 ] LUCENE-5871 : IW.close really closes the IndexWriter, even on exception
        Hide
        ASF subversion and git services added a comment -

        Commit 1618063 from Michael McCandless in branch 'dev/trunk'
        [ https://svn.apache.org/r1618063 ]

        LUCENE-5871: test bugs

        Show
        ASF subversion and git services added a comment - Commit 1618063 from Michael McCandless in branch 'dev/trunk' [ https://svn.apache.org/r1618063 ] LUCENE-5871 : test bugs
        Hide
        ASF subversion and git services added a comment -

        Commit 1619623 from Use account "steve_rowe" instead in branch 'dev/trunk'
        [ https://svn.apache.org/r1619623 ]

        LUCENE-5859, LUCENE-5871: Remove Version.LUCENE_CURRENT from javadocs

        Show
        ASF subversion and git services added a comment - Commit 1619623 from Use account "steve_rowe" instead in branch 'dev/trunk' [ https://svn.apache.org/r1619623 ] LUCENE-5859 , LUCENE-5871 : Remove Version.LUCENE_CURRENT from javadocs
        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:
            Ryan Ernst
            Reporter:
            Ryan Ernst
          • Votes:
            0 Vote for this issue
            Watchers:
            4 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development